From 2126c3ef0ecc6d1bdce6a0b20d65bb6d1b82a256 Mon Sep 17 00:00:00 2001 From: Mika Wahlroos Date: Wed, 9 Jun 2021 14:53:18 +0300 Subject: [PATCH 1/3] Linux: handle offline CPUs when getting frequencies Fixes: #648 If some CPUs have been turned offline, those CPUs will not appear in the active CPU count parsed from /proc/stat. Offline CPUs that are still present may still count in terms of sequential CPU IDs in both /proc/cpuinfo and the CPU entries in /sys/devices/system/cpu/. Reading the frequencies for the N active CPUs from CPUFreq information in /sys/devices/system/cpu/cpu[0..N-1] may thus give incorrect results if some of the CPUs [0..N-1] are actually offline. If e.g. CPUs 1 and 3 have been turned offline on a system with four CPU cores, the code would attempt to read the frequencies for the remaining two online CPUs from /sys/devices/system/cpu/cpu{0,1}, while the correct sysfs CPU entries from which to read the frequencies would be {0,2}. The code path that reads the frequencies from /proc/cpuinfo instead of CPUfreq suffers from a similar problem. This fixes the reporting of frequencies in case of offline CPUs being mixed within the range of online CPUs. --- linux/LinuxProcessList.c | 56 ++++++++++++++++++++++++++++++++++------ 1 file changed, 48 insertions(+), 8 deletions(-) diff --git a/linux/LinuxProcessList.c b/linux/LinuxProcessList.c index 79706c043..9167ce39c 100644 --- a/linux/LinuxProcessList.c +++ b/linux/LinuxProcessList.c @@ -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[64]; + + 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); +} + static int scanCPUFreqencyFromSysCPUFreq(LinuxProcessList* this) { - unsigned int cpus = this->super.cpuCount; + int configuredCPUs = getConfiguredCPUCount(); int numCPUsWithFrequency = 0; unsigned long totalFrequency = 0; @@ -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"); @@ -1859,14 +1897,14 @@ static int scanCPUFreqencyFromSysCPUFreq(LinuxProcessList* this) { if (fscanf(file, "%lu", &frequency) == 1) { /* 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; @@ -1876,6 +1914,7 @@ static int scanCPUFreqencyFromSysCPUFreq(LinuxProcessList* this) { } } + onlineCPUIndex++; } if (numCPUsWithFrequency > 0) @@ -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)) { @@ -1905,6 +1944,7 @@ 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) || @@ -1912,11 +1952,11 @@ static void scanCPUFreqencyFromCPUinfo(LinuxProcessList* this) { (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; From 07a59b495b9a0cd123c68a12d936ecccab8dd303 Mon Sep 17 00:00:00 2001 From: Mika Wahlroos Date: Wed, 9 Jun 2021 21:52:41 +0300 Subject: [PATCH 2/3] Use a more appropriate buffer size for path --- linux/LinuxProcessList.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/linux/LinuxProcessList.c b/linux/LinuxProcessList.c index 9167ce39c..feea00a9c 100644 --- a/linux/LinuxProcessList.c +++ b/linux/LinuxProcessList.c @@ -1829,7 +1829,7 @@ static bool cpuIsOnline(unsigned int cpuId) { * the CPU is probably online, so assume online by default */ bool online = 1; - char pathBuffer[64]; + char pathBuffer[48]; xSnprintf(pathBuffer, sizeof(pathBuffer), "/sys/devices/system/cpu/cpu%u/online", cpuId); From e9c1d8f7b166cdc481e764dcd2819e207739e87b Mon Sep 17 00:00:00 2001 From: Mika Wahlroos Date: Wed, 9 Jun 2021 22:05:28 +0300 Subject: [PATCH 3/3] Fix indentation --- linux/LinuxProcessList.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/linux/LinuxProcessList.c b/linux/LinuxProcessList.c index feea00a9c..2b23130f0 100644 --- a/linux/LinuxProcessList.c +++ b/linux/LinuxProcessList.c @@ -1825,24 +1825,24 @@ static inline double LinuxProcessList_scanCPUTime(ProcessList* super) { } 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]; + /* 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); + 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); + FILE* file = fopen(pathBuffer, "r"); + unsigned int readval; + if (file) { + if (fscanf(file, "%u", &readval) == 1) { + online = readval; } + fclose(file); + } - return online; + return online; } static int getConfiguredCPUCount(void) {