RFR: 8154750: Add missing OrderAccess operations to ClassLoaderData lock-free data structures

[Coleen Phillimore]

On 5/27/16 5:58 PM, David Holmes wrote:
> On 28/05/2016 6:59 AM, Kim Barrett wrote:
>>> On May 26, 2016, at 11:39 PM, David Holmes <david.holmes at oracle.com> 
>>> wrote:
>>>
>>> Hi Kim,
>>>
>>> Updated webrev:
>>>
>>> http://cr.openjdk.java.net/~dholmes/8154750/webrev.v2/
>>
>> ------------------------------------------------------------------------------ 
>>
>> src/share/vm/classfile/classLoaderData.cpp
>>  363         // lock-free iteration will either see k or 
>> k->next_link, both
>>  364         // of which are stable
>>
>> I think this new comment isn't needed, given Stefan's reply and your
>> plan to add an assert_locked_or_safepoint to this function.
>
> If that works out yes. A JPRT run without the lock and an assertion 
> for being at a safepoint passed but I need to run more tests. I'd also 
> like Coleen to chime in. :)

Yes, please remove this comment too.

>
>> ------------------------------------------------------------------------------ 
>>
>> src/share/vm/classfile/classLoaderData.cpp
>>  210 void ClassLoaderData::packages_do(void f(PackageEntry*)) {
>>  211   assert_locked_or_safepoint(Module_lock);
>>
>> Lock mismatch.  CLD::packages() protects _packages using the
>> metaspace_lock().
>
> It is the same check that CLDG::packages_do() uses - I just copied the 
> assertions into the CLD methods. But you are right this is a problem.
>
> When I started work on this I queried about the different locks used 
> in different places:
>
> "The purpose of the Module_lock is to protect access to module 
> specific JVM data structures including the ModuleEntryTables and 
> PackageEntryTables.  Each ClassLoaderData has its own instances of 
> these two tables but they can be accessed by multiple threads.
>
> The metaspace lock, in this case, is used when creating these tables 
> and storing their addresses in the classLoaderData's _modules and 
> _packages fields."
>

Right, metaspace_lock protects the fields of ClassLoaderData (and is 
reused to protect the metaspace also).

> So basically the metaspace lock controls creation of the tables but 
> once created they are modified under the modules lock. But the 
> ModuleEntryTable creation actually needs both.
>
> What does that mean for our memory ordering issues? It means that if 
> packages_do() is called with the Modules_lock, then it need not 
> synchronize with a thread lazily creating the package table under the 
> Modules lock. So I can remove the assert in CLD::packages_do and have 
> to initially read _packages with load_ptr_acquire.

http://cr.openjdk.java.net/~dholmes/8154750/webrev.v3/src/share/vm/classfile/classLoaderData.cpp.udiff.html

Can you put a comment in modules_do to describe why you don't need 
load_ptr_acquire? in modules_do and here:

+ // Check if _modules got allocated while we were waiting for this lock.
+ if ((modules = _modules) == NULL) {

The racing thread has done a store_release so you don't need another 
load_acquire after you get the lock?

I've reviewed this and this looks really good.  Thank you for fixing this!

I guess the alternative to the double check locking algorithm is to 
always take the lock but I think this would be slower.  But maybe not.  
Anyway, we can copy this design pattern now from your changes.

Thanks,
Coleen


>
> Well spotted! I'll recheck the other cases as well.
>
> Thanks,
> David
>
>
>
>
>> ------------------------------------------------------------------------------ 
>>
>>