8146115 - Improve docker container detection and resource configuration usage

[Hohensee, Paul]

Hadn’t known about the existence of ‘get_’ getter names. That means there’s a mix currently, which is confusing, but a matter for another RFE or a change to the style guide. See

    https://wiki.openjdk.java.net/display/HotSpot/StyleGuide

which says

    Getter accessor names are noun phrases, with no "get_" noise word. Boolean getters can also begin with "is_" or "has_".

Re GEN_CONTAINER_GET_INFO_STR, on Intel processors the compare-and-branch generated for scan_fmt == NULL is a single cycle and probably similarly cheap on other platforms. Not a lot of overhead in exchange for de-duplication.

Thanks,

Paul

On 9/27/17, 9:10 AM, "Bob Vandette" <bob.vandette at oracle.com> wrote:

    Thanks for the review Paul.  I’ll take care of all of your suggested changes where I haven’t commented below.
    
    > On Sep 25, 2017, at 5:34 PM, Hohensee, Paul <hohensee at amazon.com> wrote:
    > 
    > Hi Bob,
    > 
    > Looks functionally ok, but I’m not an expert on that, so most of this review is sw engineering comments.
    > 
    > In os_linux.cpp:
    > 
    > Is ActiveProcessorCount used to constrain the container? The only use I can see is in active_processor_count(), which just returns the value specified on the command line. Seems to be a nop otherwise.
    Yes, it returns ActiveProcessorCount in the os:active_processor_count() function rather than trying to determine the
    number of active processors by other mechanisms.  This is not a noop.
    
    > 
    > In available_memory(), is the cast to julong in “avail_mem = (julong)(mem_limit – mem_usage);” needed? Maybe to get rid of a compiler warning? Otherwise it’s strange.
    
    Yes, compiler warning.
    
    > 
    > In print_container_info(), “Running in a container: “ is common to both if and else. You could replace the first call to st->print() with
    >    st->print(“container: “; if (!OSContainer::is_containerized()) st->print(“not “); st->print(“running in a container.”)
    > I’d remove the “OSContainer::” prefix string, it’s a bit verbose, plus you probably want an st->cr() at the end of the method.
    > A jlong is a long on some platforms and a long long on others, so I’d replace “0L” with just ‘0’, since ‘0’ will get widened properly.
    
    > Minor formatting nits: ‘else’ goes on the same line as the ‘}’ closing the ‘then’; if an ‘else’ clause isn’t a block, then it usually goes on the same line as the ‘else’.
    
    > 
    > In osContainer.hpp/cpp:
    > 
    > If is_containerized() is frequently called, you might want to inline it. Also, init() is called only from create_java_vm(), so it’s not multi-thread safe, which means that checking _is_initialized and calling init() if it’s not set is confusing. I’d remove _is_initiatialized.
    
    > 
    > Getters in Hotspot don’t have “get_” prefixes (at least, they never used to!), so replace “get_container_type” with “container_type” and “pd_get_container_type” with “pd_container_type”.
    
    There are many examples of get_xxx in hotspot now.
    
    > 
    > In osContainer_linux.hpp/cpp:
    > 
    > In the CgroupSubsystem, you may want to use the os:: versions of strdup and free.
    > MAXBUF is used as the maximum length of various file paths. Use MAXPATHLEN+1 instead?
    
    > Change get_subsystem_path() to subsystem_path()?
    > 
    > Is it really necessary to have a separate GEN_CONTAINER_GET_INFO_STR macro? You could just pass NULL into GEN_CONTAINER_GET_INFO and have it check for scan_fmt == NULL.
    I was trying to avoid additional runtime testing since I already have a lot of required error checking to do.
    
    > 
    > Minor formatting nits: ‘else goes on the same line as the ‘}’ closing the ‘then’; ‘{‘ s/b at the end of the line defining cpuset_cpus_to_count().
    > 
    > The GET_CONTAINER_INFO macro should bracket the code with a block: {}.
    > 
    > Manifest constant 9223372036854771712 should be ‘static const julong UNLIMITED_MEM = 0x7ffffffffffff000;’ or ‘#define UNLIMITED_MEM 0x7ffffffffffff000’.
    > 
    
    Thanks,
    Bob.
    
    > Thanks,
    > 
    > Paul
    > 
    > On 9/22/17, 7:28 AM, "hotspot-dev on behalf of Bob Vandette" <hotspot-dev-bounces at openjdk.java.net on behalf of bob.vandette at oracle.com> wrote:
    > 
    >    Please review these changes that improve on docker container detection and the
    >    automatic configuration of the number of active CPUs and total and free memory
    >    based on the containers resource limitation settings and metric data files.
    > 
    >    http://cr.openjdk.java.net/~bobv/8146115/webrev.00/ <http://cr.openjdk.java.net/~bobv/8146115/webrev.00/>
    > 
    >    These changes are enabled with -XX:+UseContainerSupport.
    > 
    >    You can enable logging for this support via -Xlog:os+container=trace.
    > 
    >    Since the dynamic selection of CPUs based on cpusets, quotas and shares
    >    may not satisfy every users needs, I’ve added an additional flag to allow the
    >    number of CPUs to be overridden.  This flag is named -XX:ActiveProcessorCount=xx.
    > 
    > 
    >    Bob.
    > 
    > 
    > 
    > 
    >