Request for review: 6922246: Deadlock during intensive class loading in parallel

[David Holmes]

Igor Veresov said the following on 10/20/10 18:31:
> On 10/20/10 12:16 AM, David Holmes wrote:
>> Igor Veresov said the following on 10/20/10 16:52:
>>> There's no question a store-store barrier is needed after 291 for
>>> non-TSO if we were to make it lock-free (with the current code monitor
>>> lock should contain such barrier anyway).
>>
>> There's no requirement for a storestore barrier when acquiring a lock.
>> You will happen to get that on sparc and x86 if the lock acquisition
>> involves a CAS, but that is not true on other archs. In general terms it
>> is considered safe to move stores into a critical section at which time
>> it can be reordered with other stores inside the critical section.
> 
> Yes, that's what I meant. Basically a lock should guarantee that all 
> stores before the lock precede stores in the critical section, which is 
> essentially a storestore barrier.

But my point is that locks in general don't guarantee this.

>> Right. If it were not safe/correct for more than one thread to create a
>> MDO, and have all but one thrown away, then we'd have a different
>> problem to solve. Which reminds we, if another thread does beat us to
>> storing the MDO then we need to delete the one we created.
> 
> Yes, perhaps we can try to deallocate. Although I suppose the race 
> should occur infrequently and GC will take care of the unused copy anyway.

Ah! Is this a GC'able oop we are creating? I had assumed it was a C/C++ 
object.

David

> igor
> 
>>
>> Cheers,
>> David
>>
>>>
>>> igor
>>>
>>> On 10/19/10 3:59 PM, David Holmes wrote:
>>>> Hi Coleen,
>>>>
>>>> coleen phillimore said the following on 10/20/10 08:11:
>>>>> Summary: allocate methodData before taking out the MethodData_lock
>>>>> open webrev at http://cr.openjdk.java.net/~coleenp/6922246/
>>>>> bug link at http://bugs.sun.com/view_bug.do?bug_id=6922246
>>>>>
>>>>> Thanks to Ramki for the diagnosis and suggestion for the fix.
>>>>
>>>> This is a variation on the unsafe double-checked-locking idiom.
>>>>
>>>> 286 if (method->method_data() == NULL) {
>>>> 291 methodDataOop method_data = oopFactory::new_methodData(method,
>>>> CHECK);
>>>> 292 MutexLocker ml(MethodData_lock, THREAD);
>>>> 295 if (method->method_data() == NULL) {
>>>> 296 method->set_method_data(method_data);
>>>>
>>>> In general terms any stores associated with the method_data 
>>>> construction
>>>> are not guaranteed to be visible by the time set_method_data is called.
>>>> A second thread could then see a non-null method->method_data() but
>>>> access uninitialized method_data values.
>>>>
>>>> A general solution would need memory barriers before 286 and after 291
>>>> to ensure correctness on all platforms. A simpler, though perhaps
>>>> heavier solution would be to use a MutexUnlocker around the allocation:
>>>>
>>>> MutexLocker ml(MethodData_lock, THREAD);
>>>> if (method->method_data() == NULL) {
>>>> methodDataOop method_data = NULL;
>>>> { // need new block for scope
>>>> MutexUnLocker ml(MethodData_lock, THREAD);
>>>> method_data = oopFactory::new_methodData(method, CHECK);
>>>> }
>>>> // someone might have beaten us to it
>>>> if (method->method_data() == NULL) {
>>>> method->set_method_data(method_data);
>>>> }
>>>> }
>>>>
>>>> A lock-less solution as Tom proposed would also require suitable
>>>> memory-barriers.
>>>>
>>>> David
>>>
>