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

[David Holmes]

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.

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