-
Notifications
You must be signed in to change notification settings - Fork 187
[RFC] scx_layered: Add tickless layer support #2262
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
base: main
Are you sure you want to change the base?
Conversation
| struct layered_timer layered_timers[MAX_TIMERS] = { | ||
| {15LLU * NSEC_PER_SEC, CLOCK_BOOTTIME, 0}, | ||
| {1LLU * NSEC_PER_SEC, CLOCK_BOOTTIME, 0}, | ||
| {1LLU * NSEC_PER_MSEC, CLOCK_BOOTTIME, 0}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was chosen arbitrarily, should probably be more configurable but makes working with timers a little annoying
|
I'm curious if we boot the kernel with proper tickless options + isolcpus. Could we in theory run our selected user space tasks indefinitely? Or is there still some kernel threads that we'd need to yield once a while to avoid stalls? |
| } | ||
|
|
||
| bpf_for(layer_id, 0, nr_layers) { | ||
| if (cpuc->layer_id != layer_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cpu_ctx also has a task_layer_id field as well, but I think this is right because it's the cpu "owned" layer
I think that would be something that still needs to be tested, but IIRC affinitized kthreads can preempt. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few comments but LGTM
| return true; | ||
| } | ||
|
|
||
| static void tick_layer(struct cpu_ctx *cpuc, u32 layer_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe preempt_layer() to make it clear that it's not a tick callback?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tickless_kick_layer()?
| { | ||
| u64 dsq_id = layer_dsq_id(layer_id, cpuc->llc_id); | ||
| if (!scx_bpf_dsq_nr_queued(SCX_DSQ_LOCAL_ON | cpuc->cpu) && | ||
| !scx_bpf_dsq_nr_queued(dsq_id)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may also want to skip the kick if the CPU is idle or if the task running on it hasn't used enough time slice, but the code is simpler as it is and it's probably fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it also consider fallback DSQs?
|
|
||
| if (!enable_antistall) | ||
| return true; | ||
| return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be a separate patch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahh @hodgesds this change is noop right? i.e. that's just the return to the timer callback which does nothing IIUC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, can make it a separate patch... it prevents the timer from rerunning if antistall is disabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suspect the option to disable antistall ought to be removed btw
- also lol thx for letting me know that wasn't noop, thought return true was just verifier noise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I refactored the timer interface in #2266. If that makes sense then I'll use that to make the tickless interval configurable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suspect the option to disable antistall ought to be removed btw
* also lol thx for letting me know that wasn't noop, thought return true was just verifier noise.
Let's not do that, disabling antistall is very useful when validating a new scheduler release behaves, as the kicks are much better than tanking performance on a machine by 90% (or whatever) but staying running.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| return true; | ||
| } | ||
|
|
||
| static void tick_layer(struct cpu_ctx *cpuc, u32 layer_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tickless_kick_layer()?
| { | ||
| u64 dsq_id = layer_dsq_id(layer_id, cpuc->llc_id); | ||
| if (!scx_bpf_dsq_nr_queued(SCX_DSQ_LOCAL_ON | cpuc->cpu) && | ||
| !scx_bpf_dsq_nr_queued(dsq_id)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it also consider fallback DSQs?
f0b2522 to
7f2bc90
Compare
|
Rebased with the timer changes and borrowed some of the configuration from |
9bc93be to
759119b
Compare
|
Updated to use @arighi 's approach of preempting the tickless task during enqueue and no stalls when running with perf tests: without vs |
9e086a7 to
4e88203
Compare
Add a option to enable tickless scheduling on a layer. This may improve throughput in certain scenarios by reducing the number of context switches. Signed-off-by: Daniel Hodges <[email protected]>
4e88203 to
53e3b17
Compare
Add a option to enable tickless scheduling on a layer. This may improve throughput in certain scenarios by reducing the number of context switches.
Not sure this is a good idea, but why not?
bpftool prog traceTested with
schbenchand performance seemed ok:vs
eevdfUsing the following config: