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

[Vladimir Kozlov]

Thank you, David

I will remove second storestore in 241.

Vladimir

David Holmes wrote:
> Vladimir Kozlov said the following on 01/12/11 12:44:
>> 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().
> 
> Thanks - now I see the DCL pattern that Tom described.
> 
>>>>
>>>> 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());
> 
> I assume you meant at 218, which is the same as 230 but without the lock 
> held. Yes it is possible that 218 won't see the value set at 240 until 
> the lock is released, but that is okay, it will proceed to get the lock 
> and recheck at line 230. The key thing is that if 218 does see the value 
> set at 240 then it must also see the value set at 238 - and the 
> storestore achieves that.
> 
> And of course the same for the change in the other file.
> 
> David
> -----
> 
>> 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.
>>>>>>