Skip to content

Linux: handle offline CPUs when getting frequencies #650

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

Closed
mwahlroos wants to merge 3 commits into from
Closed

Linux: handle offline CPUs when getting frequencies #650

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2126c3e
Linux: handle offline CPUs when getting frequencies
mwahlroos Jun 9, 2021
07a59b4
Use a more appropriate buffer size for path
mwahlroos Jun 9, 2021
e9c1d8f
Fix indentation
mwahlroos Jun 9, 2021
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
56 changes: 48 additions & 8 deletions linux/LinuxProcessList.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1824,8 +1824,33 @@ static inline double LinuxProcessList_scanCPUTime(ProcessList* super) {
return period;
}

static bool cpuIsOnline(unsigned int cpuId) {
/* if there's no sysfs file for the CPU node indicating online/offline status,
* the CPU is probably online, so assume online by default
*/
bool online = 1;
char pathBuffer[48];

xSnprintf(pathBuffer, sizeof(pathBuffer), "/sys/devices/system/cpu/cpu%u/online", cpuId);

FILE* file = fopen(pathBuffer, "r");
unsigned int readval;
if (file) {
if (fscanf(file, "%u", &readval) == 1) {
online = readval;
}
fclose(file);
}

return online;
}

static int getConfiguredCPUCount(void) {
return (int) sysconf(_SC_NPROCESSORS_CONF);
}
Comment on lines +1848 to +1850
Copy link
Copy Markdown
Member

@BenBE BenBE Jun 9, 2021

Choose a reason for hiding this comment

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

This should be available globally already IIRC.

Copy link
Copy Markdown
Author

@mwahlroos mwahlroos Jun 9, 2021

Choose a reason for hiding this comment

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

I tried looking for that but couldn't find it.

ProcessList has cpuCount, which is updated on Linux by LinuxProcessList_updateCPUcount, but that number only includes the number of active CPUs, and using it as the upper limit for the CPU id leads to precisely the problem that I'm trying to solve.

Any pointers to where this would be available globally?


static int scanCPUFreqencyFromSysCPUFreq(LinuxProcessList* this) {
unsigned int cpus = this->super.cpuCount;
int configuredCPUs = getConfiguredCPUCount();
Copy link
Copy Markdown
Member

@BenBE BenBE Jun 9, 2021

Choose a reason for hiding this comment

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

Try to cache that value and avoid requesting it on every run; occasional refresh is no problem though.

int numCPUsWithFrequency = 0;
unsigned long totalFrequency = 0;

Expand All @@ -1843,12 +1868,25 @@ static int scanCPUFreqencyFromSysCPUFreq(LinuxProcessList* this) {
return -1;
}

for (unsigned int i = 0; i < cpus; ++i) {
if (configuredCPUs < 0) {
/* may be -1 if getting the number of CPUs from sysconf failed */
return -1;
}

unsigned int cpus = (unsigned int) configuredCPUs;
unsigned int onlineCPUIndex = 0;

for (unsigned int i = 0; i < cpus && onlineCPUIndex <= this->super.cpuCount - 1; ++i) {
char pathBuffer[64];

/* omit CPUs that have been hotplugged off */
if (!cpuIsOnline(i))
continue;

xSnprintf(pathBuffer, sizeof(pathBuffer), "/sys/devices/system/cpu/cpu%u/cpufreq/scaling_cur_freq", i);

struct timespec start;
if (i == 0)
if (onlineCPUIndex == 0)
clock_gettime(CLOCK_MONOTONIC, &start);

FILE* file = fopen(pathBuffer, "r");
Copy link
Copy Markdown
Member

@BenBE BenBE Jun 9, 2021

Choose a reason for hiding this comment

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

Avoid fopen in favour of openat; opendir the path /sys/devices/system/cpu and only scan from there. May combine this with the above and opendir cpu%d additionally.

Expand All @@ -1859,14 +1897,14 @@ static int scanCPUFreqencyFromSysCPUFreq(LinuxProcessList* this) {
if (fscanf(file, "%lu", &frequency) == 1) {
Copy link
Copy Markdown
Member

@BenBE BenBE Jun 9, 2021

Choose a reason for hiding this comment

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

Use readfile API

/* convert kHz to MHz */
frequency = frequency / 1000;
this->cpus[i + 1].frequency = frequency;
this->cpus[onlineCPUIndex + 1].frequency = frequency;
numCPUsWithFrequency++;
totalFrequency += frequency;
}

fclose(file);

if (i == 0) {
if (onlineCPUIndex == 0) {
struct timespec end;
clock_gettime(CLOCK_MONOTONIC, &end);
const time_t timeTakenUs = (end.tv_sec - start.tv_sec) * 1000000 + (end.tv_nsec - start.tv_nsec) / 1000;
Expand All @@ -1876,6 +1914,7 @@ static int scanCPUFreqencyFromSysCPUFreq(LinuxProcessList* this) {
}
}

onlineCPUIndex++;
}

if (numCPUsWithFrequency > 0)
Expand All @@ -1889,9 +1928,9 @@ static void scanCPUFreqencyFromCPUinfo(LinuxProcessList* this) {
if (file == NULL)
return;

unsigned int cpus = this->super.cpuCount;
int numCPUsWithFrequency = 0;
double totalFrequency = 0;
int cpuIndex = -1;
int cpuid = -1;

while (!feof(file)) {
Expand All @@ -1905,18 +1944,19 @@ static void scanCPUFreqencyFromCPUinfo(LinuxProcessList* this) {
(sscanf(buffer, "processor : %d", &cpuid) == 1) ||
(sscanf(buffer, "processor: %d", &cpuid) == 1)
) {
cpuIndex++;
continue;
} else if (
(sscanf(buffer, "cpu MHz : %lf", &frequency) == 1) ||
(sscanf(buffer, "cpu MHz: %lf", &frequency) == 1) ||
(sscanf(buffer, "clock : %lfMHz", &frequency) == 1) ||
(sscanf(buffer, "clock: %lfMHz", &frequency) == 1)
) {
if (cpuid < 0 || (unsigned int)cpuid > (cpus - 1)) {
if (cpuid < 0 || cpuIndex < 0 || (unsigned int)cpuIndex > (this->super.cpuCount - 1)) {
continue;
}

CPUData* cpuData = &(this->cpus[cpuid + 1]);
CPUData* cpuData = &(this->cpus[cpuIndex + 1]);
/* do not override sysfs data */
if (isnan(cpuData->frequency)) {
cpuData->frequency = frequency;
Expand Down