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

[Aleksey Shipilev]

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.

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

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

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

Thanks,
-Aleksey.