RFR: 8154750: Add missing OrderAccess operations to ClassLoaderData lock-free data structures
Coleen Phillimore
coleen.phillimore at oracle.com
Tue May 31 22:08:51 UTC 2016
This pattern also exists in
InstanceKlass::mask_for()
which is primarily called walking frames (could be in parallel) during a
safepoint.
Could you apply your change there also?
Thanks,
Coleen
On 5/31/16 5:57 PM, Coleen Phillimore wrote:
>
>
> 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
>>
>>
>>
>>
>>> ------------------------------------------------------------------------------
>>>
>>>
>
More information about the hotspot-runtime-dev
mailing list