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

Use sysconf() to determine active and existing CPUs on Linux #1342

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

glaubitz
Copy link

@glaubitz glaubitz commented Dec 3, 2023

Instead of enumerating the number of active and existing CPUs on Linux by iterating over the pseudo-files below /sys/devices/system/cpu/, it is better to retrieve the number of active and existing CPUs using the sysconf() function using _SC_NPROCESSORS_ONLN and _SC_NPROCESSORS_CONF which will always report the correct numbers.

Fixes #757

Instead of enumerating the number of active and existing CPUs on Linux
by iterating over the pseudo-files below /sys/devices/system/cpu/, it
is better to retrieve the number of active and existing CPUs using the
sysconf() function using _SC_NPROCESSORS_ONLN and _SC_NPROCESSORS_CONF
which will always report the correct numbers.

Fixes htop-dev#757
@glaubitz
Copy link
Author

glaubitz commented Dec 3, 2023

This should be amended by a configuration option to hide offline CPUs.

@glaubitz glaubitz marked this pull request as draft December 3, 2023 21:21
if (i <= active)
this->cpuData[i].online = true;
else
this->cpuData[i].online = false;
Copy link
Member

Choose a reason for hiding this comment

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

This seems to assume that CPUs can only be offline from the end of the CPU numbering range (i.e. greater than active but less that existing) - I'm not certain that assumption is universally true. Isn't it possible to take an arbitrary CPU offline? (a quick google suggests yes, any CPU can be taken offline).

The existing code (which is supposed to do the same thing sysconf is doing internally in glibc AFAICS) will visit each CPU and test whether it is online or offline. It'd be best to understand where the current code gets it wrong than to replace it in this way (though certainly would been nice to remove all that code).

Copy link
Author

Choose a reason for hiding this comment

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

I have thought about this, too. However, I wasn't sure whether the mapping of the CPU array in htop actually directly matches the CPU enumeration of the Linux kernel. My impression was that htop just counts the number of available CPUs and displays them.

Copy link
Member

Choose a reason for hiding this comment

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

htop currently does not have a proper sense of CPU core mapping. That's also where most of the temperature mapping goes wrong. A proper solution would be to build a proper map of IDs to underlying (physical) CPUs and their threads.

Copy link
Author

Choose a reason for hiding this comment

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

And we need to find a way to reliably determine the online status of a CPU. Currently, it checks for the presence of an online file if I remember correctly which does not exist. Plus, the UI needs an option to hide offline CPUs as otherwise the large number of offline CPUs are cluttering the UI.

Copy link
Member

Choose a reason for hiding this comment

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

Those are two separate issues, but yes.

Copy link
Contributor

@C0rn3j C0rn3j Jan 30, 2024

Choose a reason for hiding this comment

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

I'm not certain that assumption is universally true.

It's not, LXD cpu.limit can assign any thread to the container, so it can for example give it one thread ID 10 and this code will not mark it online because 10 is not equal or more than 1.

lscpu shows things correctly.

[0] # lscpu     
Architecture:            x86_64
  CPU op-mode(s):        32-bit, 64-bit
  Address sizes:         46 bits physical, 48 bits virtual
  Byte Order:            Little Endian
CPU(s):                  48
  On-line CPU(s) list:   2,5,19,21,24,30
  Off-line CPU(s) list:  0,1,3,4,6-18,20,22,23,25-29,31-47

https://github.com/search?q=repo%3Autil-linux%2Futil-linux+is_cpu_online&type=code

So does /sys/devices/system/cpu/online:

2,5,19,21,24,30

But in current master many more threads are marked online than they should be, as per screenshots in the main issue.

This one marks too little in comparison.

@C0rn3j
Copy link
Contributor

C0rn3j commented Jan 30, 2024

Instead of enumerating the number of active and existing CPUs on Linux by iterating over the pseudo-files below /sys/devices/system/cpu/, it is better to retrieve the number of active and existing CPUs using the sysconf() function using _SC_NPROCESSORS_ONLN and _SC_NPROCESSORS_CONF which will always report the correct numbers.

I would like to point out that they seem to do the same iteration anyway, as per NOTES here, but I suppose it's better than reinventing the wheel.


This is broken when applying against the 3.3.0 release, at least on one of the machines I have access to, I have not tested against master but I don't see any relevant commits.

LXD 5.19 container with 6 threads using 5.15.0-58-generic kernel(a bit odd that temps on the 4 soon-to-be-missing threads are N/A):
image

+ this patch(missing 4 threads):
image


LXD 5.19 container with 4 threads using 5.4.0-113-generic kernel:
image

+ this patch(seemingly correct):
image

@glaubitz
Copy link
Author

This is broken when applying against the 3.3.0 release, at least on one of the machines I have access to, I have not tested against master but I don't see any relevant commits.

If this change breaks LXD, I would argue it's a bug either in sysconf() or LXD causing the former to return an incorrect number of online CPUs.

FWIW, I am not saying that my patch is the way to go. It's just a draft and I currently don't have any better idea to improve CPU enumeration. The current enumeration method on Linux is broken, too, however.

@C0rn3j
Copy link
Contributor

C0rn3j commented Jan 30, 2024

EDIT: the PR code is wrong, added a comment
EDIT2: And sent my own PR as the current logic is wrong anyway

sysconf works correctly on the container, what else can I check?

[130] # cat x.c; echo; gcc x.c -o x; ./x
#include <stdio.h>
#include <stdlib.h>
#include <sys/sysinfo.h>
#include <unistd.h>

long sysconf(int name);

int main(void) {
    int nproc_conf, nproc_onln;
    nproc_conf = sysconf(_SC_NPROCESSORS_CONF);
    nproc_onln = sysconf(_SC_NPROCESSORS_ONLN);
    printf("_SC: This system has %d processors configured and %d processors online.\n", nproc_conf, nproc_onln);
    printf("get_nprocs: This system has %d processors configured and %d processors available.\n", get_nprocs_conf(), get_nprocs());
    exit(EXIT_SUCCESS);
}

_SC: This system has 48 processors configured and 6 processors online.
get_nprocs: This system has 48 processors configured and 6 processors available.

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.

[sparc64] limit cpu shown on active (online) cpus only?
4 participants