-
Notifications
You must be signed in to change notification settings - Fork 74
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
scx_layered: Add topology aware preemption #666
base: main
Are you sure you want to change the base?
Conversation
b7ae3d8
to
d0707cc
Compare
Example stats output:
|
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 looks good to me. I wonder if it's worth changing the stuff in the new if (!disable_topology)
into two separate functions? Might make things easier to follow.
rust/Cargo.lock
Outdated
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.
Is there a reason we weren't committing this before? Imo it should be committed, just surprising to see it now.
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'm not sure.... I think @likewhatevs recently removed the lock files from .gitignore so we could have reproducible builds. So maybe it hasn't been committed yet?
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 did remove the git ignore, but it is kinda weird one would be there at all tbh. Like a top-level Cargo.lock I'd get (workspace level), or a project level Cargo.lock I'd also get (project level).
In the middle seems off to me.
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 think I know what happened, I think I created this branch before the lock files and before some of the refactoring of the builds. Then after rebasing the old lock file was still around and I added it on accident, so should be safe to delete.
u32 cache_idx; | ||
u32 node_idx; |
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.
Did these swap for a reason?
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.
was thinking going from the least specific to the most specific for ordering, but either way is fine
if (!(cachec = lookup_cache_ctx(cctx->cache_idx)) || | ||
!(nodec = lookup_node_ctx(cctx->node_idx))) | ||
return; |
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: would it be clearer if we split these into two early returns?
if (!(cachec = lookup_cache_ctx(cctx->cache_idx)) || | |
!(nodec = lookup_node_ctx(cctx->node_idx))) | |
return; | |
if (!(cachec = lookup_cache_ctx(cctx->cache_idx))) | |
return; | |
if (!(nodec = lookup_node_ctx(cctx->node_idx))) | |
return; |
Add topology aware preemption that begins in the local LLC and attempts to preempt from cpus nearest in the topology. Signed-off-by: Daniel Hodges <hodges.daniel.scott@gmail.com>
Add stats for XLLC/XNUMA preemptions. Signed-off-by: Daniel Hodges <hodges.daniel.scott@gmail.com>
Use the cast_mask helper to clean up some of the bpf cpumask conversion code for preemption. Signed-off-by: Daniel Hodges <hodges.daniel.scott@gmail.com>
d0707cc
to
0185989
Compare
Signed-off-by: Daniel Hodges <hodges.daniel.scott@gmail.com>
Add topology aware preemption, which changes the order of preemption to first attempt to preempt CPUs in the local LLC, local NUMA node and finally across all NUMA nodes. This could probably be made a little more efficient in the use of the bpf cpumasks, but should work for now. The old logic is still available behind the
-t
(disable topology) flag. Resolves #659