RFR (XS): 8161993: G1 crashes if active_processor_count changes during startup

[Thomas Schatzl]

Hi David,

On Fri, 2016-07-22 at 12:36 +1000, David Holmes wrote:
> Hi Thomas,
> 
> On 21/07/2016 11:05 PM, Thomas Schatzl wrote:
> > 
> > Hi David,
> > 
> >   thanks for your review.
> Thanks for picking up 8147910 for me. One of those little tasks I 
> couldn't quite justify doing at the moment :)

Np. Just want to get that unnecessary g1 crash fixed :)

[...]
> 
> > > out. All that would involve is committing the os.hpp and os.cpp
> > > and any change to error reporting under 8147910, and the rest
> > > under 8161993. A single review thread and combined push is still
> > > fine.
> > Okay, can do the split of this CR into two.
> > 
> > I briefly looked through the hs_err log file, but could not find
> > any particular section where this information would fit well. Any
> > suggestions?
> In os.cpp I would modify os::print_cpu_info as follows:
> 
> void os::print_cpu_info(outputStream* st, char* buf, size_t buflen) {
>    // cpu
>    st->print("CPU:");
>    st->print("total %d", os::processor_count());
>    // It's not safe to query number of active processors after crash
>    // st->print("(active %d)", os::active_processor_count());
> + // but we can report the initial number of active processors
> + st->print(" (initial active %d)",
> os::initial_active_processor_count());
>    st->print(" %s", VM_Version::features_string());
>    st->cr();
>    pd_print_cpu_info(st, buf, buflen);
> }
> 
> (I prefer the term "available" to "active" but the later is ingrained
> in the VM :(  ).

Yeah, but let's do this renaming another time.

Thanks for the suggestion.

[...]
> > > The call could be put in the os::init_before_ergo() (or
> > VMVersion::init_before_ergo()) call.
> > 
> > I would prefer to avoid code duplication.
> Agreed. init_before_ergo() seems most reasonable of the two - as this
> is  os related. The important thing is that it happens after 
> processor_count() is initialized (which happens earlier within
> os::init)  and before anyone tries to query it (which I think the GC
> will do as part of the ergonomic setup). So an assertion in 
> initial_active_processor_count() to ensure it is not zero would be
> good. And for good measure an assertion that it is zero in 
> set_initial_active_processor_count().
> 
> > > 
> > > Also if we log the initial_active_count we should log the
> > > _processor_count too.
> > I can log both in log messages and the error log - although
> > processor
> > count information is already in the log.
> With the new proposal I would just add the logging to 
> set_initial_active_processor_count().

New webrev:
http://cr.openjdk.java.net/~tschatzl/8161993/webrev.1/ (full, it's 5
lines now, so I did not bother creating a diff webrev)

Testing:
Some more manual checking, jprt

I will post an RFR for JDK-8147910 soon.

Thanks,
  Thomas