RFR: 8230848: OSContainer: Refactor container detection code

Severin Gehwolf sgehwolf at redhat.com
Tue Oct 1 17:54:43 UTC 2019


Hi Bob,

Thanks for looking at this! I appreciate that.

On Tue, 2019-10-01 at 10:43 -0400, Bob Vandette wrote:
> Severin,
> 
> I’d prefer if you just added cgroupv2 support with these types of changes all at the same time.
> Having to look at two patches is just more work for nothing.

OK. I guess that's reviewer preference :) The reason why I did it that
way is to keep the patch sizes smaller. Personally, I tend to find
smaller, incremental patches easier to review than one gigantic patch.
Do you want me to fold these three bugs/patches together and produce
one big patch?

no-op refactoring:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8230848/03/webrev/

added cgroups v2:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8230305/05/webrev/

add Metrics.java support for cgroups v2:
<no patch as of yet>

> In terms of your factoring approach, I don’t see why you need to move so much logic out of os_linux.cpp.

I do find the code easier to follow if the exposed interfaces of
os_container are minimal and kept more broad. To some extent the
osContainer interface is a leaky abstraction. Example: after the
refactoring we'd generally have (in pseudo code):

julong available_memory() {
  if (isContainerized()) {
    return container_available_memory();
  }
  return system_available_memory();
}

over having the osContainer impl in osLinux directly:

julong available_memory() {
  if (isContainerized()) {
     mem_limit = container_mem_limit();
     if (error) {
        trace_container(...)
     }
     mem_usage = container_memory_usage();
     if (error) {
        trace_container(...)
     }
     if (mem_limit > 0 && mem_usage > 0) {
        return mem_limit > mem_usage ? mem_limit - mem_usage : 0;
     }
  }
  return system_available_memory();
}

Perhaps that's just me. We should perhaps do a similar refactoring for
physical memory...

> The OSContainer interface should be used independent of which cgroup implementation is active.

Yes, agreed. I guess the disagreement is in the granularity of the
interface functions.

> os_linux should use the existing OSContainer interface.

OK. If the OSContainer interface is set in stone I will change it back.

My concern was that it exposes too many functions for no good reason
not needed by os_linux (the only consumer). Most functions are used by
print_container_info() anyway, which could be kept more general.
Perhaps I'm missing something?

> OSContainer_linux should include the logic which selects cgroupv1 versus cgroupv2 and
> calls through a common cgroupSubsystem_linux class.  This cgroupSubsystem_linux class
> should provide common functions that both v1 and v2 use. 

Yes. If you look at the cgroups v2 patch, that's what is being
proposed. CgroupSubsystemFactory::create() is being called from
OsContainer_linux. The factory does the selection and returns the
abstract cgroupSubsystem class (either v1 or v2):
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8230305/05/webrev/

> I’d then expect to see cgroupV1Subsystem_linux.?pp and cgroupV2Subsystem_linux.?pp files which 
> extends cgroupSubsystem and contain version specific functions.

Sure, I'd be happy to move things into version specific files if that's
desired. Currently, I've kept them in cgroupSubsystem_linux.{c,h}pp. It
shouldn't be hard to separeate them out. It makes sense to do it
actually.

Thanks again for you input!

Cheers,
Severin

> Bob.
> 
> 
> > On Sep 19, 2019, at 10:20 AM, Severin Gehwolf <sgehwolf at redhat.com> wrote:
> > 
> > Hi,
> > 
> > Please review this code refactoring which will help getting a cgroups
> > v2 implementation for OSContainer implemented. The proposed changes are
> > mentioned in the bug. In essence, after this patch only methods
> > actually called in os_linux.cpp are exposed via OSContainer. Everything
> > else remains an implementation detail. After this refactoring it should
> > also be clearer what the actual bits implemented via cgroups v1 are in
> > hotspot. Functionally there should be no difference before and after
> > this patch.
> > 
> > If you are curious where I'm going with this, have a look at JDK-
> > 8230305 which has a draft of an implementation of cgroups v2 building
> > on top of this patch.
> > 
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8230848
> > webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8230848/03/webrev/
> > 
> > Testing: jdk-submit, tier1 tests on Linux x86_64, container tests
> >         with docker and podman.
> > 
> > Thoughts?
> > 
> > Thanks,
> > Severin
> > 



More information about the hotspot-dev mailing list