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.

Sign up for GitHub

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

Jump to bottom

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
Draft

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 10 additions & 54 deletions linux/LinuxMachine.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,6 @@ in the source distribution for its full text.
#define O_PATH 010000000 // declare for ancient glibc versions
#endif

/* Similar to get_nprocs_conf(3) / _SC_NPROCESSORS_CONF
* https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/getsysstats.c;hb=HEAD
*/
static void LinuxMachine_updateCPUcount(LinuxMachine* this) {
unsigned int existing = 0, active = 0;
Machine* super = &this->super;
Expand All @@ -56,63 +53,22 @@ static void LinuxMachine_updateCPUcount(LinuxMachine* this) {
super->existingCPUs = 1;
}

DIR* dir = opendir("/sys/devices/system/cpu");
if (!dir)
return;

unsigned int currExisting = super->existingCPUs;

const struct dirent* entry;
while ((entry = readdir(dir)) != NULL) {
if (entry->d_type != DT_DIR && entry->d_type != DT_UNKNOWN)
continue;

if (!String_startsWith(entry->d_name, "cpu"))
continue;
existing = sysconf(_SC_NPROCESSORS_CONF);
active = sysconf(_SC_NPROCESSORS_ONLN);

char* endp;
unsigned long int id = strtoul(entry->d_name + 3, &endp, 10);
if (id == ULONG_MAX || endp == entry->d_name + 3 || *endp != '\0')
continue;
this->cpuData = xReallocArrayZero(this->cpuData, currExisting ? (currExisting + 1) : 0, existing + 1, sizeof(CPUData));
this->cpuData[0].online = true; /* average is always "online" */
currExisting = existing;

#ifdef HAVE_OPENAT
int cpuDirFd = openat(dirfd(dir), entry->d_name, O_DIRECTORY | O_PATH | O_NOFOLLOW);
if (cpuDirFd < 0)
continue;
#else
char cpuDirFd[4096];
xSnprintf(cpuDirFd, sizeof(cpuDirFd), "/sys/devices/system/cpu/%s", entry->d_name);
#endif

existing++;

/* readdir() iterates with no specific order */
unsigned int max = MAXIMUM(existing, id + 1);
if (max > currExisting) {
this->cpuData = xReallocArrayZero(this->cpuData, currExisting ? (currExisting + 1) : 0, max + /* aggregate */ 1, sizeof(CPUData));
this->cpuData[0].online = true; /* average is always "online" */
currExisting = max;
}

char buffer[8];
ssize_t res = xReadfileat(cpuDirFd, "online", buffer, sizeof(buffer));
/* If the file "online" does not exist or on failure count as active */
if (res < 1 || buffer[0] != '0') {
active++;
this->cpuData[id + 1].online = true;
} else {
this->cpuData[id + 1].online = false;
}

Compat_openatArgClose(cpuDirFd);
for (unsigned int i = 0; i < existing; i++) {
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 •
edited
Loading

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.

}

closedir(dir);

// return if no CPU is found
if (existing < 1)
return;

#ifdef HAVE_SENSORS_SENSORS_H
/* When started with offline CPUs, libsensors does not monitor those,
* even when they become online. */
Expand Down