On Wed, 27 Nov 2024 10:18:24 GMT, Severin Gehwolf <sgehw...@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
