-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Sync cgroup v2 in libraries with coreclr #34665
Conversation
Tagging @eiriktsarpalis as an area owner |
src/libraries/Common/src/Interop/Linux/cgroups/Interop.cgroups.cs
Outdated
Show resolved
Hide resolved
src/libraries/Common/src/Interop/Linux/cgroups/Interop.cgroups.cs
Outdated
Show resolved
Hide resolved
src/libraries/Common/src/Interop/Linux/cgroups/Interop.cgroups.cs
Outdated
Show resolved
Hide resolved
src/libraries/Common/src/Interop/Linux/cgroups/Interop.cgroups.cs
Outdated
Show resolved
Hide resolved
cc @tmds |
6c77805
to
a00c76f
Compare
@stephentoub CI is green now, can you please review this again? |
@omajid What are you doing to test this? I'll give it a try also. |
Not much: relying on unit tests to make sure the file parsing and the path computations are still correct. The output from |
I also want to try the corresponding runtime change. Any suggestions for that? |
Heh, I stole your idea for a runtime test :) My runtime test is the one at #34287. Copying it here for easy of use: #include "cgroup.cpp"
int main(int, char *[])
{
InitializeCGroup();
printf("initialized cgroup\n");
size_t physical_memory_limit = GetRestrictedPhysicalMemoryLimit();
printf("GetResitrictedPhysicalMemoryLimit: %lu\n", physical_memory_limit);
size_t used_memory = 0;
bool okay_memory_used = GetPhysicalMemoryUsed(&used_memory);
printf("GetPhysicalMemoryUsed: %d %lu\n", okay_memory_used, used_memory);
uint32_t cpu_limit = 0;
bool okay_cpu_limit = GetCpuLimit(&cpu_limit);
printf("GetCpuLimit: %d %u\n", okay_cpu_limit, cpu_limit);
CleanupCGroup();
printf("cleaned up cgroups\n");
return 0;
} You need to change the path to point to the non-pal version of FROM fedora:31
RUN dnf install gdb -y
ADD * /
#CMD gdb /cg
CMD /cg Then the container should work on all Fedora versions and report the correct values:
On Fedora 31, cgroupv2 is used. You can switch to cgroupv1 using:
Then rebooting. To test what your system is using, run:
If your system has cgroupv1 by default, the output will include Alternatively, |
It has a permissive license 😄 Thanks for the steps! Generally speaking, the cgroup code in coreclr/corefx now supports a number of configurations, and I don't know how to cover all of them. I'll try out a couple of things. |
src/libraries/Common/src/Interop/Linux/cgroups/Interop.cgroups.cs
Outdated
Show resolved
Hide resolved
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).
a00c76f
to
e016d63
Compare
This commit brings in two changes from coreclr to libraries:
Fix named cgroup handling in docker #980: "Fix named cgroup handling in docker"
This fixes getting cgroup information for named cgroups inside containers.
Add cgroup v2 support to coreclr #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).
cc'ing some potential reviewers: @bartonjs @janvorli @krwq @lpereira @stephentoub @wtgodbe