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

[David Holmes]

HI Igor,

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.

> But why do you think it needs barrier before 286? 

I was thinking in very general terms from age old discussions on DCL 
whereby from a theoretical perspective you need a read-barrier to make 
sure that the load through the pointer must read from main memory if the 
pointer itself was freshly read from main memory. In our terms we would 
need a loadload barrier between the load of method->method_data and any 
subsequent loads via that pointer. Again this would be a noop on x86 and 
sparc, but not other archs.

> There seems to be 
> nothing wrong in allocating an extra MDO... Moreover, the code is 
> written under this assumption.

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.

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
>