[PING?] RFR(s): 8247863: Unreachable code in OperatingSystemImpl.getTotalSwapSpaceSize()

[Severin Gehwolf]

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 <sgehwolf at redhat.com> 
> Sent: Mittwoch, 15. Juli 2020 11:47
> To: core-libs-dev <core-libs-dev at openjdk.java.net>; serviceability-dev <serviceability-dev at openjdk.java.net>
> Cc: Baesken, Matthias <matthias.baesken at sap.com>; Bob Vandette <bob.vandette at 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