Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Scheduler trait #1035

Merged
merged 31 commits into from Oct 3, 2023

Conversation

tsoutsman
Copy link
Member

@tsoutsman tsoutsman commented Sep 4, 2023

Adds the Scheduler trait to task which can be used to dynamically switch between schedulers, akin to SELECT_NEXT_TASK_FUNC but with more functionality. Merges the runqueue_* and scheduler_* crates as a byproduct because now the state must be stored with policy.

The PR makes run queues (i.e. schedulers) more opaque by removing the ability to directly access a run queue. I think this is the way we want to go because it prevents bugs such as #1000. Realistically, users shouldn't have complete access over the run queue.

Also I'm not sure about the API for task::scheduler. I was thinking instead of the function variants (e.g. add_task add_task_to, add_task_to_current) to instead have something like

// Or some better name
pub enum SchedulerSpecifier {
    Any,
    Specific(CpuId),
    Current,
}

pub fn add_task(task: TaskRef, scheduler: SchedulerSpecifier) {
    // ...
}

but I'm also fine with the current API.

Depends on #1036.

Signed-off-by: Klimenty Tsoutsman <klim@tsoutsman.com>
Signed-off-by: Klimenty Tsoutsman <klim@tsoutsman.com>
Signed-off-by: Klimenty Tsoutsman <klim@tsoutsman.com>
@tsoutsman tsoutsman marked this pull request as draft September 4, 2023 05:33
Signed-off-by: Klimenty Tsoutsman <klim@tsoutsman.com>
Signed-off-by: Klimenty Tsoutsman <klim@tsoutsman.com>
Signed-off-by: Klimenty Tsoutsman <klim@tsoutsman.com>
Signed-off-by: Klimenty Tsoutsman <klim@tsoutsman.com>
There are no guarantees about the value of the interrupt flag when
context switching. If the context switch is voluntary, i.e. a thread
called `schedule`, interrupts will most likely be enabled, whereas
if a thread is preempted, interrupts will be disabled. But this means
that if a preempted thread A switches to a thread B that voluntarily
yielded, thread B will return from the call to `schedule` with
interrupts disabled.

The AArch64 code also needs to be modified but I'll leave that to
@NathanRoyer.

Signed-off-by: Klimenty Tsoutsman <klim@tsoutsman.com>
@tsoutsman tsoutsman force-pushed the idle-task-in-cpu-local branch 2 times, most recently from 0eae81f to 41c7692 Compare September 6, 2023 13:10
Signed-off-by: Klimenty Tsoutsman <klim@tsoutsman.com>
Signed-off-by: Klimenty Tsoutsman <klim@tsoutsman.com>
Signed-off-by: Klimenty Tsoutsman <klim@tsoutsman.com>
Signed-off-by: Klimenty Tsoutsman <klim@tsoutsman.com>
Signed-off-by: Klimenty Tsoutsman <klim@tsoutsman.com>
Signed-off-by: Klimenty Tsoutsman <klim@tsoutsman.com>
Signed-off-by: Klimenty Tsoutsman <klim@tsoutsman.com>
Signed-off-by: Klimenty Tsoutsman <klim@tsoutsman.com>
Signed-off-by: Klimenty Tsoutsman <klim@tsoutsman.com>
Signed-off-by: Klimenty Tsoutsman <klim@tsoutsman.com>
@tsoutsman tsoutsman marked this pull request as ready for review September 8, 2023 16:19
@tsoutsman tsoutsman marked this pull request as draft September 8, 2023 16:56
@tsoutsman
Copy link
Member Author

Forgot to add the single_simd_task_optimisation config blocks back. Will add that tomorrow.

@tsoutsman tsoutsman marked this pull request as ready for review September 8, 2023 20:54
Signed-off-by: Klimenty Tsoutsman <klim@tsoutsman.com>
Signed-off-by: Klimenty Tsoutsman <klim@tsoutsman.com>
kernel/scheduler_epoch/src/lib.rs Outdated Show resolved Hide resolved
kernel/scheduler_epoch/src/lib.rs Outdated Show resolved Hide resolved
kernel/scheduler_epoch/src/lib.rs Outdated Show resolved Hide resolved
.clone()
.into_iter()
.map(|priority_task| priority_task.task)
.collect()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for this and the other impls of dump, i'd vote to change the signature to return an iterator over TaskRefs as Items rather than forcibly collect() it into a Vec. No need to eagerly collect if the caller is just going to iterate anyway.

Could also re-name it more accurately to something like task_iter, or a similar name of your choosing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't return an associated type because then the trait wouldn't be object safe.

Could also re-name it more accurately to something like task_iter, or a similar name of your choosing.

As I've stated above it can't return an iterator so I've renamed it to tasks because it returns a vec of the tasks in the run queue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I didn't mean to suggest using an associated type like my old scheduler refactor design, but rather an impl Iterator<...>. But I just realized that I had only recently heard about RPITIT being supported in soon-to-come future versions of Rust, so I was mixing up the fact that RPITIT support is coming soon with the fact that it doesn't actually exist yet in the version of Rust we're pinned to.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually it appears that it is already supported on nightly -- but we can save that for another PR.

kernel/scheduler_priority/src/lib.rs Outdated Show resolved Hide resolved
kernel/task/src/scheduler.rs Outdated Show resolved Hide resolved
tsoutsman and others added 7 commits October 3, 2023 22:57
Co-authored-by: Kevin Boos <1139460+kevinaboos@users.noreply.github.com>
Signed-off-by: Klimenty Tsoutsman <klim@tsoutsman.com>
Signed-off-by: Klimenty Tsoutsman <klim@tsoutsman.com>
Signed-off-by: Klimenty Tsoutsman <klim@tsoutsman.com>
Signed-off-by: Klimenty Tsoutsman <klim@tsoutsman.com>
Signed-off-by: Klimenty Tsoutsman <klim@tsoutsman.com>
Signed-off-by: Klimenty Tsoutsman <klim@tsoutsman.com>
@kevinaboos kevinaboos merged commit 810e12f into theseus-os:theseus_main Oct 3, 2023
3 checks passed
github-actions bot pushed a commit that referenced this pull request Oct 3, 2023
…it (#1035)

* Add the `Scheduler` trait, which represents a scheduler policy.
  * By default, there is one instance of a `Scheduler` policy per CPU.
  * Support dynamically registering and switching between
    different scheduler policies on a per-CPU basis.
  * Because this is now defined in the `task` crate instead of in the
    `scheduler` crate, the `task` crate can now access all functionality
    provided by a scheduler policy, which allows for custom actions
    like setting priority or removing/adding blocked/unblocked tasks
    to/from a runqueue.

* Combine `runqueue_*` crates with their respective  and `scheduler_*` crates,
  which is an improved design because external crates should not be able to
  view or modify a scheduler policy's internal runqueue contents.
  * This design makes sense, and also prevents issues like #1000.

* Modify most applications that access runqueues via the old `runqueue_*`
  crate APIs to go through the new `Scheduler` API instead.

----------
Signed-off-by: Klimenty Tsoutsman <klim@tsoutsman.com>
Co-authored-by: Kevin Boos <kevinaboos@gmail.com> 810e12f
@kevinaboos kevinaboos mentioned this pull request Oct 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants