Change scheduler to only store runnable tasks on run queue #1042
Conversation
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>
…to idle-task-in-cpu-local
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>
The new test is significantly more robust than the old one. As of right now, the test isn't particularly useful because we don't have task migration, but theseus-os#1042 adds implicit task migration when unblocking a task. Hence, the test has a focus on blocking and unblocking tasks. Signed-off-by: Klimenty Tsoutsman <klim@tsoutsman.com>
Signed-off-by: Klimenty Tsoutsman <klim@tsoutsman.com>
…n-queue Signed-off-by: Klimenty Tsoutsman <klim@tsoutsman.com>
Signed-off-by: Klimenty Tsoutsman <klim@tsoutsman.com>
When spawning a pinned task, `spawn` didn't previously set `inner.pinned_cpu`. This created problems in theseus-os#1042 because the scheduler didn't know that tasks were pinned and freely migrated them across cores. Signed-off-by: Klimenty Tsoutsman <klim@tsoutsman.com>
…ueue Signed-off-by: Klimenty Tsoutsman <klim@tsoutsman.com>
* When spawning a pinned task, `spawn` didn't previously set `inner.pinned_cpu` for the newly-created `Task`. * This is not currently a problem because the scheduler doesn't perform task migration across CPUs, but when that gets enabled (in #1042), it would cause the pinning choice to be ignore by the scheduler. Signed-off-by: Klimenty Tsoutsman <klim@tsoutsman.com>
* When spawning a pinned task, `spawn` didn't previously set `inner.pinned_cpu` for the newly-created `Task`. * This is not currently a problem because the scheduler doesn't perform task migration across CPUs, but when that gets enabled (in #1042), it would cause the pinning choice to be ignore by the scheduler. Signed-off-by: Klimenty Tsoutsman <klim@tsoutsman.com> cae8ca8
* The new `test_scheduler` is significantly more robust than the old one. Currently, the test isn't particularly useful because we don't have task migration enabled, but #1042 will add implicit task migration when unblocking a task. * Hence, the test currently focuses on blocking/unblocking tasks. * Add a function to iterate over all initialized CPUs. Signed-off-by: Klimenty Tsoutsman <klim@tsoutsman.com>
|
all PR dependencies have been merged in. 👍 |
* The new `test_scheduler` is significantly more robust than the old one. Currently, the test isn't particularly useful because we don't have task migration enabled, but #1042 will add implicit task migration when unblocking a task. * Hence, the test currently focuses on blocking/unblocking tasks. * Add a function to iterate over all initialized CPUs. Signed-off-by: Klimenty Tsoutsman <klim@tsoutsman.com> e9416d6
* The new `test_scheduler` is significantly more robust than the old one. Currently, the test isn't particularly useful because we don't have task migration enabled, but theseus-os#1042 will add implicit task migration when unblocking a task. * Hence, the test currently focuses on blocking/unblocking tasks. * Add a function to iterate over all initialized CPUs. Signed-off-by: Klimenty Tsoutsman <klim@tsoutsman.com> e9416d6
Signed-off-by: Klimenty Tsoutsman <klim@tsoutsman.com>
| let ExposedTask { task: mut new_task } = exposed; | ||
There was a problem hiding this comment.
first line seems good, second line seems wrong (extra whitespace for no reason?)
kernel/spawn/src/lib.rs
Outdated
| } else { | ||
| task::scheduler::add_task(task_ref.clone()); | ||
| } | ||
| if !self.idle & !self.blocked { |
| pub fn block(&self) -> Result<RunState, RunState> { | ||
| use RunState::{Blocked, Runnable}; | ||
|
|
||
| let run_state = self.0.task.runstate(); |
There was a problem hiding this comment.
Unless I'm missing something, I believe this (and the same logic in unblock()) simply obtains a copy of the task's current RunState and then modifies that copy; it doesn't actually modify the RunState of the task itself.
There was a problem hiding this comment.
I think a good way to avoid this is to split up the functions:
- in
task_struct::Task::block(), only the runstate is modified. Basically, this function as it was.- the functions in
task_struct::Taskmay need to be renamed.
- the functions in
- in
task::TaskRef::block(), it callstask_struct::Task::block()and then modifies the runqueue accordingly. unblock()is similar.
| Ok(Runnable) | ||
| } else if run_state.compare_exchange(Blocked, Blocked).is_ok() { | ||
| // warn!("Blocked an already blocked task: {:?}", self); | ||
| Ok(Blocked) |
There was a problem hiding this comment.
It looks like blocked tasks are not proactively removed from the runqueue they're on, but rather done lazily by each scheduler policy.
This is fine, as it's much faster, but it should be clearly documented somewhere (i.e., in the docs for the Scheduler trait's next() function) because it's a non-symmetric pair of operations. Otherwise, a scheduler policy implementor wouldn't know that it's okay to simply remove a task from the runqueue when it comes across one that is blocked.
| let locked = SCHEDULERS.lock(); | ||
|
|
||
| let mut min_busyness = usize::MAX; | ||
| let mut least_busy_index = None; | ||
|
|
||
| for (i, (_, scheduler)) in locked.iter().enumerate() { | ||
| let busyness = scheduler.lock().busyness(); | ||
| if busyness < min_busyness { | ||
| least_busy_index = Some(i); | ||
| min_busyness = busyness; | ||
| } | ||
| } |
There was a problem hiding this comment.
this function is incredibly slow. In general, I don't think we should ever really access SCHEDULERS except when initializing or changing scheduler policies, though we can think about that more later (and how to improve this, e.g., via a separate estimated utilization metric or something that avoids the need to iterate over all schedulers).
It's likely that using this function every time a task is unblocked is going to absolutely destroy performance... right? Also I imagine it will cause lots and lots of task migrations, which is generally something you want to keep to a minimum unless a given CPU core is at max utilization for a "long" time (which we currently don't track a metric for).
| while let Some(task) = self.out_of_tokens.pop() { | ||
| self.have_tokens.push_back(task); |
There was a problem hiding this comment.
nit: Is there a way to do this in one batch rather than item-by-item? I guess not when we're using VecDeque for one list and Vec for the other...?
| // This check prevents an interleaving where `TaskRef::unblock` wouldn't add | ||
| // the task back onto the run queue. `TaskRef::unblock` sets the run state and | ||
| // then checks `is_on_run_queue` so we have to do the inverse. | ||
| if unlikely(task.task.is_runnable()) { | ||
| if let Some(task) = self.add_epoch_task(task) { | ||
| return Some(task); | ||
| } | ||
| } |
There was a problem hiding this comment.
So, IIUC, the crux of the issue surrounding this is_on_run_queue() atomic boolean is that we need to do two things in one "atomic" step:
- change the task's runstate to
Runnable - add the task back to a runqueue
Since we cannot do that, you're using a separate atomic bool to indicate whether a task is on a runqueue. I understand this, but I don't love this design. It's complex without bringing any other benefits, and kind of a messy design choice, as it makes both current and future scheduler policies harder to write.
Ideally this shouldn't be a necessary step. Can we avoid this complexity via an alternative design that's a bit cleaner and doesn't burden the Scheduler policy implementor with additional concerns?
Side note: if we actually wanted to store some piece of state inside a task about whether that task is on a runqueue, we might as well store a reference to (or the ID of) the actual runqueue that it's currently on. This would at least make it faster to add the task back once it gets unblocked, which addresses my other comment as a plus.
Note that this inherently adds task migration as when a task is unblocked it can be added to any core.
Depends on #1035, #1044, #1045.