Request for reviews (XS): 7011386: race in objArrayKlass::array_klass_impl

[Vladimir Kozlov]

Thank you, David

David Holmes wrote:
> David Holmes said the following on 01/12/11 12:21:
>> Vladimir Kozlov said the following on 01/12/11 09:48:
>>> You are right, I added store_store barriers and make these fields 
>>> volatile:
>>>
>>> http://cr.openjdk.java.net/~kvn/7011386/webrev
>>
>> It strikes me as very odd to have to add explicit memory barriers to 
>> code that holds two locks while it executes! I understand the problem 
>> is that there is a lock-free reader of the values being set. Where is 
>> the code that does the lock-free reading of these values?

In objArrayKlass::multi_allocate().

>>
>> Here:
>>
>>  238         ak->set_lower_dimension(this_oop());
>>  239         OrderAccess::storestore();
>>  240         this_oop->set_higher_dimension(ak());
>>  241         OrderAccess::storestore();
>>
>> what is the subsequent store that #241 is ordering with? I think this 
>> is either unnecessary, or else is intended to be a full fence() if we 
>> are really saying that the store at #240 must be made visible to all 
>> other threads.
> 
> On further thought I'd say it is unnecessary. Following the barrier 
> there will be two mutex releases that involve additional barriers. The 
> key requirement is that if you see the store from #240 then you must see 
> the store from #238, and the storestore at #239 achieves that.

I was afraid that the store from 240 could be visible to an other thread
only after locks release so that thread could get old value in the next line:

  230       ak = objArrayKlassHandle(THREAD, this_oop->higher_dimension());

Thanks,
Vladimir

> 
> David
> -----
>> David
>>
>>> Vladimir
>>>
>>> Tom Rodriguez wrote:
>>>> On Jan 11, 2011, at 11:15 AM, Vladimir Kozlov wrote:
>>>>
>>>>> Tom,
>>>>>
>>>>> Using oop_store() instead of oop_store_without_check()
>>>>> will generate barriers but it is overkill.
>>>>
>>>> Those are gc barriers.  I was really referring to fences.  The code 
>>>> is a classic instance of the double check locking idiom for lazy init:
>>>>
>>>>   objArrayKlassHandle ak (THREAD, this_oop->higher_dimension());
>>>>   if (ak.is_null()) {
>>>>     if (or_null)  return NULL;
>>>>
>>>>     ResourceMark rm;
>>>>     JavaThread *jt = (JavaThread *)THREAD;
>>>>     {
>>>>       MutexLocker mc(Compile_lock, THREAD);   // for vtables
>>>>       // Ensure atomic creation of higher dimensions
>>>>       MutexLocker mu(MultiArray_lock, THREAD);
>>>>
>>>>       // Check if another thread beat us
>>>>       ak = objArrayKlassHandle(THREAD, this_oop->higher_dimension());
>>>>       if( ak.is_null() ) {
>>>>
>>>> So doesn't that mean that we need a fence of some kind?
>>>>
>>>> tom
>>>>
>>>>> I think marking receiver as volatile should be enough:
>>>>>
>>>>>  void set_higher_dimension(klassOop k) volatile { 
>>>>> oop_store_without_check((oop*) &_higher_dimension, (oop) k); }
>>>>>  void set_lower_dimension(klassOop k)  volatile { 
>>>>> oop_store_without_check((oop*) &_lower_dimension, (oop) k); }
>>>>>
>>>>> Thanks,
>>>>> Vladimir
>>>>>
>>>>> Tom Rodriguez wrote:
>>>>>> That look good, though I keep wondering if we need a barrier in 
>>>>>> between or if those fields really should be volatile.  It seems 
>>>>>> like we're playing a little loose with the locking for these lazy 
>>>>>> values.
>>>>>> tom
>>>>>> On Jan 11, 2011, at 9:54 AM, Vladimir Kozlov wrote:
>>>>>>> http://cr.openjdk.java.net/~kvn/7011386/webrev
>>>>>>>
>>>>>>> Fixed 7011386: race in objArrayKlass::array_klass_impl
>>>>>>>
>>>>>>> Other threads may access _lower_dimension field before
>>>>>>> it is initialized by thread which holds the lock in
>>>>>>> objArrayKlass::array_klass_impl().
>>>>>>>
>>>>>>> Move _lower_dimension field initialization before
>>>>>>> _higher_dimension.
>>>>