[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-runtime-dev
Subject:    Re: (XS) RFR: 8056183: os::is_MP() always reports true when NMT is enabled
From:       David Holmes <david.holmes () oracle ! com>
Date:       2014-09-09 22:40:15
Message-ID: 540F81CF.80400 () oracle ! com
[Download RAW message or body]

For the public record ...

On 9/09/2014 5:21 PM, David Holmes wrote:
> Hi Aleksey,
>
> Thanks for looking at this - you've exposed a fatal flaw.
>
> 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. :(
>
> 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;
> }

This was a completely pointless suggestion as it doesn't affect the 
timing of when the stub was generated. No changes in this area are in 
fact needed. Thanks to Dean Long for pointing this out.

David


> 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?
>
> 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.
>
> IMHO the problem here is NMT - it uses VM services but insists on being
> able to run before the VM can reliably provide those services. I don't
> think the ability to track a few early allocations is a good trade-off
> for the fragility this code introduces. I don't know whether NMT can be
> specialized to handle this pre-initialization usage.
>
>> Or, can we initialize _processor_count more eagerly?
>
> As I understand it the NMT usage occurs before any part of the VM is
> initialized, so even if we could make this the very first thing
> initialized, I think it would still be too late.
>
> Thanks,
> David
>
>> Thanks,
>> -Aleksey.
>>
>>
[prev in list] [next in list] [prev in thread] [next in thread]