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

[David Holmes]

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

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

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.

Well spotted! I'll recheck the other cases as well.

Thanks,
David




> ------------------------------------------------------------------------------
>