Add CGroup CFS CPU limit support #12797

gboucher90 · 2017-07-13T16:15:14Z

Virtualization containers systems (such as Docker, Mesos, Kubernetes...) often relies on cgroup cpu to limit resources.

Currently "Environment.ProcessorCount" in such systems return the physical number of processors. The problem is that this variable is used to properly scale the thread pool.

In our case the cpu limit in Mesos was set to 2 while the machine had 24 processors. Then the application (running a Kestrel HTTP Server) performed very badly and was often irresponsive.

Memory limit was already correctly implemented. Thus it makes sense to do the same for CPU.

justinvp · 2017-07-13T18:07:11Z

src/gc/unix/gcenv.unix.cpp

@@ -530,6 +532,9 @@ uint32_t GCToOSInterface::GetCurrentProcessCpuCount()
    if (count == 0 || count > 64)
        count = 64;

+    if (cgroup.GetCpuLimit(&cgroupCpuCount)){


Is the opening brace missing a closing brace below?

Yes indeed sorry about that. I wonder how it could have passed all the checks.
I will update the code on Monday. Thank you!

Actually I was able to change it now

@gboucher90 This code (the files in src/gc/unix) doesn't build by default, you can run build.cmd <arch> <config> buildstandalonegc to test it and see if it builds.

Thank you! I was able to check the build for this target.

(I was going to push a fix for the broken build but I see you just dit it here 8b82312)

swgillespie · 2017-07-17T16:59:02Z

@dotnet-bot test Ubuntu Checked standalone_gc

swgillespie

Can you also bring your changes to this file to https://github.com/dotnet/coreclr/blob/master/src/pal/src/misc/cgroup.cpp ? There's some code duplication in this area because the GC has the option to build as a standalone dynamic library, and when it does so it does not depend on the runtime's PAL to interact with the operating system.

It's a little confusing at the moment (and I'm sorry for that, I'll strive to improve the docs a little), but when building standalone (build.cmd/sh <arch> <config> buildstandalonegc), the GC uses the files in src\gc\unix to provide its own implementation of GCToOSInterface instead of using the runtime's implementation of GCToOSInterface, which mostly delegates to the PAL (see the implementation here: https://github.com/dotnet/coreclr/blob/master/src/vm/gcenv.os.cpp#L353)

Also, if you want your changes to be visible from the runtime (the Environment.ProcessorCount) scenario, you'll need to bring your changes to the PAL since the runtime will be using it to talk to the underlying OS for the forseeable future.

The code changes themselves look good to me!

gboucher90 · 2017-07-17T22:05:05Z

Thank you for this thorough explanation it helped me a lot.

I made the changes ;)

swgillespie

The changes under src/gc look fine to me (assuming you've validated this locally). I'll defer to others (particularly @janvorli) for a review on the PAL code, I'm not particularly familiar with PAL conventions.

swgillespie · 2017-07-18T00:36:57Z

src/vm/gcenv.os.cpp

+#else
+   size_t cpuLimit;
+
+   if (PAL_GetCpuLimit(&cpuLimit))


Do you still need to do this if GetCurrentProcessCpuCount also checks the CPU limit?

Nop indeed this is useless

janvorli · 2017-07-19T10:54:10Z

@gboucher90 I'll take a look today or early tomorrow. I've just got back from a couple of weeks of vacation.

janvorli · 2017-07-20T22:29:07Z

src/gc/unix/cgroup.cpp

+        if (errno != 0)
+            goto done;      
+
+          result = true;


A nit - incorrect text alignment

janvorli · 2017-07-20T22:30:31Z

src/pal/inc/pal.h

@@ -2569,6 +2569,11 @@ PALAPI
 PAL_GetWorkingSetSize(size_t* val);

 PALIMPORT
+BOOL
+PALAPI
+PAL_GetCpuLimit(size_t* val);


Could you please change the parameter type to unsigned int? There is no need for it to be bitness specific.

janvorli · 2017-07-20T22:42:35Z

src/gc/unix/cgroup.cpp

+
+    static char* FindMemoryCgroupPath(){
+        char* memory_cgroup_path = nullptr;
+        char* memoryHierarchyMount = nullptr;


A nit - could you please be consistent with naming convention of the locals?

janvorli · 2017-07-20T22:50:55Z

src/gc/unix/cgroup.cpp

+        cpu_count = quota / period;
+        if (cpu_count >= 1 && cpu_count < UINT32_MAX){
+            *val = cpu_count;
+        } else {


A nit - the coding convention we use is to have the opening curly brace and also the else on separate lines like this:

if (x) { } else { }

janvorli · 2017-07-20T22:54:45Z

src/utilcode/util.cpp

+    size_t cpuLimit;
+
+    if (PAL_GetCpuLimit(&cpuLimit))
+       count = cpuLimit;


The limit should only limit the count, but it should not be able to provide a larger value than the real count of processors if someone sets the period and quota so that their ratio is larger than the real count of processors.

Yes good point

janvorli · 2017-07-20T23:11:50Z

src/gc/unix/cgroup.cpp

+    }
+
+    static bool IsCpuSubsystem(const char *strTok){
+        return strncmp("cpu", strTok, 3) == 0 && strlen(strTok) == 3;


The same as for the memory case above.

janvorli · 2017-07-20T23:16:15Z

src/gc/unix/cgroup.cpp

+        result = ReadLongLongValueFromFile(cfs_period_filename, &period);
+        free(cfs_period_filename);
+        if (!result || period <= 0)
+             return false;


It would be nice to extract the lines 82..91 into a separate function and using it for both the period and quota instead of repeating the same block of code twice. You could make it so that the return value would be either positive for success and negative for failure and the function would get the CFS_PERIOD_FILENAME or CFS_QUOTA_FILENAME as a parameter.

Yes indeed, I introduced "long long ReadCpuCGroupValue(const char* subsystemFilename)" function.

janvorli · 2017-07-20T23:26:31Z

src/gc/unix/cgroup.cpp

+        if (cpu_count >= 1 && cpu_count < UINT32_MAX){
+            *val = cpu_count;
+        } else {
+            *val = 1;


Since we want to apply that value as a limit as I have mentioned in another comment, it seems that we should rather return the UINT32_MAX in this case.

yes, indeed, it makes more sense

janvorli · 2017-07-20T23:29:04Z

src/classlibnative/bcltype/system.cpp

+    size_t cpuLimit;
+
+    if (PAL_GetCpuLimit(&cpuLimit))
+       processorCount = cpuLimit;


The limit should only limit the count, but it should not be able to provide a larger value than the real count of processors if someone sets the period and quota so that their ratio is larger than the real count of processors.

janvorli · 2017-07-20T23:29:22Z

src/gc/unix/gcenv.unix.cpp

@@ -530,6 +532,9 @@ uint32_t GCToOSInterface::GetCurrentProcessCpuCount()
    if (count == 0 || count > 64)
        count = 64;

+    if (GetCpuLimit(&cgroupCpuCount))
+        count = cgroupCpuCount;


The limit should only limit the count, but it should not be able to provide a larger value than the real count of processors if someone sets the period and quota so that their ratio is larger than the real count of processors.

gboucher90 · 2017-07-21T11:34:47Z

Hello @janvorli, thank you for the review! I made the suggested changes.

janvorli

LGTM, thank you for making this change!

There are 2 Docker CLI command line options available that we are interested in here: - `--cpus`: this limits the amount of CPU time available to the container (ex: 1.8 means 180% CPU time, ie on 2 cores 90% for each core, on 4 cores 45% on each core, etc.) - `--cpuset-cpus`: this limits the number of processors we have access to on the CPU; it also specifies which specific processor we have access to, but that’s irrelevant here All the runtime components depending on the number of processors available are: - ThreadPool - GC - `Environment.ProcessorCount` via `SystemNative::GetProcessorCount` - `SimpleRWLock::m_spinCount` - `BaseDomain::m_iNumberOfProcessors` (it's used to determine the GC heap to affinitize to) All components but `Environment.ProcessorCount` above are aware and take advantage of the values passed to `--cpus` and `--cpuset-cpus`. **`--cpus`** dotnet#12797 has already been done. It impacts all above runtime components (allowing to optimize performance in a container/machine with limited resources). This makes sure the runtime components makes the best use of available resources. In the case of `Environment.ProcessorCount`, the behavior is such that passing `--cpus=1.5` on a machine with 8 processors will return `1` as shown in https://github.com/dotnet/coreclr/issues/22302#issuecomment-459092299. This behavior is not consistent with [Windows Job Objects](https://docs.microsoft.com/en-us/windows/desktop/api/winnt/ns-winnt-jobobject_cpu_rate_control_information) which still returns the number of processors for the container/machine even if it only gets parts of the total number of cycles. This behavior is erroneous because the container still has access to the full range of processors on the machine, and only its _processor time_ is limited. For example, in the case of a 4 processors machine, with a value of `--cpus=1.8`, there can be 4 threads running in parallel even though each thread will only get `1.8 / 8 = .45` or 45% of all cycles of each processor. The work consist in reverting the behavior of `SystemNative::GetProcessorCount` to pre dotnet#12797. **`--cpuset-cpus`** The work has been done here for all runtime components except `Environment.ProcessorCount`. The work consist in fixing `PAL_GetLogicalCpuCountFromOS` to use `sched_getaffinity`. Fixes https://github.com/dotnet/coreclr/issues/22302

This focuses on better supporting Docker CLI's parameter `--cpus`, which limits the amount of CPU time available to the container (ex: 1.8 means 180% CPU time, ie on 2 cores 90% for each core, on 4 cores 45% on each core, etc.) All the runtime components depending on the number of processors available are: - ThreadPool - GC - `Environment.ProcessorCount` via `SystemNative::GetProcessorCount` - `SimpleRWLock::m_spinCount` - `BaseDomain::m_iNumberOfProcessors` (it's used to determine the GC heap to affinitize to) All the above components take advantage of `--cpus` via `CGroup::GetCpuLimit` with dotnet#12797, allowing to optimize performance in a container/machine with limited resources. This makes sure the runtime components makes the best use of available resources. In the case of `Environment.ProcessorCount`, the behavior is such that passing `--cpus=1.5` on a machine with 8 processors will return `1` as shown in https://github.com/dotnet/coreclr/issues/22302#issuecomment-459092299. This behavior is not consistent with [Windows Job Objects](https://docs.microsoft.com/en-us/windows/desktop/api/winnt/ns-winnt-jobobject_cpu_rate_control_information) which still returns the number of processors for the container/machine even if it only gets parts of the total number of cycles. This behavior is erroneous because the container still has access to the full range of processors on the machine, and only its _processor time_ is limited. For example, in the case of a 4 processors machine, with a value of `--cpus=1.8`, there can be 4 threads running in parallel even though each thread will only get `1.8 / 8 = .45` or 45% of all cycles of each processor. The work consist in reverting the behavior of `SystemNative::GetProcessorCount` to pre dotnet#12797.

dnfclas added the cla-already-signed label Jul 13, 2017

justinvp reviewed Jul 13, 2017

View reviewed changes

gboucher90 force-pushed the criteo branch from b99adc0 to 1aa8bc1 Compare July 13, 2017 18:41

jkotas requested a review from janvorli July 16, 2017 05:10

jkotas added the area-GC label Jul 16, 2017

jkotas requested a review from swgillespie July 16, 2017 05:12

swgillespie suggested changes Jul 17, 2017

View reviewed changes

gboucher90 force-pushed the criteo branch 2 times, most recently from 5d3945b to 139a61b Compare July 17, 2017 21:57

gboucher90 force-pushed the criteo branch from 139a61b to 6e86a31 Compare July 17, 2017 22:08

swgillespie reviewed Jul 18, 2017

View reviewed changes

gboucher90 force-pushed the criteo branch 4 times, most recently from cd09507 to 3e8bf5d Compare July 18, 2017 12:27

janvorli suggested changes Jul 20, 2017

View reviewed changes

gboucher90 force-pushed the criteo branch 2 times, most recently from ea38399 to d7800f1 Compare July 21, 2017 09:37

Add CGroup CFS CPU limit support

8e23fc7

gboucher90 force-pushed the criteo branch from d7800f1 to 8e23fc7 Compare July 21, 2017 09:42

janvorli approved these changes Jul 21, 2017

View reviewed changes

janvorli merged commit df214e6 into dotnet:master Jul 21, 2017

gboucher90 deleted the criteo branch July 21, 2017 18:49

tmds pushed a commit to tmds/coreclr that referenced this pull request Aug 22, 2017

Add CGroup CFS CPU limit support (dotnet#12797)

0d53e14

karelz modified the milestone: 2.1.0 Aug 28, 2017

janvorli mentioned this pull request Sep 11, 2017

Port to 2.0.0: Complete and fix cgroup cpu and memory limiting #13895

Merged

luhenry mentioned this pull request Mar 21, 2019

Improve support for --cpus to Docker CLI #23398

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CGroup CFS CPU limit support #12797

Add CGroup CFS CPU limit support #12797

gboucher90 commented Jul 13, 2017

justinvp Jul 13, 2017

gboucher90 Jul 13, 2017

gboucher90 Jul 13, 2017

swgillespie Jul 17, 2017 •

edited

gboucher90 Jul 17, 2017

gboucher90 Jul 17, 2017

swgillespie commented Jul 17, 2017

swgillespie left a comment •

edited

gboucher90 commented Jul 17, 2017

swgillespie left a comment

swgillespie Jul 18, 2017

gboucher90 Jul 18, 2017

janvorli commented Jul 19, 2017

janvorli Jul 20, 2017

janvorli Jul 20, 2017

janvorli Jul 20, 2017

janvorli Jul 20, 2017

janvorli Jul 20, 2017

gboucher90 Jul 21, 2017

janvorli Jul 20, 2017

janvorli Jul 20, 2017

gboucher90 Jul 21, 2017

janvorli Jul 20, 2017

gboucher90 Jul 21, 2017

janvorli Jul 20, 2017

janvorli Jul 20, 2017

gboucher90 commented Jul 21, 2017

janvorli left a comment

Add CGroup CFS CPU limit support #12797

Add CGroup CFS CPU limit support #12797

Conversation

gboucher90 commented Jul 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swgillespie Jul 17, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swgillespie commented Jul 17, 2017

swgillespie left a comment • edited

Choose a reason for hiding this comment

gboucher90 commented Jul 17, 2017

swgillespie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janvorli commented Jul 19, 2017

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

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

gboucher90 commented Jul 21, 2017

janvorli left a comment

Choose a reason for hiding this comment

swgillespie Jul 17, 2017 •

edited

swgillespie left a comment •

edited