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.
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
ProcessorCount when running with docker cpuset #7798
Comments
@janvorli does the runtime check the available memory? If so, how does it do that? Maybe some change is needed there too. |
@tmds yes, it does. See the GlobalMemoryStatusEx in PAL. I am just in a process of adding swap file info to that. |
FYI: |
@janvorli Similar problem for memory, kernel APIs are not aware of the container. jdk9 has an experimental option, from https://bugs.openjdk.java.net/browse/JDK-8170888
|
This is the wrong answer, though. The number of processors the OS will allow a process to use at any one time / what it's affinitized to is different from the number of processors in the environment / on the machine. On Windows you can affinitize a process to only a subset of cores as well, but we still return from Environment.ProcessorCount the number of cores available to the OS and not just the ones to which the process is affinitized. |
I agree it's not perfect. It's a pragmatic solution mono and jdk have adopted to report an appropriate number of cpus in docker containers. |
Adding an alternative. Like the memory, cpu's can also be read from the cgroup filesystem.
|
@janvorli @stephentoub looking at the code, I see the clr is already aware of process affinity. I wonder if it is properly implemented for non-Windows? I see some For memory, I'm wondering if there is already a knob that allows to limit it. I haven't found one in the clr-configuration-knobs. |
@tmds Are you referring to the GC code? If that's the case, it is only for Windows and I am working on adding support for Unix at that place right now. |
In the gc code. I saw it here too in util.cpp: https://github.com/dotnet/coreclr/blob/d8e91d3febea933dce2bb2e0825ffcea1c76480d/src/utilcode/util.cpp#L1284 |
@tmds UNIX == FEATURE_PAL, so we are using the code you have referred to. |
I don't think it takes into account process affinity. |
@tmds that's what I meant - you have pointed to the call to GetSystemInfo which doesn't take affinity into account. |
👍 |
@janvorli @stephentoub perhaps the runtime can pick up the available memory from an envvar? It would then be the containers responsibility to set this var (e.g. by reading /sys/fs/cgroup/memory/memory.limit_in_bytes). What do you think? |
@tmds it seems to me that we should rather obtain that information from the cgroups directly. The benefit would be that we would not support just docker, but also the standalone usage of cgroups for limiting the resources. We have recently added getting physical memory limit via cgroups for GC purposes (see src/pal/src/misc/cgroup.cpp), so we can build on that. |
For memory, it seems everything is implemented to use the cgroup limits. This is handled in the gc code, GlobalMemoryStatusEx is not cgroup aware. |
Implemented in dotnet/coreclr#11742 |
When running in Docker on Linux, Environment.Processor returns the number of available logical processors in the host system. It doesn't take into account the limitations that were set on the container.
Dockerfile:
src/Program.cs:
src/app.csproj:
Executing:
The latter call should return 1.
Mono was fixed for this issue, see bug report: https://bugzilla.xamarin.com/show_bug.cgi?id=39279.
Number of processors is determined via sched_getaffinity: https://github.com/mono/mono/blob/17aa73d0ae216529b59d5aa3d0ee68007bb035c4/mono/utils/mono-proclib.c#L774-L780
Related issue for Android/ARM:
The mono_cpu_count function contains other special cases for PLATFORM_ANDROID, HOST_ARM and HOST_ARM64. On Android, it's preferable not to use _SC_NPROCESSORS_ONLN because the number of online processors varies for power saving. On ARM, the number of cpus returned by sched_getaffinity varies as well. Comments in the code suggests that the ARM&ANDROID cases can be handled together by reporting _SC_NPROCESSORS_CONF.
Comment from @akoeplinger:
The text was updated successfully, but these errors were encountered: