-
Notifications
You must be signed in to change notification settings - Fork 163
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
Support CPU topology with non-unique core ids #480
Conversation
Hi. A small comment, see please that this fails the CI test:
Your reasoning for the design you chose makes sense. I will take a deeper look at the code now |
glommio/src/sys/hardware_topology.rs
Outdated
} | ||
} | ||
|
||
if core_id_not_unique { |
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 would work as well if the core id is unique, right?
To keep things simpler and easier to reason, I'd just invoke this all the time so we have the same code paths in all cases.
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.
Yes, it works.
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.
Fixed in 4aac955.
There are a lot of CI failures here that look suspicious. @HippoBaro can you take a look ? |
This also has the issue with the cargo-deadlinks you pointed at. My best guess is that we are not pinning the rust version, and it now started failing as CI picks up a new version. |
Sounds very plausible! I'm AFK until tomorrow, but I will take a look ASAP. In the meantime, if you are confident this is valid code, we can bypass CI and merge. @glommer let me know how that sounds. |
#483 is now merged; @topecongiro, if you rebase on master CI should be happier. Sorry for the delay! |
@HippoBaro for cases like that we can do the update directly on github and if there are no conflicts merge right away. It's one button =) I just did that and Ci is running on it now |
What does this PR do?
Support CPU topology with non-unique core ids.
The basic strategy is to sort CPUs by their (NUMA node id, core id) and assign virtual core id in this order. Note we need to ensure that the CPUs on the same core will have the same core id.
Motivation
To make
cargo test
pass in my environment.Related issues
Additional Notes
I first tried to fix the issue by calculating the virtual core id from the equation
core id + a number of cores in a NUMA node * NUMA node id
; this did not work for the following reasons:0, 1, 2, 4, 5, 6
instead of0, 1, 2, 3, 4, 5
)Checklist
[] I have added unit tests to the code I am submitting
[] My unit tests cover both failure and success scenarios
[] If applicable, I have discussed my architecture