Fix initial thread affinity on Linux

[janvorli]

On Linux, a new thread inherits the affinity mask of the thread
that created the new thread. This is a problem for background GC
threads that are created by one of the server GC threads that are
affinitized to single core.
This change adds resetting each new thread affinity to match the
current process affinity.
In addition to that, I've also fixed the extraction of the CPU count
that was using PID 0. While the doc says that 0 represents current process,
it in fact means current thread.
And as a small bonus, I've added caching of the value returned by
the PAL_GetLogicalCpuCountFromOS, since it cannot change during runtime.

[ezsilmar]

Hello,

Thanks for the fix! Just curious why the affinity is changed only after the thread starts? It could be done during pthread_create like this:

pthread_attr_setaffinity_np(&pthreadAttr, sizeof(cpu_set_t), &cpuSet);
iError = pthread_create(&pthread, &pthreadAttr, CPalThread::ThreadEntry, pNewThread);

Tested example (your fix is much cleaner though): criteo-forks@78e335d

[janvorli]

@ezsilmar interesting, I have not realized there is an attribute for setting the affinity. I like that solution better. Let me update the PR.

[tmds]

The main thread affinity and the process affinity are the same thing (on Linux).

Perhaps the process affinity can be captured at startup? In case the user changes the main thread affinity, we can still use the 'process affinity' for new threads.

Note that there is no API for the user to change thread affinity, so maybe not important to cover this.

[kevingosse]

Perhaps the process affinity can be captured at startup? In case the user changes the main thread affinity, we can still use the 'process affinity' for new threads.

I believe using the "current" main thread affinity for new threads is the right way to go. If a user wants to change the affinity of the process (for instance, to pin it to a single CPU on a multi-CPU architecture), it will be expected that new threads follow that affinity. If the original affinity is saved and the bgc threads are spawned after the user pins the process, then those threads would ignore the custom affinity and use all the CPUs. I don't think that's a desirable behavior.

[tmds]

If a user wants to change the affinity of the process (for instance, to pin it to a single CPU on a multi-CPU architecture), it will be expected that new threads follow that affinity. If the original affinity is saved and the bgc threads are spawned after the user pins the process, then those threads would ignore the custom affinity and use all the CPUs. I don't think that's a desirable behavior.

If there is an API that allows the user to change the process affinity, that api should also update original saved affinity. Then new threads will follow the 'process affinity' and not the 'main thread affinity'.

[janvorli]

@kevingosse, @tmds there is no managed API to set thread affinity. So an application would have to use pinvoke to do that at runtime.
But I think that if a user wanted to change the process affinity, the preferrable way would be to use the taskset command to launch the process with the limits already set.

[kevingosse]

I wasn't referring to any API, just to changing the affinity with taskset after the process is started. On Windows, if you changed the affinity of a process after it has started (using task manager of process explorer), you wouldn't expect threads spawned after this point to ignore the affinity you just set. My expectations would be the same for Linux: after changing all the threads affinity with taskset, I don't know why I would want new threads to be spawned with the original affinity.

Put in other words: having new threads inherit the affinity of their immediate parent leads to tricky edge cases (like the bgc threads). Still, I don't think inheritance should be discarded as a whole, we should just make sure that new threads inherit from the main thread (as this review is currently doing).

[janvorli]

@kevingosse makes sense.

[tmds]

I wasn't referring to any API, just to changing the affinity with taskset after the process is started.

I agree, this should work, and break with what I was suggesting.

[ezsilmar]

Do you plan to backport this fix to netcore 2.2? Performance boost it gives is quite noticeable.

For our internal build we will do that in any case, I can send a PR when it's done.

[tmds]

Do you plan to backport this fix to netcore 2.2? Performance boost it gives is quite noticeable.

What is causing the noticeable performance boost compared to 2.2? Does 2.2 affinitize the background GC threads to a single cpu?

[kevingosse]

Do you plan to backport this fix to netcore 2.2? Performance boost it gives is quite noticeable.

What is causing the noticeable performance boost compared to 2.2? Does 2.2 affinitize the background GC threads to a single cpu?

2.2 has the exact same bug, yes.

[Maoni0]

I'm unclear what the support policy is for 2.2 at this point. if it's an LTS then certainly it would be good to port it back. @jkotas, @RussKeldorph, can you help answer this?

[RussKeldorph]

https://dotnet.microsoft.com/platform/support/policy/dotnet-core

.NET Core 2.1 is LTS. 2.2 is not. This doesn't sound like the type of fix that would meet the bar for servicing, LTS or otherwise, but if you want to push for it I can help you propose it. Email me offline.

[Maoni0]

thanks @RussKeldorph. in that case I would only attempt a port request is someone actually wants it backported for a good reason :)