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

[David Holmes]

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;
}

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