-
Notifications
You must be signed in to change notification settings - Fork 102
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
Upstream fix for OpenBSD cpuid stats #1622
base: master
Are you sure you want to change the base?
Conversation
#ifdef __OpenBSD__ | ||
// HW_NCPUONLINE accounts for hyperthreading off/on | ||
int mib[2] = { CTL_HW, HW_NCPUONLINE }; | ||
#else | ||
int mib[2] = { CTL_HW, HW_NCPU }; |
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.
perhaps regular linux could use the new constant as well? is it available and kosher?
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.
Linux seems to use something similar judging from the variable name _SC_NPROCESSORS_ONLN
, see https://github.com/beyond-all-reason/spring/blob/BAR105/rts/lib/libcpuid/libcpuid/cpuid_main.c#L253. This segment with HW_NCPU
is for various *BSD OS, QNX. Just looking at FreeBSD's sysctl(3) man page, they don't seem to have this kind of MIB entry 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.
Looks decent. There's a merge freeze though.
@@ -290,7 +290,12 @@ static bool set_cpu_affinity(logical_cpu_t logical_cpu) | |||
|
|||
static int get_total_cpus(void) | |||
{ | |||
#ifdef __OpenBSD__ | |||
// HW_NCPUONLINE accounts for hyperthreading off/on |
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.
// HW_NCPUONLINE accounts for hyperthreading off/on | |
// HW_NCPUONLINE accounts for hyperthreading off/on, | |
// not available on regular linux |
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 isn't related to linux. It looks like linux uses sysconf as I linked above, with different header definitions. This code section is inside:
#if defined __FreeBSD__ || defined __OpenBSD__ || defined __NetBSD__ || defined __bsdi__ || defined __QNX__
In my opinion the HW_NCPUONLINE is clearly enough expressed as OpenBSD-only by the #ifdef __OpenBSD__
right before it, but if you feel strongly that this needs stating, then it should be something like:
// not available on FreeBSD/NetBSD/bsdi/QNX
or
// only available on OpenBSD
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.
Alright, in that case it's fine as-is IMO.
These changes are for libcpuid. Shouldn't these changes be submitted to that project? If we update to a later version we'd need to re-apply these changes? |
I will see about approaching upstream libcpuid... |
Use
HW_NCPUONLINE
for OpenBSD CPU count and implementcpu_clock_by_os
.HW_NCPUONLINE
is the preferred count on OpenBSD;HW_NCPU
would count virtual hyperthreading cores that might be deactivated.