RFR(S): 8239785: Cgroups: Incorrect detection logic on old systems in hotspot

[Severin Gehwolf]

Hi Bob,

Thanks for the review!

On Tue, 2020-02-25 at 15:08 -0500, Bob Vandette wrote:
> http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8239785/01/webrev/src/hotspot/os/linux/cgroupSubsystem_linux.cpp.sdiff.html
> 
> If both cgroupv1 and v2 controllers are mounted, you might have some problems.  You might want to only look for
> cgroup2 mounts if is_cgroupV2 is true otherwise only look for cgroup mounts.

OK.

> 194           cg_infos[i]._mount_path = os::strdup(tmp_mount_point);
> This will stomp the pointer:
> 213           cg_infos[memory_idx]._mount_path = os::strdup(tmpmount);

You are right. If we are on a hybrid hierarchy and for some reason the
cgroup v2 mount shows up *after* cgroup mounts in /proc/self/mountinfo,
we'd get the mount path wrong. So ordering matters! The reason this
worked for me on hybrid is that cgroup2 mounts come *before* cgroup
mounts on the system I've tested.

> You should free all strdup’d pointers in cg_infos in places where you fail and return NULL.
> 
>  81       cg_infos[memory_idx]._name = os::strdup(name);
> 213           cg_infos[memory_idx]._mount_path = os::strdup(tmpmount);
> 214           cg_infos[memory_idx]._root_mount_path = os::strdup(tmproot);

Done now. More thoughts?

Updated webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8239785/03/webrev/

Thanks,
Severin

> > On Feb 25, 2020, at 2:22 PM, Severin Gehwolf <sgehwolf at redhat.com> wrote:
> > 
> > Hi,
> > 
> > Could I please get reviews of this cgroup-related patch? After JDK-
> > 8230305 old systems with no mounted cgroup controllers would get
> > detected as cgroups v2 (wrong). Then, when initializing the v2
> > subsystem, it would fail. The trace about cgroupv2 mount point not
> > found is misleading in this case. While the outcome is similar pre/post
> > patch (NULL cgroup subsystem), I'd like to be explicit about this case.
> > 
> > The suggested fix is to look at /proc/self/mountinfo in order to
> > correctly detect whether this is cgroup v2 or cgroup v1 with no mounted
> > controllers. In the latter case we just stop initialization as we'd
> > fail later in cgroupv1 code anyway. This aligns hotspot code with core-
> > libs after JDK-8239559.
> > 
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8239785
> > webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8239785/01/webrev/
> > 
> > Testing: jdk-submit, hotspot docker tests on cgroup v1 and cgroup v2. All pass.
> > 
> > Thoughts?
> > 
> > Thanks,
> > Severin
> >