You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
BTW, I am testing with .NET7.0 RC2 which has:
[release/7.0] [mono] Determine any memory/CPU limitations from sysfs cgroup#74881 (dotnet/runtime#74881)
But it does not have:
[release/7.0][mono][sgen] Update memory load information after world is resumed #76534 (dotnet/runtime#76534).
Is this second commit also needed for Cgroup Limits to work? Note: This second commit dotnet/runtime#76534 landed in the .NET7 GA code
So I believe the intent was to allow using "75% of the container limit, but at least 20MB" - but the code actually does "75% of the container limit, but at least 200MB". This seems to match the measurements above. @janani66 can you try larger values? E.g. with 300MB container limit, you should be seeing more than 200MB.
@nealef, I'm assuming the comment is correct and this should really be 20MB instead of 200MB? I cannot find either limit in the CoreCLR implementation at first glance, so I'm not sure where these values come from ...
IMO, It's not a blocker. Most importantly, it's not a regression. It only affects mono (where cgroup was previously compleletey broken). If we have a fix, we can take it, or we can wait for a fix to flow in through a monthly servicing update.
@omajid if changing the MINMEMSZ variable fixes this, is this something that could be done for GA?
Yes. We don't have the final builds GA yet so we have some time to tweak things. Since this only affects mono, if you and @janani66 agree on a fix, We can include it. It would be nice to send a PR to dotnet/runtime for 7.0 though (for review/sanity-check and reduce our maintenance burden down the road).
// If the hard limit is specified, the user is saying even if the process is already
// running in a container, use this limit for the GC heap.
if (gc_heap::heap_hard_limit)
{
#ifdef FEATURE_EVENT_TRACE
gc_heap::hard_limit_config_p = true;
#endif //FEATURE_EVENT_TRACE
}
else
{
if (gc_heap::is_restricted_physical_mem)
{
uint64_t physical_mem_for_gc = gc_heap::total_physical_mem * (uint64_t)75 / (uint64_t)100;
gc_heap::heap_hard_limit = (size_t)max ((20 * 1024 * 1024), physical_mem_for_gc);
}
}
Will do. The irony is I had coded it as 20 *1024*1024 but was told I should use a #define and then incorrectly calculated the value instead of leaving it in that form.
-------- Original message --------
From: Ulrich Weigand ***@***.***>
Date: 10/28/22 10:27 (GMT-05:00)
To: redhat-developer/dotnet-regular-tests ***@***.***>
Cc: Neale Ferguson ***@***.***>, Mention ***@***.***>
Subject: Re: [redhat-developer/dotnet-regular-tests] Re-enable cgroup test for s390x (Issue #189)
I've now found the corresponding place in CoreCLR, and it is indeed using 20 MB, not 200 MB. See the comment here:
https://github.com/dotnet/runtime/blob/a6f4527ebf2c3faa5b9f55e8a7acb12df4571425/src/coreclr/gc/gcpriv.h#L4062-L4067
and the implementation at around line 45176 in src/coreclr/gc/gc.cpp:
// If the hard limit is specified, the user is saying even if the process is already
// running in a container, use this limit for the GC heap.
if (gc_heap::heap_hard_limit)
{
#ifdef FEATURE_EVENT_TRACE
gc_heap::hard_limit_config_p = true;
#endif //FEATURE_EVENT_TRACE
}
else
{
if (gc_heap::is_restricted_physical_mem)
{
uint64_t physical_mem_for_gc = gc_heap::total_physical_mem * (uint64_t)75 / (uint64_t)100;
gc_heap::heap_hard_limit = (size_t)max ((20 * 1024 * 1024), physical_mem_for_gc);
}
}
The attached patch implements the obvious fix to change to 20 MB in Mono as well:
mono-cgroup-minmemsz-fix.txt<https://github.com/redhat-developer/dotnet-regular-tests/files/9889185/mono-cgroup-minmemsz-fix.txt>
@nealef<https://github.com/nealef> given that this is originally your implementation, could you move that forward as PRs in all affected branches as well? Thanks!
—
Reply to this email directly, view it on GitHub<#189 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AACLN6WGKRAN2C5EBPLLJCLWFPPDRANCNFSM5OA72ETA>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
Activity
omajid commentedon Oct 27, 2022
This was fixed in mono with dotnet/runtime#74237 and should be a part of 7.0 now. This test is now expected to pass on 7.0 for mono runtimes too.
omajid commentedon Oct 27, 2022
I have run this test on ppc64le and s390x now and both fail:
s390x
ppc64le
@tmds @uweigand @janani66 Does this seem expected? Any idea what's going on? Is it a test bug or a bug in mono?
tmds commentedon Oct 27, 2022
means
GC.GetGCMemoryInfo().TotalAvailableMemoryBytesreturns exactly 200MB, which is more than the 100MB limit we've set.If you vary the
MemoryLimit, how doesTotalAvailableMemoryBytesbehave?janani66 commentedon Oct 27, 2022
@tmds Well changing the MemoryLimit doesn't seem to change anything on ppc64le running .NET7 RC2 code
[cgroup-limit]#systemd-run -q --scope -p CPUQuota=100% -p MemoryLimit=250M bin/Debug/net7.0/cgroup-limit
Limits:
1
209715200
[cgroup-limit]#systemd-run -q --scope -p CPUQuota=100% -p MemoryLimit=200M bin/Debug/net7.0/cgroup-limit
Limits:
1
209715200
[cgroup-limit]#systemd-run -q --scope -p CPUQuota=100% -p MemoryLimit=100M bin/Debug/net7.0/cgroup-limit
Limits:
1
209715200
janani66 commentedon Oct 27, 2022
BTW, I am testing with .NET7.0 RC2 which has:
[release/7.0] [mono] Determine any memory/CPU limitations from sysfs cgroup#74881 (dotnet/runtime#74881)
But it does not have:
[release/7.0][mono][sgen] Update memory load information after world is resumed #76534 (dotnet/runtime#76534).
Is this second commit also needed for Cgroup Limits to work? Note: This second commit dotnet/runtime#76534 landed in the .NET7 GA code
tmds commentedon Oct 28, 2022
@omajid this is not a blocker for 7.0, right?
uweigand commentedon Oct 28, 2022
Hmm. Looking at the sources, I see:
However, the variable
MINMEMSZis actually defined as:So I believe the intent was to allow using "75% of the container limit, but at least 20MB" - but the code actually does "75% of the container limit, but at least 200MB". This seems to match the measurements above. @janani66 can you try larger values? E.g. with 300MB container limit, you should be seeing more than 200MB.
@nealef, I'm assuming the comment is correct and this should really be 20MB instead of 200MB? I cannot find either limit in the CoreCLR implementation at first glance, so I'm not sure where these values come from ...
uweigand commentedon Oct 28, 2022
@omajid if changing the
MINMEMSZvariable fixes this, is this something that could be done for GA?janani66 commentedon Oct 28, 2022
@uweigand -- you nailed it
[root@ltcden12-lp6 cgroup-limit]# systemd-run -q --scope -p CPUQuota=100% -p MemoryLimit=300M bin/Debug/net7.0/cgroup-limit
Limits:
1
235929600
[root@ltcden12-lp6 cgroup-limit]# systemd-run -q --scope -p CPUQuota=100% -p MemoryLimit=350M bin/Debug/net7.0/cgroup-limit
Limits:
1
275251200
omajid commentedon Oct 28, 2022
IMO, It's not a blocker. Most importantly, it's not a regression. It only affects mono (where cgroup was previously compleletey broken). If we have a fix, we can take it, or we can wait for a fix to flow in through a monthly servicing update.
Yes. We don't have the final builds GA yet so we have some time to tweak things. Since this only affects mono, if you and @janani66 agree on a fix, We can include it. It would be nice to send a PR to dotnet/runtime for 7.0 though (for review/sanity-check and reduce our maintenance burden down the road).
uweigand commentedon Oct 28, 2022
I've now found the corresponding place in CoreCLR, and it is indeed using 20 MB, not 200 MB. See the comment here:
https://github.com/dotnet/runtime/blob/a6f4527ebf2c3faa5b9f55e8a7acb12df4571425/src/coreclr/gc/gcpriv.h#L4062-L4067
and the implementation at around line 45176 in src/coreclr/gc/gc.cpp:
The attached patch implements the obvious fix to change to 20 MB in Mono as well:
mono-cgroup-minmemsz-fix.txt
@nealef given that this is originally your implementation, could you move that forward as PRs in all affected branches as well? Thanks!
nealef commentedon Oct 28, 2022
nealef commentedon Oct 31, 2022
PRs generated -
janani66 commentedon Nov 1, 2022
I have been able to verify the fix in the runtime repo
[root@kop5 cgroup-limit]# systemd-run -q --scope -p CPUQuota=100% -p MemoryLimit=200M bin/Debug/net7.0/cgroup-limit
Limits:
1
157286400
[root@kop5 cgroup-limit]# systemd-run -q --scope -p CPUQuota=100% -p MemoryLimit=100M bin/Debug/net7.0/cgroup-limit
Limits:
1
78643200
[root@kop5 cgroup-limit]# systemd-run -q --scope -p CPUQuota=100% -p MemoryLimit=250M bin/Debug/net7.0/cgroup-limit
Limits:
1
196608000
[root@kop5 cgroup-limit]# systemd-run -q --scope -p CPUQuota=100% -p MemoryLimit=300M bin/Debug/net7.0/cgroup-limit
Limits:
1
235929600
omajid commentedon Dec 20, 2022
Fixed with #225 and #226