RFR: 8343191: Cgroup v1 subsystem fails to set subsystem path [v6]

[Sergey Chernyshev]

On Wed, 27 Nov 2024 10:18:24 GMT, Severin Gehwolf <sgehwolf at openjdk.org> wrote:

>> Sergey Chernyshev has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   adjust path suffix in cgroup (v1) version specific code, when root != cgroup
>
> src/hotspot/os/linux/cgroupUtil_linux.cpp line 70:
> 
>> 68:     os::free(limit_cg_path); // handles nullptr
>> 69:     limit_cg_path = os::strdup(cg_path);
>> 70:   }
> 
> We can avoid the duplicate copy of the original cgroup path, which is already captured in `orig` by using:
> 
> 
> jlong lowest_limit = limit < 0 ? phys_mem : limit;
> julong orig_limit = ((julong)lowest_limit) != phys_mem ? lowest_limit : phys_mem;
> 
> 
> And on line 91 we change the condition from:
> 
> 
> if ((julong)lowest_limit != phys_mem) {
> 
> 
> to:
> 
> 
> if ((julong)lowest_limit != orig_limit) {

Thanks! Accepted.

> src/hotspot/os/linux/cgroupUtil_linux.cpp line 129:
> 
>> 127:     lowest_limit = cpus;
>> 128:     os::free(limit_cg_path); // handles nullptr
>> 129:     limit_cg_path = os::strdup(cg_path);
> 
> Same here with the extra allocation of `cg_path`;

done.

> src/hotspot/os/linux/cgroupV2Subsystem_linux.cpp line 322:
> 
>> 320:       log_warning(os, container)("Cgroup v2 path at [%s] is [%s], cgroup limits can be wrong.",
>> 321:         mount_path, cgroup_path);
>> 322:     }
> 
> Why the cast to `char*`?
> 
> We should probably move this warning to `CgroupUtil::adjust_controller`, right before we've determined that we actually need to adjust. I wonder, though, if we should just print the warning and set the cgroup_path to `/` and return early. Otherwise, path adjustment will run with no different result.

Removed extra (char*) cast.

> We should probably move this warning to `CgroupUtil::adjust_controller`, right before we've determined that we actually need to adjust. I wonder, though, if we should just print the warning and set the cgroup_path to `/` and return early. Otherwise, path adjustment will run with no different result.

"../" only appears in corner case with cgroupns=private and the process moved to the outer group. In that specific case we should avoid concatenating with whatever starts with "../".

> test/hotspot/jtreg/containers/docker/TestMemoryWithSubgroups.java line 39:
> 
>> 37:  * @modules java.base/jdk.internal.platform
>> 38:  * @library /test/lib
>> 39:  * @build jdk.test.whitebox.WhiteBox CheckOperatingSystemMXBean
> 
> `CheckOperatingSystemMXBean` seems unused.

fixed

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/21808#discussion_r1863595789
PR Review Comment: https://git.openjdk.org/jdk/pull/21808#discussion_r1863596132
PR Review Comment: https://git.openjdk.org/jdk/pull/21808#discussion_r1863607722
PR Review Comment: https://git.openjdk.org/jdk/pull/21808#discussion_r1863608362