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

[Bob Vandette]

It still looks like the _mount_path can be corrupted if both cgroupv1 and cgroupv2 are mounted.
You’ll only initialize the _mount_path if cgroupv2 is detected in the cgroups file but if cgroupv1
is also mounted, you’ll still clobber the _mount_path.

Maybe you should just do the cgroupv2 processing if /proc/cgroups has cgroupv2 enabled and if that
fails continue looking for cgroup v1 mounts.



Do you really need to do this?  The BSS section should be zeroed.

55   for (int i = 0; i < CG_INFO_LENGTH; i++) {
  56     cg_infos[i]._name = NULL;
  57     cg_infos[i]._cgroup_path = NULL;
  58     cg_infos[i]._root_mount_path = NULL;
  59     cg_infos[i]._mount_path = NULL;
  60   }

You don’t need to check for NULL in cleanup, os::free can handle NULL pointers.

 339 void CgroupSubsystemFactory::cleanup(CgroupInfo* cg_infos) {
 340   assert(cg_infos != NULL, "Invariant");
 341   for (int i = 0; i < CG_INFO_LENGTH; i++) {
 342     if (cg_infos[i]._name != NULL) {
 343       os::free(cg_infos[i]._name);
 344     }
 345     if (cg_infos[i]._cgroup_path != NULL) {
 346       os::free(cg_infos[i]._cgroup_path);
 347     }
 348     if (cg_infos[i]._root_mount_path != NULL) {
 349       os::free(cg_infos[i]._root_mount_path);
 350     }
 351     if (cg_infos[i]._mount_path != NULL) {
 352       os::free(cg_infos[i]._mount_path);
 353     }
 354   }
 355 }

Bob.



> On Feb 28, 2020, at 11:42 AM, Severin Gehwolf <sgehwolf at redhat.com> wrote:
> 
> 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
>>> 
>