RFR: 8146115 - Improve docker container detection and resource configuration usage

[David Holmes]

Hi Bob,

On 12/10/2017 5:11 AM, Bob Vandette wrote:
> Here’s an updated webrev for this RFE that contains changes and cleanups 
> based on feedback I’ve received so far.
> 
> I’m still investigating the best approach for reacting to cpu shares and 
> quotas.  I do not believe doing nothing is the answer.

I do. :) Let me try this again. When you run outside of a container you 
don't get 100% of the CPUs - you have to share with whatever else is 
running on the system. You get a fraction of CPU time based on the load. 
We don't try to communicate load information to the VM/application so it 
can adapt. Within a container setting shares/quotas is just a way of 
setting an artificial load. So why should we be treating it any differently?

That's not to say an API to provide load/shares/quota information may 
not be useful, but that is a separate issue to what the "active 
processor count" should report.

> 
> http://cr.openjdk.java.net/~bobv/8146115/webrev.01
> 
> Updates:
> 
> 1. I had to move the processing of AggressiveHeap since the container 
> memory size needs to be known before this can be processed.

I don't like the placement of this - we don't call os:: init functions 
from inside Arguments - we manage the initialization sequence from 
Threads::create_vm. Seems to me that container initialization can/should 
happen in os::init_before_ergo, and the AggressiveHeap processing can 
occur at the start of Arguments::apply_ergo().

That said we need to be sure nothing touched by 
set_aggressive_heap_flags will be used before we now reach that code - 
there are a lot of flags being set in there.

> 
> 2. I no longer use the cpuset.cpus contents since sched_getaffinity 
> reports the correct results
> even if someone manually updates the cgroup data.  I originally didn’t 
> think this was the case since
> sched_setaffinity didn’t automatically update the cpuset file contents 
> but the inverse is true.

Ok.

> 
> 3. I ifdef’d the container function support in 
> src/hotspot/share/runtime/os.hpp to avoid putting stubs in all other os
> platform directories.  I can do this if it’s absolutely necessary.

You should not need to do this if initialization moves as I suggested 
above. os::init_before_ergo() in os_linux.cpp can call 
OSContainer::init(). No need for os::initialize_container_support() or 
os::pd_initialize_container_support.


Some further comments:

src/hotspot/share/runtime/globals.hpp

+           "Optimize heap optnios

Typo.

+   product(intx, ActiveProcessorCount, -1,

Why intx? It can be int then the logging

      log_trace(os)("active_processor_count: "
                    "active processor count set by user : %d",
                    (int)ActiveProcessorCount);

can use %d without casts. Or you can init to 0 and make it uint (and use 
%u).

+   product(bool, UseContainerSupport, true, 
      \
+           "(Linux Only)

Sorry don't recall if we already covered this, but this should be in 
./os/linux/globals_linux.hpp

---

src/hotspot/os/linux/os_linux.cpp/.hpp

187         log_trace(os)("available container memory: " JULONG_FORMAT, 
avail_mem);
  188         return avail_mem;
  189       } else {
  190         log_debug(os,container)("container memory usage call 
failed: " JLONG_FORMAT, mem_usage);

Why "trace" (the third logging level) to show the information, but 
"debug" (the second level) to show failed calls? You use debug in other 
files for basic info. Overall I'm unclear on your use of debug versus 
trace for the logging.

---

src/hotspot/os/linux/osContainer_linux.cpp

Dead code:

  376 #if 0
  377   os::Linux::print_container_info(tty);
  ...
  390 #endif

Thanks,
David

> Bob.