Fix temperature readings on Intel CPUs with heterogeneous core design #1269

dawidpotocki · 2023-07-17T04:27:30Z

I don't really know C, so it is what it is.

This code probably should be changed to also handle CPUs that use ,
instead of - in sysfs. Could probably then replace the code that just
blindly copies the temperatures of the first half to the second half.

Screenshots

Before

After

Example sysfs output of a Raptor Lake CPU (i7-13700K)

P-cores

==> /sys/devices/system/cpu/cpu0/topology/thread_siblings_list <==
0-1
==> /sys/devices/system/cpu/cpu1/topology/thread_siblings_list <==
0-1
==> /sys/devices/system/cpu/cpu2/topology/thread_siblings_list <==
2-3
==> /sys/devices/system/cpu/cpu3/topology/thread_siblings_list <==
2-3
==> /sys/devices/system/cpu/cpu4/topology/thread_siblings_list <==
4-5
==> /sys/devices/system/cpu/cpu5/topology/thread_siblings_list <==
4-5
==> /sys/devices/system/cpu/cpu6/topology/thread_siblings_list <==
6-7
==> /sys/devices/system/cpu/cpu7/topology/thread_siblings_list <==
6-7
==> /sys/devices/system/cpu/cpu8/topology/thread_siblings_list <==
8-9
==> /sys/devices/system/cpu/cpu9/topology/thread_siblings_list <==
8-9
==> /sys/devices/system/cpu/cpu10/topology/thread_siblings_list <==
10-11
==> /sys/devices/system/cpu/cpu11/topology/thread_siblings_list <==
10-11
==> /sys/devices/system/cpu/cpu12/topology/thread_siblings_list <==
12-13
==> /sys/devices/system/cpu/cpu13/topology/thread_siblings_list <==
12-13
==> /sys/devices/system/cpu/cpu14/topology/thread_siblings_list <==
14-15
==> /sys/devices/system/cpu/cpu15/topology/thread_siblings_list <==
14-15

E-cores

==> /sys/devices/system/cpu/cpu16/topology/thread_siblings_list <==
16
==> /sys/devices/system/cpu/cpu17/topology/thread_siblings_list <==
17
==> /sys/devices/system/cpu/cpu18/topology/thread_siblings_list <==
18
==> /sys/devices/system/cpu/cpu19/topology/thread_siblings_list <==
19
==> /sys/devices/system/cpu/cpu20/topology/thread_siblings_list <==
20
==> /sys/devices/system/cpu/cpu21/topology/thread_siblings_list <==
21
==> /sys/devices/system/cpu/cpu22/topology/thread_siblings_list <==
22
==> /sys/devices/system/cpu/cpu23/topology/thread_siblings_list <==
23

Fixes htop-dev#1048

Explorer09 · 2023-07-18T18:47:15Z

linux/LibSensors.c

@@ -261,6 +261,43 @@ void LibSensors_getCPUTemperatures(CPUData* cpus, unsigned int existingCPUs, uns
      goto out;
   }

+   /* Read sysfs to get HT/SMT cores and correctly distribute the temperature values */
+   if (coreTempCount < activeCPUs) {
+      double oldData[existingCPUs + 1];


Variable length array is not recommended in C.

Please avoid VLA. Although they are part of C99 (which htop targets), there are later language versions that make them optional. Also, having dynamically sized data on the stack is an accident waiting to happen, from a security PoV. Thus: No VLAs!

Sorry for a late response, but isn't this also a VLA?

htop/linux/LibSensors.c

Line 146 in 477c20b

double data[existingCPUs + 1];

That's where I got this from.

I don't mind changing it though.

The LibSensors code is not the cleanest in some places … ;-)

That said, I did some cleanup in #1273.

Explorer09 · 2023-07-18T18:49:50Z

linux/LibSensors.c

+      int dataCell = 1;
+      for (unsigned int i = 0; i < existingCPUs; i++) {
+         char path[100];
+         sprintf(path, "/sys/devices/system/cpu/cpu%d/topology/thread_siblings_list", i);


We have xSnprintf function in htop codebase. Please use that instead.

Also if you are iterating over several items in a common parent directory, it might be worth using things like opendir and openat.

BenBE

This code should best be split in a way that the gathered topology information can be re-used for frequency and temperatures alike.

Also, please take better care of defense-in-depth precautions to avoid security related issues.

BenBE · 2023-07-18T18:58:46Z

linux/LibSensors.c

+      memcpy(oldData, data, sizeof(oldData));
+
+      int dataCell = 1;
+      for (unsigned int i = 0; i < existingCPUs; i++) {


Use size_t for things that represent the number of items/counts of things.

BenBE · 2023-07-18T19:00:42Z

linux/LibSensors.c

+      int dataCell = 1;
+      for (unsigned int i = 0; i < existingCPUs; i++) {
+         char path[100];
+         sprintf(path, "/sys/devices/system/cpu/cpu%d/topology/thread_siblings_list", i);


Also if you are iterating over several items in a common parent directory, it might be worth using things like opendir and openat.

BenBE · 2023-07-18T19:01:53Z

linux/LibSensors.c

+      for (unsigned int i = 0; i < existingCPUs; i++) {
+         char path[100];
+         sprintf(path, "/sys/devices/system/cpu/cpu%d/topology/thread_siblings_list", i);
+         FILE* file = fopen(path, "r");


The fopen APIs are usually avoided due to their overhead and performance impact. As mentioned above, use opendir and openat (cf. wrappers in XUtils.h) instead.

BenBE · 2023-07-18T19:04:31Z

linux/LibSensors.c

+         char contents[10];
+         fgets(contents, 10, file);
+         fclose(file);
+
+         int start, end;
+         int rangeNums = sscanf(contents, "%d-%d", &start, &end);


This buffer is too short for the potential values that could be returned by the kernel. Should be at least 24 bytes (2x 10 bytes for int32_t plus 2 bytes for - and \n, plus one final \0 terminator.

BenBE · 2023-07-18T19:05:42Z

linux/LibSensors.c

+         int start, end;
+         int rangeNums = sscanf(contents, "%d-%d", &start, &end);


Prefer fixed width integer types and suffixes defined in inttypes.h.

BenBE · 2023-07-18T19:07:21Z

linux/LibSensors.c

+         int rangeNums = sscanf(contents, "%d-%d", &start, &end);
+
+         if (rangeNums == 1) {
+            data[i+1] = oldData[dataCell];


Inconsistent use of indexing source. When just one value given, this should equal to running the below code for just the start index, which is not the case at all.

BenBE · 2023-07-18T19:07:27Z

linux/LibSensors.c

+            dataCell++;
+         } else if (rangeNums == 2) {
+            for (int j = start; j <= end; j++) {
+               data[j+1] = oldData[dataCell];


Unchecked buffer overflow from untrusted/unsanitized input.

BenBE · 2023-07-18T19:12:30Z

linux/LibSensors.c

+         fgets(contents, 10, file);
+         fclose(file);
+
+         int start, end;


Use some sane initialization, even though sscanf will potentially assign those variables.

eworm-de · 2024-05-23T08:21:25Z

Hmm, this does not give the expected results for me. Actually it show the temperature for one core less than before.

BenBE · 2024-05-23T11:05:36Z

@dawidpotocki Do you mind updating this PR and incorporate the comments from the different reviews? TIA.

@eworm-de Mind to provide some dump to reproduce this?

eworm-de · 2024-05-23T11:42:13Z

Hmm, #1352 addresses the same issue. Does it make sense to focus on this topic in just one PR?

BenBE · 2024-05-23T11:51:46Z

Whichever fixes the open issues first … I do not have a particular preference right now …

eworm-de · 2024-05-23T12:03:26Z

Ok. So what info to provide from my system?

This is from sensors:

coretemp-isa-0000
Adapter: ISA adapter
Package id 0:  +47.0°C  (high = +100.0°C, crit = +100.0°C)
Core 0:        +44.0°C  (high = +100.0°C, crit = +100.0°C)
Core 4:        +44.0°C  (high = +100.0°C, crit = +100.0°C)
Core 8:        +42.0°C  (high = +100.0°C, crit = +100.0°C)
Core 12:       +44.0°C  (high = +100.0°C, crit = +100.0°C)
Core 16:       +45.0°C  (high = +100.0°C, crit = +100.0°C)
Core 17:       +45.0°C  (high = +100.0°C, crit = +100.0°C)
Core 18:       +45.0°C  (high = +100.0°C, crit = +100.0°C)
Core 19:       +45.0°C  (high = +100.0°C, crit = +100.0°C)
Core 20:       +43.0°C  (high = +100.0°C, crit = +100.0°C)
Core 21:       +43.0°C  (high = +100.0°C, crit = +100.0°C)
Core 22:       +43.0°C  (high = +100.0°C, crit = +100.0°C)
Core 23:       +43.0°C  (high = +100.0°C, crit = +100.0°C)

BAT1-acpi-0
Adapter: ACPI interface
in0:          17.34 V  
curr1:         0.00 A  

iwlwifi_1-virtual-0
Adapter: Virtual device
temp1:        +34.0°C  

nvme-pci-0100
Adapter: PCI adapter
Composite:    +38.9°C  (low  =  -5.2°C, high = +83.8°C)
                       (crit = +87.8°C)

acpitz-acpi-0
Adapter: ACPI interface
temp1:        +49.8°C  
temp2:        +45.8°C  
temp3:        +47.8°C  
temp4:        +43.8°C  
temp5:        +32.8°C

dawidpotocki · 2024-05-23T13:07:56Z

@BenBE I was going to update it ages ago, but stuff happened and forgot about this a bit.

I will not have time to work on this until at least July.

BenBE · 2024-05-23T13:16:19Z

@BenBE I was going to update it ages ago, but stuff happened and forgot about this a bit.

I will not have time to work on this until at least July.

No worries. Take the time you need.

There were quite a few comments that probably will need a bit of work to address properly. In any case: If you need help or if you have questions feel free to ask.

Fix temperature readings on Intel CPUs with heterogeneous core design

477c20b

Fixes htop-dev#1048

BenBE added the Linux 🐧 Linux related issues label Jul 17, 2023

Explorer09 reviewed Jul 18, 2023

View reviewed changes

BenBE requested changes Jul 18, 2023

View reviewed changes

BenBE added the needs-rebase Pull request needs to be rebased and conflicts to be resolved label May 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix temperature readings on Intel CPUs with heterogeneous core design #1269

Fix temperature readings on Intel CPUs with heterogeneous core design #1269

dawidpotocki commented Jul 17, 2023

Explorer09 Jul 18, 2023

BenBE Jul 18, 2023

dawidpotocki Jul 28, 2023 •

edited

Loading

BenBE Jul 28, 2023

Explorer09 Jul 18, 2023

BenBE Jul 18, 2023

BenBE left a comment

BenBE Jul 18, 2023

BenBE Jul 18, 2023

BenBE Jul 18, 2023

BenBE Jul 18, 2023

BenBE Jul 18, 2023

BenBE Jul 18, 2023

BenBE Jul 18, 2023

BenBE Jul 18, 2023

eworm-de commented May 23, 2024

BenBE commented May 23, 2024

eworm-de commented May 23, 2024

BenBE commented May 23, 2024

eworm-de commented May 23, 2024

dawidpotocki commented May 23, 2024

BenBE commented May 23, 2024

		int start, end;
		int rangeNums = sscanf(contents, "%d-%d", &start, &end);

Fix temperature readings on Intel CPUs with heterogeneous core design #1269

Are you sure you want to change the base?

Fix temperature readings on Intel CPUs with heterogeneous core design #1269

Conversation

dawidpotocki commented Jul 17, 2023

Screenshots

Example sysfs output of a Raptor Lake CPU (i7-13700K)

P-cores

E-cores

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dawidpotocki Jul 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenBE left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eworm-de commented May 23, 2024

BenBE commented May 23, 2024

eworm-de commented May 23, 2024

BenBE commented May 23, 2024

eworm-de commented May 23, 2024

dawidpotocki commented May 23, 2024

BenBE commented May 23, 2024

dawidpotocki Jul 28, 2023 •

edited

Loading