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

[David Holmes]

updated webrev: http://cr.openjdk.java.net/~dholmes/8154750/webrev.v3/

packages_do() uses load_ptr_acquire.

remove_classes() no longer locks, and asserts that we're at a safepoint.

hs-nightly testing still under way.

Thanks,
David

On 28/05/2016 8:02 AM, David Holmes wrote:
> Correction ...
>
> On 28/05/2016 7:58 AM, 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. :)
>>
>>> ------------------------------------------------------------------------------
>>>
>>>
>>> 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.
>
> That should be metaspace_lock.
>
> David
> -----
>
>
> 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
>>
>>
>>
>>
>>> ------------------------------------------------------------------------------
>>>
>>>
>>>