RFR (S) 8164207: Checking missing load-acquire in relation to _pd_set in dictionary.cpp

Wed Aug 30 13:49:55 UTC 2017

On 8/30/17 7:54 AM, David Holmes wrote:
> On 30/08/2017 9:14 PM, coleen.phillimore at oracle.com wrote:
>>
>> Hi, I changed the edit for David to only use ordering semantics in 
>> the places where needed in the lock free access to pd_set. Since only 
>> contains_protection_domain is read lock free, it should be ok.
>>
>> open webrev at http://cr.openjdk.java.net/~coleenp/8164207.04/webrev
>> bug link https://bugs.openjdk.java.net/browse/JDK-8164207
>
> Thanks Coleen. This looks good to me.

Thanks, David.

Coleen

>
> David
> -----
>
>> Thanks,
>> Coleen
>>
>> On 8/29/17 2:28 AM, David Holmes wrote:
>>> Hi Coleen,
>>>
>>> On 29/08/2017 5:39 AM, coleen.phillimore at oracle.com wrote:
>>>>
>>>>
>>>> On 8/28/17 3:38 PM, coleen.phillimore at oracle.com wrote:
>>>>>
>>>>> Here is the third webrev with the names of pd_set and set_pd_set 
>>>>> renamed to pd_set_acquire and release_set_pd_set.
>>>>
>>>> open webrev at http://cr.openjdk.java.net/~coleenp/8164207.03/webrev
>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8164207
>>>
>>> This API should also be renamed:
>>>
>>> !   ProtectionDomainEntry* pd_set() const            { return 
>>> _inner.pd_set_acquire(); }
>>> !   void set_pd_set(ProtectionDomainEntry* new_head) { 
>>> _inner.release_set_pd_set(new_head); }
>>>
>>> These are the ones that need to give visibility to the fact we're 
>>> accessing things lock-free (if indeed we are).
>>>
>>> More below ...
>>>
>>>>> On 8/28/17 8:07 AM, coleen.phillimore at oracle.com wrote:
>>>>>> On 8/28/17 12:25 AM, David Holmes wrote:
>>>>>>> Hi Coleen,
>>>>>>>
>>>>>>> On 25/08/2017 11:26 PM, coleen.phillimore at oracle.com wrote:
>>>>>>>>
>>>>>>>> Thank you Zhengyu for noticing this change was wrong, and 
>>>>>>>> Christian for the idea.   New webrev:
>>>>>>>>
>>>>>>>> open webrev at 
>>>>>>>> http://cr.openjdk.java.net/~coleenp/8164207.02/webrev
>>>>>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8164207
>>>>>>>
>>>>>>> The idea of a load-acquire accessor and release_store-setter is 
>>>>>>> fine in principal, but it seems to me that we now use these 
>>>>>>> everywhere, even if we may not need them because there is no 
>>>>>>> concurrent/lock-free access. Overall I find it very difficult to 
>>>>>>> determine what the concurrent access patterns are for a 
>>>>>>> Dictionary versus a DictionaryEntry, and which paths are in fact 
>>>>>>> lock and/or safepoint free, and may be racing with locked or 
>>>>>>> safepointed code. ??
>>>>>>
>>>>>> That's exactly the point of making them accessors.  So one 
>>>>>> doesn't have to visit each individual call site and spend time 
>>>>>> answering the question for each case.  And probably getting it 
>>>>>> wrong.   The performance delta for these accesses is minimal 
>>>>>> since it's only getting the head of the list, not each element.
>>>>>>
>>>>>> Then it's also future proof so that if a lock is removed, then we 
>>>>>> don't miss one of the accessors at a later time. Note that 
>>>>>> observing bugs caused by this is very difficult to do, and can 
>>>>>> only be done by inspection.   That's why I erred on the side of 
>>>>>> safety and consistency.
>>>
>>> Sorry, it may sound strange to say that I don't agree with "erring 
>>> on the side of safety and consistency" but I do not agree with just 
>>> using acquire/release semantics everywhere just in case! If we don't 
>>> know the lock-free paths then how can we possibly know things are 
>>> correct. The whole point of these accessors is to make it obvious 
>>> where the lock-free accesses are.
>>>
>>>>>>>
>>>>>>> That aside I don't understand why you added a level of 
>>>>>>> indirection with the ProtectionDomainSet class?
>>>>>>
>>>>>> Only the code is a level of indirection not the access. That is 
>>>>>> to avoid what I said above.  See Christian's and Zhengyu's comments.
>>>
>>> Okay - I see what you did but I would not expect to have to protect 
>>> _pd_set from direct use within its own class - anyone messing with 
>>> that class should be aware of the need to use the accessors. Though 
>>> I suppose this encapsulation is little different to defining the 
>>> field as some kind of "Atomic" type rather than a "raw" type.
>>>
>>> Thanks,
>>> David
>>> -----
>>>
>>>>>>>
>>>>>>> Also we have been trying to include release/acquire in the names 
>>>>>>> of such accessors so that it is clear when we are relying on 
>>>>>>> memory ordering properties ie. pd_set_acquire and 
>>>>>>> release_set_pd_set
>>>>>>>
>>>>>>
>>>>>> I will change the names of these functions.
>>>>>>
>>>>>> thanks,
>>>>>> Coleen
>>>>>>> Thanks,
>>>>>>> David
>>>>>>>
>>>>>>>
>>>>>>>> I reran parallel class loading tests and jck testing is in 
>>>>>>>> progress, but order access requires inspection.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Coleen
>>>>>>>>
>>>>>>>>
>>>>>>>> On 8/24/17 5:11 PM, coleen.phillimore at oracle.com wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 8/24/17 5:00 PM, Christian Thalinger wrote:
>>>>>>>>>>> On Aug 24, 2017, at 10:54 AM, coleen.phillimore at oracle.com 
>>>>>>>>>>> wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On 8/24/17 4:07 PM, Zhengyu Gu wrote:
>>>>>>>>>>>> Hi Coleen,
>>>>>>>>>>>>
>>>>>>>>>>>> There are two instances probably overlooked?
>>>>>>>>>>>>
>>>>>>>>>>>> dictionary.cpp #103 and #124
>>>>>>>>>>>>
>>>>>>>>>>>>     for (ProtectionDomainEntry* current = _pd_set;
>>>>>>>>>>>> =>
>>>>>>>>>>>>     for (ProtectionDomainEntry* current = pd_set();
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>> Oh yeah, you're right.  That's embarrasing. I'll fix and 
>>>>>>>>>>> retest.
>>>>>>>>>> Which also shows that there is a potential for future 
>>>>>>>>>> mistakes. Can we isolate the field better so it’s only 
>>>>>>>>>> accessible via setter and getter?
>>>>>>>>>
>>>>>>>>> Yes, great idea.
>>>>>>>>> Coleen
>>>>>>>>>
>>>>>>>>>>> Thank you!!
>>>>>>>>>>> Coleen
>>>>>>>>>>>
>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>
>>>>>>>>>>>> -Zhengyu
>>>>>>>>>>>>
>>>>>>>>>>>> On 08/24/2017 02:28 PM, coleen.phillimore at oracle.com wrote:
>>>>>>>>>>>>> Summary: Use load_acquire for accessing 
>>>>>>>>>>>>> DictionaryEntry::_pd_set since it's accessed outside the 
>>>>>>>>>>>>> SystemDictionary_lock
>>>>>>>>>>>>>
>>>>>>>>>>>>> Ran parallel class loading tests that we have as well as 
>>>>>>>>>>>>> tier1 tests. See bug for details.
>>>>>>>>>>>>>
>>>>>>>>>>>>> open webrev at 
>>>>>>>>>>>>> http://cr.openjdk.java.net/~coleenp/8164207.01/webrev
>>>>>>>>>>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8164207
>>>>>>>>>>>>>
>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>> Coleen
>>>>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>>
>>>>
>>