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

[Zhengyu Gu]

Sorry for the noise, I did not check BasicHashtableEntry, I see the 
fixes there.

-Zhengyu

On 06/01/2016 09:33 AM, Zhengyu Gu wrote:
> Does it sound like that all the hash tables with lock-free lookup, 
> such as SymbolTable, etc., are exposed with the same issues?
>
> -Zhengyu
>
>
> On 05/31/2016 06:08 PM, Coleen Phillimore wrote:
>>
>> 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
>>>>
>>>>
>>>>
>>>>
>>>>> ------------------------------------------------------------------------------ 
>>>>>
>>>>>
>>>
>>
>