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

[David Holmes]

On 22/07/2016 9:35 PM, Thomas Schatzl wrote:
> 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.

I don't expect it to ever be changed :)

> 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)

Looks good!

Thanks,
David

> Testing:
> Some more manual checking, jprt
>
> I will post an RFR for JDK-8147910 soon.
>
> Thanks,
>   Thomas
>