RFR: 8253435: Cgroup: 'stomping of _mount_path' crash if manually mounted cpusets exist

[Volker Simonis]

On Wed, 23 Sep 2020 09:49:00 GMT, Severin Gehwolf <sgehwolf at openjdk.org> wrote:

>>> > Did you run container tests with this?
>>> 
>>> Yes. It took me some time to find out that I have to set `-Djdk.test.docker.image.name=ubuntu` and
>>> `-Djdk.test.docker.image.version=18.04` on Ubuntu in order to run them :)
>> 
>> Yes, this is a bit painful. I should have said that.
>> 
>>> For release builds they all pass with and without the change (except `TestJFRWithJMX.java` which always fails, but I
>>> don' t think that's related to this issue). Fast- and slowdebug builds don't even start without the fix and pass all
>>> the tests with it (again except `TestJFRWithJMX.java`).
>> 
>> Hmm, which ones did you run? It seems odd that they fail to run in fastdebug config.
>> 
>> FWIW, I've crafted a regression test for this issue. Please include something like that if you can:
>> https://github.com/simonis/jdk/pull/1
>> 
>> A fix like this should make it pass (uses the `/sys/fs/cgroup` convention) - on top of your change:
>> 
>> diff --git a/src/hotspot/os/linux/cgroupSubsystem_linux.cpp b/src/hotspot/os/linux/cgroupSubsystem_linux.cpp
>> index 21be7a8260c..4c6ae541929 100644
>> --- a/src/hotspot/os/linux/cgroupSubsystem_linux.cpp
>> +++ b/src/hotspot/os/linux/cgroupSubsystem_linux.cpp
>> @@ -295,10 +295,10 @@ bool CgroupSubsystemFactory::determine_type(CgroupInfo* cg_infos,
>>          // Skip cgroup2 fs lines on hybrid or unified hierarchy.
>>          continue;
>>        }
>> -      if (strcmp("none", tmpsource) == 0) {
>> -        // Skip cpusets created manually or by cset/cpuset (https://github.com/lpechacek/cpuset)
>> -        // The "mount source" for these mounts is usually "none" while the source of "true" Cgroup
>> -        // controllers is usually "cgroup". But this is just another heuristic...
>> +      if (strcmp(tmpmount, "/sys/fs/cgroup") < 0) {
>> +        // Skip potentially duplicate, manually mounted cgroup controllers
>> +        // not on /sys/fs/cgroup
>> +        log_info(os, container)("%s not mounted at /sys/fs/cgroup, skipping!", tmpmount);
>
>> > > Did you run container tests with this?
> 
> For the record, the important one to run is this one:
> test/hotspot/jtreg/containers/cgroup/CgroupSubsystemFactory.java
> 
> It's independent of your hosts cgroup files. I believe that test broke with your proposed v1 fix because after your
> patch any `none` entries would be skipped. All of them are `none` for this test after JDK-8252359.

Hi Severin, Bob,

so here comes the new version as discussed. @jerboaa thanks for the additional test. I've merged it and extended it
such that it checks both variants, when the manual csets controller comes before and when it comes after all the other
controllers to exercise both code path in the detection.

All container tests pass now (except `TestJFRWithJMX.java` which doesn't seem to be related to this issue and change).

Are you OK with the change?

Thank you and best regards,
Volker

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

PR: https://git.openjdk.java.net/jdk/pull/295