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

[Tom Rodriguez]

On Jan 11, 2011, at 6:28 PM, 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?
>> 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.

That's my sense as well.

tom

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