Skip to content
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

Add cgroup v2 support to coreclr #34334

Merged
merged 1 commit into from
Apr 7, 2020
Merged

Add cgroup v2 support to coreclr #34334

merged 1 commit into from
Apr 7, 2020

Conversation

omajid
Copy link
Member

@omajid omajid commented Mar 31, 2020

Upstream cgroup v2 documentation is available at: https://www.kernel.org/doc/Documentation/cgroup-v2.txt

Some notable differences between cgroup v1 and v2, from a coreclr point of view, include:

  • cgroup v2 has a single hierarchy, so we just look for a single "cgroup2" entry in /proc/self/mountinfo (without looking for a subsystem match).

  • Since cgroup v2 has a single hierarchy, /proc/self/cgroup generally has a single line "0::/path". There's no need to match subsystems or hierarchy ids here.

  • "memory.limit_in_bytes" is now "memory.max". It can contain the literal "max" to indicate no limit.

  • "memory.usage_in_bytes" is now "memory.current"

  • "cpu.cfs_quota_us" and "cpu.cfs_period_us" have been combined into a single "cpu.max" file with the format "$MAX $PERIOD". The max value can be a literal "max" to indicate a limit is not active.

It is possible to have both cgroup v1 and v2 to both be enabled on a host (but not inside a container). In that case, this change will pick one, which may not be the correct one. We should be able to find the right one by checking the system configuration as described here.

src/coreclr/src/gc/unix/cgroup.cpp Outdated Show resolved Hide resolved
src/coreclr/src/gc/unix/cgroup.cpp Outdated Show resolved Hide resolved
src/coreclr/src/gc/unix/cgroup.cpp Outdated Show resolved Hide resolved
src/coreclr/src/gc/unix/cgroup.cpp Outdated Show resolved Hide resolved
src/coreclr/src/gc/unix/cgroup.cpp Outdated Show resolved Hide resolved
src/coreclr/src/gc/unix/cgroup.cpp Outdated Show resolved Hide resolved
src/coreclr/src/pal/src/misc/cgroup.cpp Outdated Show resolved Hide resolved
src/coreclr/src/pal/src/misc/cgroup.cpp Outdated Show resolved Hide resolved
src/coreclr/src/gc/unix/cgroup.cpp Outdated Show resolved Hide resolved
src/coreclr/src/gc/unix/cgroup.cpp Outdated Show resolved Hide resolved
src/coreclr/src/gc/unix/cgroup.cpp Outdated Show resolved Hide resolved
src/coreclr/src/gc/unix/cgroup.cpp Outdated Show resolved Hide resolved
@omajid omajid changed the title Add cgroup v2 support to coreclr WIP: Add cgroup v2 support to coreclr Apr 1, 2020
@omajid omajid force-pushed the cgroupv2 branch 7 times, most recently from 885ea76 to 16b3a75 Compare April 2, 2020 17:33
@omajid omajid changed the title WIP: Add cgroup v2 support to coreclr Add cgroup v2 support to coreclr Apr 2, 2020
@omajid
Copy link
Member Author

omajid commented Apr 2, 2020

I don't understand the macOS failures. Can someone help me understand the build issue?

@janvorli
Copy link
Member

janvorli commented Apr 2, 2020

Can someone help me understand the build issue?

Crossgen of System.Private.CoreLib.dll is crashing with SIGSEGV. Crossgen uses PAL too, so maybe it crashes in the newly added code due to the fact that there are no cgroups on OSX and somewhere we don't handle properly the case when the cgroup initialization failed?

@omajid
Copy link
Member Author

omajid commented Apr 2, 2020

Crossgen of System.Private.CoreLib.dll is crashing with SIGSEGV. Crossgen uses PAL too, so maybe it crashes in the newly added code due to the fact that there are no cgroups on OSX and somewhere we don't handle properly the case when the cgroup initialization failed?

Aaah. I think I see it. I was hoping the assertion message would be printed at that point if such a condition triggers :(

result = ReadMemoryValueFromFile(mem_usage_filename, &temp);
if (result)
{
if (temp > std::numeric_limits<size_t>::max())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is always going to be false on a 64-bit system. Do we force size_t to be 64-bit on 32-bit systems?

Copy link
Member Author

@omajid omajid Apr 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure. This code was added as part of dotnet/coreclr#25724. I only moved it around.

Edit: As I see it now, ReadMemoryValueFromFile returns a 64-bit integer. On 32-bit platforms, it needs to be clamped correctly to fit into a size_t.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, that was the intent of this check.

@omajid omajid closed this Apr 3, 2020
@omajid omajid reopened this Apr 3, 2020
Copy link
Member

@janvorli janvorli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a couple of nits, but it looks good otherwise.

src/coreclr/src/gc/unix/cgroup.cpp Outdated Show resolved Hide resolved
src/coreclr/src/gc/unix/cgroup.cpp Outdated Show resolved Hide resolved
Upstream cgroup v2 documentation is available at:
https://www.kernel.org/doc/Documentation/cgroup-v2.txt

Some notable differences between cgroup v1 and v2, from a coreclr point
of view, include:

- cgroup v2 has a single hierarchy, so we just look for a single "cgroup2"
  entry in /proc/self/mountinfo (without looking for a subsystem match).

- Since cgroup v2 has a single hierarchy, /proc/self/cgroup generally
  has a single line "0::/path". There's no need to match subsystems or
  hierarchy ids here.

- "memory.limit_in_bytes" is now "memory.max". It can contain the
  literal "max" to indicate no limit.

- "memory.usage_in_bytes" is now "memory.current"

- "cpu.cfs_quota_us" and "cpu.cfs_period_us" have been combined into a
  single "cpu.max" file with the format "$MAX $PERIOD". The max value
  can be a literal "max" to indicate a limit is not active.

It is possible to have both cgroup v1 and v2 enabled on a host (but not
inside a container, AFAIK). In that case, this change will pick one
based on /sys/fs/cgroup.
Copy link
Member

@janvorli janvorli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you!

@janvorli janvorli merged commit caab744 into dotnet:master Apr 7, 2020
@omajid
Copy link
Member Author

omajid commented Apr 7, 2020

Thanks for the review, your great suggestions and for merging this!

@janvorli
Copy link
Member

janvorli commented Apr 7, 2020

@omajid You're welcome, thank you for adding the support!

omajid added a commit to omajid/dotnet-runtime that referenced this pull request Apr 13, 2020
This commit brings in two changes from coreclr to libraries:

1. dotnet#980

   "Fix named cgroup handling in docker"

   This fixes getting cgroup information for named cgroups inside containers.

2. dotnet#34334

   "Add cgroup v2 support to coreclr"

   This is essentially the same change pushed to corefx (now libraries)
   to add cgroupv2 support, but this newer coreclr change has one major
   difference: it determines whether the system is using cgroup v1 or
   cgroup v2 once, and then explicitly uses that (only). This avoids
   issues on systems where both cgroup v1 and v2 are enabled, (but only
   one is being used by default).
stephentoub pushed a commit that referenced this pull request Apr 14, 2020
This commit brings in two changes from coreclr to libraries:

1. #980

   "Fix named cgroup handling in docker"

   This fixes getting cgroup information for named cgroups inside containers.

2. #34334

   "Add cgroup v2 support to coreclr"

   This is essentially the same change pushed to corefx (now libraries)
   to add cgroupv2 support, but this newer coreclr change has one major
   difference: it determines whether the system is using cgroup v1 or
   cgroup v2 once, and then explicitly uses that (only). This avoids
   issues on systems where both cgroup v1 and v2 are enabled, (but only
   one is being used by default).
@richlander
Copy link
Member

Is this blog post a fair assessment of the cgroup v2 landscape? If not, can you suggest something that is?

https://medium.com/nttlabs/cgroup-v2-596d035be4d7

@omajid
Copy link
Member Author

omajid commented May 7, 2020

Is this blog post a fair assessment of the cgroup v2 landscape?

Yes, I think it is. I have been using it as the source of truth myself 😄 I haven't found anything in it that's wrong.

For a more technical details, https://systemd.io/CGROUP_DELEGATION/ is also a useful resource as is https://www.kernel.org/doc/Documentation/cgroup-v2.txt.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants