8146115 - Improve docker container detection and resource configuration usage

[Hohensee, Paul]

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.

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.

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”.

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.

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,

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.