(XS) RFR: 8056183: os::is_MP() always reports true when NMT is enabled

[David Holmes]

On 9/09/2014 5:50 PM, Aleksey Shipilev wrote:
> Hi David,
>
> On 09/09/2014 11:21 AM, David Holmes wrote:
>> On 9/09/2014 4:16 PM, Aleksey Shipilev wrote:
>>> On 09/09/2014 09:50 AM, David Holmes wrote:
>>>> http://cr.openjdk.java.net/~dholmes/8056183/webrev/
>>>
>>> I would think this approach sets us up for the subtle bugs by accepting
>>> "false" as the legitimate result in the multi-threaded system, even if
>>> it is transient. I think we need to special-case NMT (e.g. make it use
>>> the non-isMP-predicated Atomics) and leave the general case alone and
>>> protected by the assert.
>>
>> Yes I knew this was fragile but I thought it manageable within the
>> context. However I misread the Windows 64-bit code - it uses a stub for
>> atomic::add(jint) (which is the routine NMT needs) and so it would
>> generate the wrong stub with this fix. :(
>
> Ouch! I wonder how many years it would go unnoticed.

I'm pretty sure the first set of tests on a MP Windows box would have 
have exposed this. Might have taken a while to track down what what 
going wrong though.

>> Note we already have a long existing fragile use of Atomic::xchg to
>> ensure only one thread in a process can try to load the VM. That too
>> caused an issue with is_MP on some platforms and had to be worked around
>> within Atomic::xchg (which is lightly used). I could do a similar hack
>> with the code in the Windows os::atomic_add_bootstrap:
>>
>> jint os::atomic_add_bootstrap(jint add_value, volatile jint* dest) {
>> + if (_processor_count > 0) {  // we're initialized
>>    // try to use the stub:
>>    add_func_t* func = CAST_TO_FN_PTR(add_func_t*,
>>                                      StubRoutines::atomic_add_entry());
>>
>>    if (func != NULL) {
>>      os::atomic_add_func = func;
>>      return (*func)(add_value, dest);
>>    }
>> + }
>>    assert(Threads::number_of_threads() == 0, "for bootstrap only");
>>    return (*dest) += add_value;
>> }
>>
>> Note this already allows for a non-atomic atomic op during
>> bootstrapping. Though I can not see why this stub is needed on x64 -
>> perhaps some optimization relative to the 32-bit code?
>
> I am confused. I thought is_MP() drives the optimization heuristics,
> i.e. if we *know* (is_MP() == false), then we can skip atomic
> operations, and we proper atomics otherwise if we don't know for sure if
> we are running on uniprocessor system. In other words, why does the
> (_processor_count == 0) case uses plain ops?! I would expect it to say
> "we don't know the environment, assume the worst, here is your atomic, sir".

We can't skip atomic ops on uniprocessors. We can skip OrderAccess 
operations on uniprocessors.

> Do we really have any dead-end problems with using atomics during the
> bootstrap?

Not sure what you mean?

>> Changing this would allow is_MP to lie during this early initialization
>> phase, without any harm being introduced. It is fragile I concede, but
>> the initialization process overall is very fragile. I don't have a
>> better short term solution - and I do need a short-term solution for this.
>
> Ok, can we return "true" from is_MP when _processor_count is not yet
> initialized? We will sacrifice the transient performance instead of
> sacrificing transient correctness.

Returning "true" is what got us into the current mess - that causes an 
incorrect stub to be generated (you can see the bug for details).

However, yes I can turn this around and have is_MP return true if 
_processor_count==0; and I can then modify the bootstrap routine on the 
platforms with the problem to skip installing the stub when it sees a 
zero count. So is_MP will still tell lies but now only the platforms 
that can't handle MP have to deal with it.

Thanks,
David


> Thanks,
> -Aleksey.
>