Skip to content
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

Merged
merged 4 commits into from Dec 23, 2021

Conversation

topecongiro
Copy link
Contributor

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:

  • Core id is not consecutive within a single NUMA node (e.g., 0, 1, 2, 4, 5, 6 instead of 0, 1, 2, 3, 4, 5)
  • Each node may have different number of core

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

@glommer
Copy link
Collaborator

glommer commented Dec 12, 2021

Hi. A small comment, see please that this fails the CI test:

thread 'tests::check_formating' panicked at 'cargo fmt failed. Note that glommio uses nightly for formatting, so please invoke cargo with +nightly', glommio/tests/linters.rs:15:9

cargo test locally should have generated the same output.

Your reasoning for the design you chose makes sense. I will take a deeper look at the code now

}
}

if core_id_not_unique {
Copy link
Collaborator

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it works.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 4aac955.

@glommer
Copy link
Collaborator

glommer commented Dec 13, 2021

There are a lot of CI failures here that look suspicious. @HippoBaro can you take a look ?

@glommer
Copy link
Collaborator

glommer commented Dec 13, 2021

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.
I am okay with the PR as is right now, let me just hear from @HippoBaro our CI Lord about what's going on here.

@HippoBaro
Copy link
Member

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.

@HippoBaro
Copy link
Member

#483 is now merged; @topecongiro, if you rebase on master CI should be happier. Sorry for the delay!

@glommer
Copy link
Collaborator

glommer commented Dec 23, 2021

@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

@glommer glommer merged commit b91b820 into DataDog:master Dec 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants