Hi Matthias,
On Wed, 2020-07-15 at 15:22 +0000, Baesken, Matthias wrote:
> Hello Severin ,
>
> > the new cgroups implementation
> > supporting v1 and v2 Metrics.getMemoryAndSwapLimit() will never return 0
>
> Wouldn’t it be possible that the coding of getMemoryAndSwapLimit returns
> a negative value (might not happen on a "healthy" system but you never know)
> :
In short, no, unless the value is actually unlimited. With the caveat
that we cannot distinguish between "kernel not supporting swap" and
"unlimited" swap with a potential cgroup memory.memsw.limit_in_bytes
file missing.
The premise is that negative values are not possible in cgroup
interface files. So the only valid reason to return negative (-1, to be
precise) is if the interface files aren't there or the values in those
files are beyond a threshold indicating "unlimited".
Let's look at this in detail:
> jdk/src/java.base/linux/classes/jdk/internal/platform/cgroupv1/CgroupV1Subsystem.java
>
> 444 public long getMemoryAndSwapLimit() {
> 445 long retval = getLongValue(memory, "memory.memsw.limit_in_bytes");
getLongValue() will not return a negative value unless an interface
file is missing, a string in the cgroup files is empty, or a very large
number is observed (which maps to unlimited, a.k.a non-container values
win).
> 446 if (retval > CgroupV1SubsystemController.UNLIMITED_MIN) {
> 447 if (memory.isHierarchical()) {
> 448 // memory.memsw.limit_in_bytes returned unlimited, attempt
> 449 // hierarchical memory limit
> 450 String match = "hierarchical_memsw_limit";
> 451 retval =
> CgroupV1SubsystemController.getLongValueMatchingLine(memory,
> 452 "memory.stat",
> 453 match);
Same for this.
> 454 }
> 455 }
> 456 return CgroupV1SubsystemController.longValOrUnlimited(retval);
At this point we map any large value to -1, unlimited, or return
'retval'.
> 457 }
>
>
> jdk/src/java.base/linux/classes/jdk/internal/platform/cgroupv2/CgroupV2Subsystem.java
>
> 278 public long getMemoryAndSwapLimit() {
> 279 String strVal = CgroupSubsystemController.getStringValue(unified,
> "memory.swap.max");
This will either return null, "MAX" or the actual value.
> 280 return limitFromString(strVal);
This maps MAX and null to -1 or the actual numberical value as string
to its representation as long.
> 281 }
>
> So the check you want to clean up does no harm, and might handle "strange"
> cases - why not keep it?
The comment in this block is now misleading:
if (limit >= 0 && memLimit >= 0) {
// we see a limit == 0 on some machines where "kernel does not support swap
limit capabilities"
return (limit < memLimit) ? 0 : limit - memLimit;
}
The only reason why a 0 was observed, was because the cgroup interface
files were missing and the old code mapped that to a 0. That's no
longer the case and, thus, it seems it would make the code clearer if
it wouldn't be there any more.
I don't feel strongly about this, though, and can just drop this patch.
The fix of JDK-8236617 has been superseded by JDK-8244500.
Thanks,
Severin
>
>
> Best regards, Matthias
>
>
> -----Original Message-----
> From: Severin Gehwolf <sgehw...@redhat.com>
> Sent: Mittwoch, 15. Juli 2020 11:47
> To: core-libs-dev <core-libs-dev@openjdk.java.net>; serviceability-dev
> <serviceability-...@openjdk.java.net>
> Cc: Baesken, Matthias <matthias.baes...@sap.com>; Bob Vandette
> <bob.vande...@oracle.com>
> Subject: Re: [PING?] RFR(s): 8247863: Unreachable code in
> OperatingSystemImpl.getTotalSwapSpaceSize()
>
> Anyone?
>
> On Mon, 2020-06-29 at 17:53 +0200, Severin Gehwolf wrote:
> > Hi,
> >
> > Could I please get a review of this dead-code removal? During review of
> > JDK-8244500 it was discovered that with the new cgroups implementation
> > supporting v1 and v2 Metrics.getMemoryAndSwapLimit() will never return
> > 0 when relevant cgroup files are missing. E.g. on a system where the
> > kernel doesn't support swap limit capabilities. Therefore this code
> > introduced with JDK-8236617 can no longer be reached and should get
> > removed.
> >
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8247863
> > webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8247863/01/webrev/
> >
> > Testing: Matthias tested this on the affected system and it did pass
> > for him. Docker tests on cgroup v1 and cgroup v2.
> >
> > Thanks,
> > Severin
