memory limit in containers on linux #10064

rahku · 2017-03-09T18:29:58Z

This adds implementation in pal to get memory limit from cgroups. Memory limit values are written in file <path_to_memory_cgroup>/memory.limit_in_bytes and <path_to_memory_cgroup>/memory.memsw.limit_int_bytes. path_to_memory_cgroup is <path_to_memory_hierarchy>/<cgroup_name>. cgroup_name can be found in file /proc/self/cgroup. <path_to_memory_hierarchy>is normally set to /sys/fs/cgroup/memory. However this is not the standard. It can be changed to any mount point. Standard way of getting that value is from /proc/self/mountinfo.

Note: CGroup implementation is not available on OSX. Docker does not run natively on OSX. It runs under a light-weigth hypervisor which runs a linux distro and you add container on top of it. So the current implementation so work just fine in this scenario.

rahku · 2017-03-09T18:30:56Z

I have tested this on ubuntu. I am still doing some additional testing.

rahku · 2017-03-09T18:31:15Z

PTAL @janvorli @gkhanna79 @Maoni0

janvorli · 2017-03-09T19:07:10Z

src/gc/unix/gcenv.unix.cpp

@@ -78,6 +78,22 @@ static uint8_t g_helperPage[OS_PAGE_SIZE] __attribute__((aligned(OS_PAGE_SIZE)))
 // Mutex to make the FlushProcessWriteBuffersMutex thread safe
 static pthread_mutex_t g_flushProcessWriteBuffersMutex;

+static size_t g_RestrictedPhysicalMemoryLimit = (size_t)MAX_PTR;
+
+static size_t GetRestrictedPhysicalMemoryLimit()


This file is for pure Unix only and so you cannot use any PAL functions here. It is used when the GC is compiled as a standalone library.

Also, no contracts and no windowsizms should be here.

that would imply code duplication..is that ok

janvorli · 2017-03-09T19:12:01Z

Yes, code duplication is ok here.

janvorli · 2017-03-09T19:10:25Z

src/pal/src/misc/cgroup.cpp

+
+class CGroup
+{
+    char _memory_cgroup_path[MAX_PATH];


For native code, the convention is to use m_ instead of _ as a member prefix.

Can you please use StackString or its specialization PathStackString instead of MAX_PATH sized arrays everywhere where the size is not known? We have introduced StackString to deal with variable sized paths on Unix.
The code will also become cleaner since you would not need to care about passing the MAX_PATH limit everywhere.

Hmm, I have just realized that the StackString is not available out of PAL for the Unix only implementation, so please ignore this.

janvorli · 2017-03-09T19:13:36Z

src/pal/src/misc/cgroup.cpp

+#include "pal/palinternal.h"
+#include <sys/resource.h>
+
+#define MAX_SIZE_T (~(SIZE_T)0)


Please use SIZE_T_MAX which is available in PAL

janvorli · 2017-03-09T19:15:18Z

src/pal/src/misc/cgroup.cpp

+        strcpy(_memory_cgroup_path, memory_cgroup_path);
+    }
+
+    const char* path()


For functions, we use pascal case naming convention in PAL.

janvorli · 2017-03-09T19:17:20Z

src/pal/src/misc/cgroup.cpp

+        return _memory_cgroup_path;
+    }
+
+    bool physicalMemoryLimit(size_t &val)


Can you please name this methods GetPhysicalMemoryLimit and GetSwapMemoryLimit? That would make the purpose clearer for readers of the code. The swapMemoryLimit is especially confusing.

Also, the convention in PAL is not to use references for output parameters, but rather use pointers.

janvorli · 2017-03-09T20:03:56Z

src/pal/src/misc/cgroup.cpp

+                                     "- %s %*s %s",
+                                     filesystemType, (unsigned)_countof(filesystemType),
+                                     options, (unsigned)_countof(options));
+            if (sscanfRet != 2)


Could you please unify the spacing style between the if / while and the following (? Sometimes you have space there and sometimes not. I prefer the style with space, but I'll leave it up to you.

janvorli · 2017-03-09T20:39:33Z

src/gc/unix/cgroup.cpp

+        char* separatorChar = strchr(line, '-');
+
+        // See man page of proc to get format for /proc/self/mountinfo file
+        int sscanfRet = sscanf_s(separatorChar, 


the sscanf_s and other _s functions are not available for plain Unix. You'll need to use the plain ones or maybe better add defines that define sscanf_s as sscanf etc so that the rest of the code is the same as for the PAL version.

janvorli · 2017-03-09T20:51:05Z

src/pal/tests/palsuite/miscellaneous/CGroup/test1/test.cpp

+int __cdecl main(int argc,char *argv[]) 
+{
+
+    char * AnExampleString = "this is the string";


These three variables doesn't seem to be used. Can you remove them please?

janvorli · 2017-03-09T20:52:25Z

src/pal/tests/palsuite/miscellaneous/CGroup/test1/test.cpp

+    FILE* file = fopen("/sys/fs/cgroup/memory/mygroup/memory.limit_in_bytes", "r");
+    if(file != NULL)
+    {
+        if(mem_limit != 4194304)


I am surprised the test can run at all with just 4MB limit. But I assume you have verified that.

janvorli · 2017-03-09T20:58:10Z

src/gc/unix/cgroup.h

+    Read memory limits for the current process
+--*/
+
+#define MAX_SIZE_T (~(SIZE_T)0)


It seems that these defines can be in the cgroup.cpp since they are not part of the public interface of the CGroup.

janvorli · 2017-03-09T21:02:25Z

src/vm/gcenv.os.cpp

    size_t restricted_limit = GetRestrictedPhysicalMemoryLimit();
-    if (restricted_limit != 0)
+    if (restricted_limit != 0 && restricted_limit != (size_t)MAX_PTR)


Is the added check needed? It looks like the GetRestrictedPhysicalMemoryLimit should return zero if there is no limit to be on par with the Windows version.

janvorli · 2017-03-09T21:02:42Z

src/vm/gcenv.os.cpp

    uint64_t restricted_limit = GetRestrictedPhysicalMemoryLimit();
-    if (restricted_limit != 0)
+    if (restricted_limit != 0 && restricted_limit != (size_t)MAX_PTR)


The same question here

gkhanna79 · 2017-03-09T21:23:21Z

src/gc/unix/cgroup.cpp

+    switch(*endptr)
+    {
+        case 'g':
+        case 'G': multiplier = 1024;


Are there no breaks expected here? Certain compilers can complain about it - it will be good to logically segregate them with a break.

no breaks are not needed it needs to fall though

The code as it is would not work. You are multiplying bool (result) by 1024. I guess the intention was to use multiplier = multiplier * 1024 in there.

yes. thanks for catching it. I did test it before and then changed variable names but missed here.

janvorli · 2017-03-09T21:26:30Z

@rahku have you tried to build it on Linux with the buildstandalonegc option? That compiles all the pure unix parts that are not built otherwise.

rahku · 2017-03-09T21:27:59Z

No i did not know that ...will do

swgillespie · 2017-03-09T21:54:24Z

There's also a CI job that runs buildstandalonegc: https://ci.dot.net/job/dotnet_coreclr/job/master/job/x64_checked_ubuntu_standalone_gc/ . I think it can be triggered on PRs with test Ubuntu Checked standalone_gc.

jkotas · 2017-03-10T01:00:11Z

src/gc/unix/cgroup.cpp

+                                 options, (unsigned)_countof(options));
+        if (sscanfRet != 2)
+        {
+            _ASSERTE(!"Failed to parse mount info file contents with sscanf_s.");


We prefer vanilla C/C++ for the standalone GC PAL: _ASSERTE -> assert

yeah i m in process of making these changes.

rahku · 2017-03-10T01:18:08Z

@dotnet-bot test Ubuntu Checked standalone_gc

rahku · 2017-03-10T01:19:10Z

@dotnet-bot test Ubuntu x64 Checked standalone_gc

rahku · 2017-03-10T01:25:48Z

PTAL i have incorprorated all review comments

Maoni0 · 2017-03-10T01:35:49Z

src/vm/gcenv.os.cpp

    uint64_t restricted_limit = GetRestrictedPhysicalMemoryLimit();
    if (restricted_limit != 0)
    {
+        size_t workingSetSize;
+        BOOL status = false;


false [](start = 22, length = 5)

nit... FALSE instead of false.

jkotas · 2017-03-10T02:27:14Z

src/gc/unix/cgroup.h

+
+class CGroup
+{
+    char m_memory_cgroup_path[MAX_PATH];


Hungarian notation is Windowsism... . Change it to just _memory_cgroup_path ?

jkotas · 2017-03-10T02:29:14Z

src/gc/unix/cgroup.h

+    bool ReadMemoryValueFromFile(const char* filename, size_t* val);
+};
+
+size_t GetRestrictedPhysicalMemoryLimit();


It would look better to have these methods as static public methods on CGroup, and have all instance methods in CGroup to be private; or move class CGroup to the .cpp file.

Since it is in the .cpp file in the CoreCLR PAL, I think it should be in the .cpp file here as well.

janvorli · 2017-03-10T13:11:42Z

src/gc/unix/cgroup.cpp

+
+class CGroup
+{
+    char _memory_cgroup_path[MAX_PATH];


@jkotas I have asked @rahku to use m_ convention instead, but I could see you have asked him to change it back However, we use m_ in all coreclr PAL and also in CoreRT native code, so I'd really prefer staying in line with that.

I think @jkotas was asking to change in this file which is for standalone gc & linux specific and so should not have any windowism?

I do not have a strong opinion. I will leave it up to you what you like best.

janvorli · 2017-03-10T13:15:54Z

src/gc/unix/cgroup.cpp

+        size_t num = 0, l, multiplier;
+        FILE* file = NULL;
+
+        if (val == NULL)


A nit - you have this check only here, but not in the other copy of the function.

jkotas · 2017-03-10T22:10:28Z

src/gc/unix/gcenv.unix.cpp

+    if (g_RestrictedPhysicalMemoryLimit != 0)
+        return g_RestrictedPhysicalMemoryLimit;
+
+    size_t memory_limit = GetRestrictedPhysicalMemoryLimit();


Isn't there an infinite recursion (GetRestrictedPhysicalMemoryLimit calling GetRestrictedPhysicalMemoryLimit)? I think this can be moved to GCToOSInterface::GetPhysicalMemoryLimit.

jkotas · 2017-03-10T22:13:34Z

src/gc/unix/cgroup.cpp

+
+size_t GetRestrictedPhysicalMemoryLimit()
+{
+   CGroup cgroup;


Nit: This does not seem to be using 4 space indentation

jkotas · 2017-03-10T22:15:28Z

src/gc/unix/cgroup.cpp

+            // See man page of proc to get format for /proc/self/cgroup file
+            int sscanfRet = sscanf(line, 
+                                   "%*[^:]:%[^:]:%s",
+                                   subsystem_list,


Is this hardened against buffer overruns? What guarantees that the result will fit into subsystem_list?

There are no secure versions of these in linux. Are you suggesting to not to use these non-secure crt functions.

If you are using these non-secure crt functions, it is up to you to guarantee no buffer overruns.

rahku · 2017-03-11T01:09:48Z

unix specific cgroup.cpp is now hardened against buffer overruns. @jkotas does that look fine now?

rahku · 2017-03-15T07:10:03Z

I have tested this on fedora & ubuntu and also done some end-to-end testing and results look good. One of the difference that I found wrt to windows is that on Linux when we go out of memory the process is killed by OS instead of allocation failure. This is with the default setting of the machine...there might be settings to change it.

rahku · 2017-03-15T17:36:37Z

@janvorli I also now do not look at file memory.memsw.limit_in_bytes. This file gives the swap space limit. Swap space limits the virtual address of the process and does not impact real memory that can be used by the process (in our case I am assuming only a single process to be run in a cgroup). But for gc we need real memory pressure and not virtual address space pressure.

rahku · 2017-03-15T22:14:54Z

@dotnet-bot test Windows_NT x64 Debug Build and Test

rahku · 2017-03-16T00:17:51Z

@dotnet-bot test OSX x64 Checked Build and Test

janvorli · 2017-03-20T16:01:28Z

src/gc/unix/cgroup.cpp

+
+    bool GetPhysicalMemoryLimit(size_t *val)
+    {
+        char mem_limit_filename[MAX_PATH];


Some time ago, we have made a sweeping change over the CoreCLR native code including PAL to get rid of fixed size buffers for file paths unless the path is guaranteed to never exceed that limit. So introducing such a fixed size buffer here makes me nervous. I would prefer using a dynamically allocated buffer at this place. You can use the length of the line from the mount info file that contains the path as an memory size estimate - we would just waste a little memory that's not needed for the path. The FindMemoryHierarchyMount can then return nullptr or the allocated buffer.
I think that similar thing can be done at other places where you use MAX_PATH.

janvorli · 2017-03-20T16:07:51Z

src/pal/src/misc/cgroup.cpp

+    if (file != NULL && getline(&line, &linelen, file) != -1)
+    {
+        int x = sscanf_s(line, "%*s %llu %*s %*s %*s %*s %*s", val);
+        if(x==1)


A nit - could you please add spaces around the == to unify it with the rest of the code?

janvorli · 2017-03-22T17:53:11Z

src/gc/unix/cgroup.cpp

+    done:
+        free(filesystemType);
+        free(options);
+        if (line)


A nit - this "if" is not needed, free can accept nullptr just fine.

janvorli · 2017-03-22T17:54:00Z

src/gc/unix/cgroup.cpp

+            free(cgroup_path);
+            cgroup_path = nullptr;
+        }
+        if (line)


A nit - this "if" is not needed, free can accept nullptr just fine.

janvorli · 2017-03-22T17:54:35Z

src/gc/unix/cgroup.cpp

+    done:
+        if (file)
+            fclose(file);
+        if (line)


A nit - this "if" is not needed, free can accept nullptr just fine.

janvorli · 2017-03-22T17:55:49Z

src/gc/unix/cgroup.cpp

+
+    // Ensure that limit is not greater than real memory size
+    size_t pages = (size_t) sysconf(_SC_PHYS_PAGES);
+    if (pages != -1) 


size_t is not a signed type, it seems you can use long here.

janvorli · 2017-03-22T17:57:33Z

src/gc/unix/cgroup.cpp

+{
+    bool result = false;
+    size_t linelen;
+    char* line = NULL;


Could you please unify on using nullptr everywhere instead of NULL at some places and nullptr elsewhere?

janvorli · 2017-03-22T17:58:09Z

src/gc/unix/cgroup.cpp

+
+    if (file)
+        fclose(file);
+    if (line)


A nit - this "if" is not needed, free can accept nullptr just fine.

The same case in the PAL version of this code for all the occurences.

janvorli · 2017-03-22T18:01:28Z

src/gc/unix/gcenv.unix.cpp

        {
-            available = sysconf(_SC_PHYS_PAGES) * sysconf(_SC_PAGE_SIZE);
-            uint64_t used = total - available;
+            available = total > used ? total-used : 0; 
            load = (uint32_t)((used * 100) / total);


This could overflow on 32 bit hardware, you'll need to keep the used type to be uint64_t.

janvorli · 2017-03-22T18:05:43Z

src/pal/src/misc/cgroup.cpp

+
+    // Ensure that limit is not greater than real memory size
+    size_t pages = (size_t) sysconf(_SC_PHYS_PAGES);
+    if (pages != -1) 


size_t is an unsigned type

janvorli

LGTM, thank you!

rahku · 2017-03-22T21:05:00Z

@dotnet-bot test Tizen armel Cross Debug Build
@dotnet-bot test Tizen armel Cross Release Build
@dotnet-bot test Ubuntu arm Cross Release Build
@dotnet-bot test Ubuntu16.04 arm Cross Debug Build

rahku · 2017-03-22T22:03:57Z

@hseok-oh you recently changed the arm test runs. Everything was passing earlier for me before your changes. I do not have a device/emulator to debug this. Could you take a look at failures or point to instructions on how to use arm emulator.

janvorli · 2017-03-22T22:17:04Z

@rahku - The problem is some buffer overrun. I can see in the log the following which indicates that:
*** stack smashing detected ***

hseok-oh · 2017-03-23T01:04:41Z

@rahku In fact, our CI ARM emulator didn't test run since testrun script changed. That could check build success only. So we removed old ARM emulator build test and reconfigured build and test based on docker.

dnfclas added the cla-already-signed label Mar 9, 2017

janvorli reviewed Mar 9, 2017

View reviewed changes

rahku force-pushed the cgroup branch from f4d4cf9 to c2b59fe Compare March 9, 2017 21:16

gkhanna79 reviewed Mar 9, 2017

View reviewed changes

jkotas reviewed Mar 10, 2017

View reviewed changes

rahku force-pushed the cgroup branch from e766e7f to 5fc16f8 Compare March 10, 2017 01:16

Maoni0 reviewed Mar 10, 2017

View reviewed changes

Maoni0 approved these changes Mar 10, 2017

View reviewed changes

jkotas reviewed Mar 10, 2017

View reviewed changes

rahku force-pushed the cgroup branch 2 times, most recently from 49ff670 to 11d92a7 Compare March 10, 2017 07:36

janvorli reviewed Mar 10, 2017

View reviewed changes

jkotas reviewed Mar 10, 2017

View reviewed changes

rahku force-pushed the cgroup branch from 11d92a7 to 56e062c Compare March 11, 2017 01:06

jkotas approved these changes Mar 11, 2017

View reviewed changes

rahku force-pushed the cgroup branch from 56e062c to 4ace378 Compare March 15, 2017 07:05

Maoni0 mentioned this pull request Mar 16, 2017

Is it possible to run dotnet-docker based containers with memory limits? dotnet/dotnet-docker#220

Closed

janvorli reviewed Mar 20, 2017

View reviewed changes

rahku force-pushed the cgroup branch from 4ace378 to b24a9a1 Compare March 22, 2017 07:57

janvorli suggested changes Mar 22, 2017

View reviewed changes

rahku force-pushed the cgroup branch from 8a5ca29 to 8599120 Compare March 22, 2017 19:11

janvorli approved these changes Mar 22, 2017

View reviewed changes

memory limit in containers on linux

b2b4ea2

rahku force-pushed the cgroup branch from 8599120 to b2b4ea2 Compare March 23, 2017 06:37

rahku merged commit b511095 into dotnet:master Mar 23, 2017

rahku deleted the cgroup branch March 29, 2017 19:02

davidfowl mentioned this pull request Aug 10, 2017

Memory Leak in 1.1.0 aspnet/KestrelHttpServer#1260

Closed

karelz modified the milestone: 2.0.0 Aug 28, 2017

memory limit in containers on linux #10064

memory limit in containers on linux #10064

Conversation

rahku commented Mar 9, 2017

rahku commented Mar 9, 2017

rahku commented Mar 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janvorli commented Mar 9, 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

Choose a reason for hiding this comment

gkhanna79 Mar 9, 2017 • 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

janvorli commented Mar 9, 2017

rahku commented Mar 9, 2017

swgillespie commented Mar 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rahku commented Mar 10, 2017

rahku commented Mar 10, 2017

rahku commented Mar 10, 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

jkotas Mar 10, 2017 • 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

Choose a reason for hiding this comment

rahku commented Mar 11, 2017

rahku commented Mar 15, 2017

rahku commented Mar 15, 2017

rahku commented Mar 15, 2017

rahku commented Mar 16, 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

janvorli left a comment

Choose a reason for hiding this comment

rahku commented Mar 22, 2017

rahku commented Mar 22, 2017

janvorli commented Mar 22, 2017

hseok-oh commented Mar 23, 2017 • edited Loading

gkhanna79 Mar 9, 2017 •

edited

Loading

jkotas Mar 10, 2017 •

edited

Loading

hseok-oh commented Mar 23, 2017 •

edited

Loading