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

[David Holmes]

Updated webrev:

http://cr.openjdk.java.net/~dholmes/8056183/webrev.v2/

Thanks,
David

On 9/09/2014 6:02 PM, David Holmes wrote:
> 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.
>>