[PING] RFR: 8231111: Cgroups v2: Rework Metrics in java.base so as to recognize unified hierarchy

[Severin Gehwolf]

Hi Mandy,

Thanks again for the review!

Updated webrev:
incremental (only review changes): http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8231111/11/incremental/webrev/
full: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8231111/11/webrev/

More below.

On Fri, 2020-02-14 at 18:16 -0800, Mandy Chung wrote:
> Hi Severin,
> 
> On 2/11/20 10:04 AM, Severin Gehwolf wrote:
> > Updated webrev:
> > Full: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8231111/10/webrev/
> > incremental: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8231111/10/incremental/webrev/
>  
> Thanks for updating this.  This patch looks okay in general while I
> still think this is quite hard to tell whether the implementation
> matches the spec in the unavailable & unlimited cases.  It'd be good
> if this can be cleaned up in the future.    Thanks for the new test.
> 
> src/java.base/linux/classes/jdk/internal/platform/CgroupSubsystemFact
> ory.java
>   92             logger.log(Level.WARNING, "Mixed cgroupv1 and cgroupv2 not supported. Metrics disabled.");
> 
> 
> There isn't a clear guideline on what logging level to use.  I will
> opt for API not emit any message as this method returns null.  or
> this should throw an exception if this should be reported instead. 
> So debug or trace level seems to be suitable for this message.

Fixed.

> 
>  130             return new CgroupMetrics(new CgroupV2Subsystem(unified));
>  131         } else {
>  132             return new CgroupV1Metrics(CgroupV1Subsystem.getInstance());
> 
> 
> CgroupV2Subsystem is instantiated with a constructor.  Do you expect
> multiple instances of CgroupV2Subsystem (as opposed to
> CgroupV1Subsystem is a singleton)?

I've made CgroupV2Subsystem a singleton too now.

> src/java.base/linux/classes/jdk/internal/platform/cgroupv1/CgroupV1Subsystem.java
>  391                 String match = "hierarchical_memory_limit";
>  392                 retval = CgroupV1SubsystemController.getLongValueMatchingLine(memory,
>  393                                                             "memory.stat",
>  394                                                             match);
> 
> 
> Nit: the match variable is not needed. Similar pattern occurs in line
> 451-453.

Actually, line 391 matches "hierarchical_memory_limit", line 450
matches "hierarchical_memsw_limit". So the match variable *is* needed.
They match different items in file "memory.stat".

> 
> src/java.base/share/classes/jdk/internal/platform/MetricsCgroupV1.java
> 
> This is cgroup v1 specific. I still think it should be moved to
> linux/classes rather than share/classes.  The tests should only run
> on linux. 

OK. Done.

> src/java.base/linux/classes/jdk/internal/platform/CgroupV1Metrics.jav
> a
>    copyright header is missing

Fixed.

>    You can define a private MetricsCgroupV1 subsystem field in this
> class to avoid the casting:
>   15         return ((MetricsCgroupV1)subsystem).getMemoryMaxUsage();
> 
> So CgroupMetrics::subsystem field can stay as private if desire.

Sure (at the cost of an additional field). Changed as suggested.

> Side note:  CgroupV1Metrics is the implementation of
> MetricsCgroupV1.  The naming is a bit strange.  CgroupV1Metrics
> sounds better for the interface and the implementation class could be
> CgroupV1MetricsImpl and it'd help a reader to understand their
> relationship.  I leave it for you to decide.

I wasn't happy with that myself. Changed to CgroupV1Metrics{,Impl} as
suggested.

> src/java.base/share/classes/sun/launcher/LauncherHelper.java
> 
> +    private static final long LONG_RETVAL_NOT_SUPPORTED = -2;

> This is specific to printSystemMetrics.  I suggest to move this to
> printSystemMetrics as a local variable.  Then formatCpuVal and
> formatLimitString to take an additional "unavailable" parameter so
> that these methods will print "N/A" if limit == unavailable.  

Fixed.

> +    private static String formatBoolean(Boolean value, String
> prefix) {
> 
> 
> This is no longer needed.  Please remove it.

Right. Removed.

> test/lib/jdk/test/lib/containers/cgroup/MetricsTester.java
>    The Oracle copyright is taken out and the copyright is also changed from GPL to GPL+CP.
>    The Red Hat copyright can be added to the top of the file immediately before "DO NOT ALTER or REMOVE" line like [1].
> 
> [1] http://hg.openjdk.java.net/jdk/jdk/file/tip/test/hotspot/jtreg/compiler/onSpinWait/TestOnSpinWaitEnableDisable.java

Hmm, old MetricsTester got renamed with this patch to
MetricsTesterCgroupV1. MetricsTesterCgroupV1 still has the old
copyright. The version you've looked at is the common part and
instantiates tester for cgroup v1 or cgroup v2 as required.

Aside: Not sure why old MetricsTester (or new MetricsTesterCgroupV1) is
GPL (over GPL+CP).l

Either way, I've changed license to GPL over GPL+CP for the new test
classes with Red Hat copyright.

> test/jdk/jdk/internal/platform/docker/MetricsMemoryTester.java
>     TestDockerMemoryMetrics requires docker.support.  I think any
> test depending on MetricsMemoryTester.java are filtered when running
> on platform other than linux.  Please verify that.

MetricsMemoryTester is only used by TestDockerMemoryMetrics, which has
@requires docker.support. FWIW, this hasn't changed with this patch.

> test/lib/jdk/test/lib/containers/cgroup/CPUSetsReader.java
>   58         try(Stream<String> stream = Files.lines(Paths.get(path))) {
> 
> Nit: space between "try" and "(" is missing.

Fixed.

> test/jdk/jdk/internal/platform/cgroup/TestCgroupSubsystemController.java
>    I expect the copyright header be placed at the top of the file
> before all imports.  

Fixed.

Thanks,
Severin