-
Notifications
You must be signed in to change notification settings - Fork 183
scx_p2dq: Acccount for non scx usage #1844
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
87a73f8
to
bf60129
Compare
This assumes the CPU was running full throttle when not in scx, but IIUC that includes time spent in the idle scheduling class, so isn't this going to skew accounting? |
Hmmm good observation... let me think about this some more may be able to test if |
45a0279
to
c98beb6
Compare
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.
Assuming PF_IDLE covers it, looks good.
|
||
UEI_DEFINE(uei); | ||
|
||
|
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.
Nit: Stray line?
if (!(cpuc = lookup_cpu_ctx(cpu))) | ||
return; | ||
|
||
if (is_idle_task(args->task)) { |
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.
Hm... I don't think ops.cpu_release()
is called if we're switching to a scheduling class below SCHED_EXT. See: https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/sched/ext.c#n3171
So this condition should be always false, unless I'm missing something...
if (!(cpuc = lookup_cpu_ctx(cpu))) | ||
return; | ||
|
||
if (is_idle_task(args->task)) { |
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.
A better approach would be to use ops.update_idle()
to track the idle transition.
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 completely missed that, let me try that!
Add accounting for non scx usage for LLC utilization. This should make load balancing decisions more accurate. Signed-off-by: Daniel Hodges <hodgesd@meta.com>
c98beb6
to
039760e
Compare
Nit: s/Acccount/Account |
Add accounting for non scx usage for LLC utilization. This should make load balancing decisions more accurate.