Fix getting affinity set on MUSL on Jetson TX2 #206

janvorli · 2019-11-22T00:28:02Z

The code in PAL_GetCurrentThreadAffinitySet relied on the fact that the
number of processors reported as configured in the system is always
larger than the maximum CPU index. However, it turns out that it is not
true on some devices / distros. The Jetson TX2 reports CPUs 0, 3, 4 and
5 in the affinity mask and the 1 and 2 are never reported. GLIBC reports
6 as the number of configured CPUs, however MUSL reports just 4. The
PAL_GetCurrentThreadAffinitySet was using the number of CPUs reported as
configured as the upper bound for scanning affinity set, so on Jetson
TX2, the affinity mask returned had just two bits set while there were
4 CPUs. That triggered an assert in the GCToOSInterface::Initialize.

This change fixes that by reading the maximum CPU index from the
/proc/cpuinfo. It falls back to using the number of processors
configured when the /proc/cpuinfo is not available (on macOS, FreeBSD, ...)

Fixes #170

The code in PAL_GetCurrentThreadAffinitySet relied on the fact that the number of processors reported as configured in the system is always larger than the maximum CPU index. However, it turns out that it is not true on some devices / distros. The Jetson TX2 reports CPUs 0, 3, 4 and 5 in the affinity mask and the 1 and 2 are never reported. GLIBC reports 6 as the number of configured CPUs, however MUSL reports just 4. The PAL_GetCurrentThreadAffinitySet was using the number of CPUs reported as configured as the upper bound for scanning affinity set, so on Jetson TX2, the affinity mask returned had just two bits set while there were 4 CPUs. That triggered an assert in the GCToOSInterface::Initialize. This change fixes that by reading the maximum CPU index from the /proc/cpuinfo. It falls back to using the number of processors configured when the /proc/cpuinfo is not available (on macOS, FreeBSD, ...)

janvorli · 2019-11-22T00:28:14Z

cc: @jashook

jkotas · 2019-11-22T04:54:27Z

src/coreclr/src/pal/src/thread/thread.cpp

    {
        const SIZE_T BitsPerBitsetEntry = 8 * sizeof(UINT_PTR);
-        int nrcpus = PAL_GetTotalCpuCount();
+        int maxCpuIndex = GetMaxCpuIndex();


Can we just use CPU_COUNT macro instead of this?

CPU_COUNT counts number of set bits in the set, that's not what we want. We could possibly use the CPU_SETSIZE macro, however it would mean that we would scan 1024 bits (the current value of the macro). But maybe you are right, we call this function just once in the whole process lifetime, so it wouldn't hurt. And if we ever need to use it more, we can always add this optimization.

CPU_COUNT counts number of set bits in the set

You can potentially use it to exit the loop once all set bits are processed, without going all the way to 1024.

But I agree that looping till CPU_SETSIZE should be ok for one-time initialization like this.

jkotas · 2019-11-22T04:56:09Z

Also add matching change to src\gc\unix\gcenv.unix.cpp ?

lpereira · 2019-11-22T05:07:04Z

src/coreclr/src/pal/src/misc/sysinfo.cpp

+
+    if (maxCpuIndex == -1)
+    {
+        FILE* cpuInfoFile = fopen("/proc/cpuinfo", "r");


This should be under #ifdef __linux__ or something like this. (procfs exists in other Unices but cpuinfo is Linux-only.)

It is intentionally not like that. I try to avoid checking for specific platform if possible. This code is run just once and on non-linux platforms, it fails to open the file and uses the fallback path.

lpereira · 2019-11-22T05:07:38Z

src/coreclr/src/pal/src/misc/sysinfo.cpp

+            char *line = nullptr;
+            size_t lineLen = 0;
+
+            while (getline(&line, &lineLen, cpuInfoFile) != -1)


Any reason to not use fscanf() directly? Saves a malloc roundtrip.

We run this just once, so couple of mallocs don't hurt. And this is a pattern that we use at quite a number of places, so I wanted to be consistent. Also, for fscanf, I would need to use a fixed line buffer, assume max line size etc.

(No need to pass a buffer to fscanf() if all you're looking for is an integer with %d.)

lpereira · 2019-11-22T05:14:16Z

src/coreclr/src/pal/src/misc/sysinfo.cpp

+            while (getline(&line, &lineLen, cpuInfoFile) != -1)
+            {
+                int cpuIndex;
+                int fieldsParsed = sscanf(line, "processor : %d", &cpuIndex);


It's been a while since I looked at this, but the format of /proc/cpuinfo is completely different for each and every architecture supported by Linux. It just so happens that both x86 (32 and 64-bit) and ARM (32 and 64 bit) have similar structure there as far as lines matching that scanf() format string, but this is quite fragile.

A better way on Linux is to read /sys/devices/system/cpu/online (for the online CPUs) or /sys/devices/system/cpu/possible (for the possible CPUs).

Ok, I didn't know the format is so architecture specific that it may not contain the "processor:" line. Thank for pointing it out.

janvorli · 2019-11-22T10:17:22Z

Also add matching change to src\gc\unix\gcenv.unix.cpp ?

Right, we need it there too.

Remove getting the max CPU index and use CPU_SETSIZE instead. Since the function is executed just once during startup, it is fine.

jkotas · 2019-11-22T12:20:55Z

Do we also need to fix GetProcessorForHeap ? It also loops till GetTotalProcessorCount only.

janvorli · 2019-11-22T12:43:57Z

Do we also need to fix GetProcessorForHeap ? It also loops till GetTotalProcessorCount only.

You are right, we do.

The loop was scanning only bits below the total processor count

janvorli · 2019-11-22T12:58:51Z

@jkotas, I've fixed the GetProcessorForHeap too.
@lpereira In the end, I've removed getting the max cpu index and scan the whole set instead.

jkotas · 2019-11-22T16:58:15Z

Nit: We are going to actively discourage creating PR branches under dotnet/runtime.

https://github.com/dotnet/consolidation/blob/master/Documentation/issues-pr-management.md#what-will-be-dotnetruntimes-branch-policy

janvorli · 2019-11-22T17:20:11Z

@jkotas oops, that has happened by accident. I've cloned runtime into a new folder locally and forgotten to update the remotes appropriately.

Ports dotnet/runtime#206 to release/3.1. The code in PAL_GetCurrentThreadAffinitySet relied on the fact that the number of processors reported as configured in the system is always larger than the maximum CPU index. However, it turns out that it is not true on some devices / distros. The Jetson TX2 reports CPUs 0, 3, 4 and 5 in the affinity mask and the 1 and 2 are never reported. GLIBC reports 6 as the number of configured CPUs, however MUSL reports just 4. The PAL_GetCurrentThreadAffinitySet was using the number of CPUs reported as configured as the upper bound for scanning affinity set, so on Jetson TX2, the affinity mask returned had just two bits set while there were 4 CPUs. That triggered an assert in the GCToOSInterface::Initialize. This change fixes that by reading the maximum CPU index from the /proc/cpuinfo. It falls back to using the number of processors configured when the /proc/cpuinfo is not available (on macOS, FreeBSD, ...) Fixes dotnet/runtime#170

janvorli added the area-PAL-coreclr label Nov 22, 2019

janvorli added this to the 5.0 milestone Nov 22, 2019

janvorli requested review from jkotas and lpereira November 22, 2019 00:28

janvorli self-assigned this Nov 22, 2019

jkotas reviewed Nov 22, 2019

View reviewed changes

lpereira suggested changes Nov 22, 2019

View reviewed changes

Reflect PR feedback

d2b289e

Remove getting the max CPU index and use CPU_SETSIZE instead. Since the function is executed just once during startup, it is fine.

Fix GCToOSInterface::GetProcessorForHeap too

f86d8e4

The loop was scanning only bits below the total processor count

jkotas approved these changes Nov 22, 2019

View reviewed changes

janvorli merged commit 82d1049 into master Nov 22, 2019

janvorli deleted the fix-getcurrenthreadaffinityset-jetsontx2 branch November 22, 2019 16:53

janvorli mentioned this pull request Nov 29, 2019

Port to 3.1 - Fix getting affinity set on MUSL on Jetson TX2 dotnet/coreclr#27957

Merged

ghost locked as resolved and limited conversation to collaborators Dec 11, 2020

Fix getting affinity set on MUSL on Jetson TX2 #206

Fix getting affinity set on MUSL on Jetson TX2 #206

Uh oh!

Conversation

janvorli commented Nov 22, 2019

Uh oh!

janvorli commented Nov 22, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas commented Nov 22, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

janvorli commented Nov 22, 2019

Uh oh!

jkotas commented Nov 22, 2019

Uh oh!

janvorli commented Nov 22, 2019

Uh oh!

janvorli commented Nov 22, 2019

Uh oh!

jkotas commented Nov 22, 2019

Uh oh!

janvorli commented Nov 22, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants