-
Notifications
You must be signed in to change notification settings - Fork 183
[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 <hodgesd@meta.com>
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 trace
Tested with
schbench
and performance seemed ok:vs
eevdf
Using the following config: