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

Do not disable interrupts when building topology #435

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nealsid
Copy link
Contributor

@nealsid nealsid commented Aug 31, 2022

Currently, interrupts are disabled during part of the process for building CPU topology on macOS, but it is not clear to me why this is necessary, so this patch uses mp_rendezvous to run CPUID on all CPUs instead of mp_rendezvous_no_intrs.

@nealsid
Copy link
Contributor Author

nealsid commented Sep 8, 2022

This commit needs to be updated for conflicts, but, if someone could let me know if I'm on the right track, I could continue or not continue with this patch. Thank you.

@nealsid
Copy link
Contributor Author

nealsid commented Sep 9, 2022

BTW, if it's interesting what the output looks like on my 2019 MacBook Pro (Coffee Lake/i5-8257U), I've been testing by doing runs of pcm & pcm-core measuring DTLB misses that cause a page table walk (the event codes are taken from /usr/share/kpep/). However, real world benchmarking is still something I'm making progress on, and macOS doesn't seem to expose a way to set CPU affinity, although it does support L2 cache affinity for separate tasks, so that they are scheduled on the same CPU. Thanks!

Screenshot 2022-09-09 at 1 13 19 PM

@nealsid nealsid force-pushed the get-topo-with-interrupts-enabled branch from 8c0fcff to b08ecac Compare September 10, 2022 20:09
@opcm
Copy link
Contributor

opcm commented Sep 12, 2022

I am still trying to find a reviewer for your patch

@nealsid
Copy link
Contributor Author

nealsid commented Sep 12, 2022

I am still trying to find a reviewer for your patch

Appreciate it!

@nealsid
Copy link
Contributor Author

nealsid commented Sep 14, 2022

If it's helpful, it wasn't clear to me why executing CPUID would require interrupts to be disabled (or even need to be in the kernel, but then I realized it needs to execute on all CPUs to build the topology), because, AFAIUI, interrupts are generally disabled when taking a lock that might be required by an interrupt handler. Since there are variants of the mp_rendezvous function that disable or do not disable interrupts, I also assumed that the barrier functionality it provides does not depend on disabling interrupts. Thanks!

@nealsid
Copy link
Contributor Author

nealsid commented Sep 14, 2022

I started digging into this some more, and mp_rendezvous, internally, will disable interrupts on non "master" CPUs (the terminology is from the XNU source and I believe it refers to the startup CPU) while enqueueing the function to be run on non-local CPUs, in order to prevent topology changes while scheduling the function:

https://github.com/apple/darwin-xnu/blob/8f02f2a044b9bb1ad951987ef5bab20ec9486310/osfmk/i386/mp.c#L909
https://github.com/apple/darwin-xnu/blob/8f02f2a044b9bb1ad951987ef5bab20ec9486310/osfmk/i386/mp.c#L1323

So the references to disabling or enabling interrupts in mp_rendezvous/mp_rendezvous_no_intr are for while the function itself is running, and it will not cause a race condition with topology changes.

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.

2 participants