Re: [PING?] RFR(s): 8247863: Unreachable code in OperatingSystemImpl.getTotalSwapSpaceSize()
On Thu, 2020-07-30 at 05:28 +, Baesken, Matthias wrote: > > 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. > > Hi Severin, thanks for clarification, I'm fine with the patch ! Thanks for the review, Matthias. Cheers, Severin
RE: [PING?] RFR(s): 8247863: Unreachable code in OperatingSystemImpl.getTotalSwapSpaceSize()
>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. Hi Severin, thanks for clarification, I'm fine with the patch ! Best regards, Matthias
Re: [PING?] RFR(s): 8247863: Unreachable code in OperatingSystemImpl.getTotalSwapSpaceSize()
Hi Matthias,
On Wed, 2020-07-15 at 15:22 +, 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
>
> 444public long getMemoryAndSwapLimit() {
> 445long 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).
> 446if (retval > CgroupV1SubsystemController.UNLIMITED_MIN) {
> 447if (memory.isHierarchical()) {
> 448// memory.memsw.limit_in_bytes returned unlimited, attempt
> 449// hierarchical memory limit
> 450String match = "hierarchical_memsw_limit";
> 451retval =
> CgroupV1SubsystemController.getLongValueMatchingLine(memory,
> 452"memory.stat",
> 453match);
Same for this.
> 454}
> 455}
> 456return 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
>
> 278public long getMemoryAndSwapLimit() {
> 279String strVal = CgroupSubsystemController.getStringValue(unified,
> "memory.swap.max");
This will either return null, "MAX" or the actual value.
> 280return 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
> Sent: Mittwoch, 15. Juli 2020 11:47
> To: core-libs-dev ; serviceability-dev
>
> Cc: Baesken, Matthias ; Bob Vandette
>
> 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
RE: [PING?] RFR(s): 8247863: Unreachable code in OperatingSystemImpl.getTotalSwapSpaceSize()
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) :
jdk/src/java.base/linux/classes/jdk/internal/platform/cgroupv1/CgroupV1Subsystem.java
444public long getMemoryAndSwapLimit() {
445long retval = getLongValue(memory, "memory.memsw.limit_in_bytes");
446if (retval > CgroupV1SubsystemController.UNLIMITED_MIN) {
447if (memory.isHierarchical()) {
448// memory.memsw.limit_in_bytes returned unlimited, attempt
449// hierarchical memory limit
450String match = "hierarchical_memsw_limit";
451retval =
CgroupV1SubsystemController.getLongValueMatchingLine(memory,
452"memory.stat",
453match);
454}
455}
456return CgroupV1SubsystemController.longValOrUnlimited(retval);
457}
jdk/src/java.base/linux/classes/jdk/internal/platform/cgroupv2/CgroupV2Subsystem.java
278public long getMemoryAndSwapLimit() {
279String strVal = CgroupSubsystemController.getStringValue(unified,
"memory.swap.max");
280return limitFromString(strVal);
281}
So the check you want to clean up does no harm, and might handle "strange"
cases - why not keep it?
Best regards, Matthias
-Original Message-
From: Severin Gehwolf
Sent: Mittwoch, 15. Juli 2020 11:47
To: core-libs-dev ; serviceability-dev
Cc: Baesken, Matthias ; Bob Vandette
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
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
