RFR: 8158854: Ensure release_store is paired with load_acquire in lock-free code

Daniel D. Daugherty daniel.daugherty at oracle.com
Mon Aug 29 20:41:57 UTC 2016


Looks good to me also.

Dan


On 8/23/16 6:04 AM, Zhengyu Gu wrote:
> Thanks. Look good to me.
>
>
> -Zhengyu
>
>
> On 08/23/2016 03:48 AM, David Holmes wrote:
>> Hi Zhengyu,
>>
>> On 22/08/2016 11:13 PM, Zhengyu Gu wrote:
>>> Hi David,
>>>
>>> The changes look good to me.
>>
>> Thanks for the review!
>>
>>> Just a minor comment:
>>>
>>> I saw you made InstanceKlass::_array_klasses pointer "volatile", but 
>>> not
>>> some other places. I know that probably it has not effort, but 
>>> should we
>>> make all these pointers "volatile" just for consistency?
>>
>> Yes. I missed  _methods_jmethod_ids in instanceKLass.hpp, and _next 
>> in classLoader.hpp - now fixed.
>>
>> Webrev updating in place.
>>
>> Thanks,
>> David
>>
>>> Thanks,
>>>
>>> -Zhengyu
>>>
>>>
>>>
>>> On 08/22/2016 12:16 AM, David Holmes wrote:
>>>> I went to push this and realized I hadn't hg add'ed the new
>>>>
>>>> src/share/vm/oops/arrayKlass.inline.hpp
>>>>
>>>> which is also missing from the webrev (but now updated in place).
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>> On 19/08/2016 12:02 PM, Daniel D. Daugherty wrote:
>>>>> On 8/17/16 8:50 PM, David Holmes wrote:
>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8158854
>>>>>>
>>>>>> webrev: http://cr.openjdk.java.net/~dholmes/8158854/webrev/
>>>>>
>>>>> src/share/vm/classfile/classLoader.hpp
>>>>>     No comments.
>>>>>
>>>>> src/share/vm/classfile/verifier.cpp
>>>>>     No comments.
>>>>>
>>>>> src/share/vm/oops/arrayKlass.hpp
>>>>>     No comments.
>>>>>
>>>>> src/share/vm/oops/instanceKlass.cpp
>>>>>     No comments.
>>>>>
>>>>> src/share/vm/oops/instanceKlass.hpp
>>>>>     No comments.
>>>>>
>>>>> src/share/vm/oops/instanceKlass.inline.hpp
>>>>>     No comments.
>>>>>
>>>>> src/share/vm/oops/objArrayKlass.cpp
>>>>>     No comments.
>>>>>
>>>>> src/share/vm/oops/typeArrayKlass.cpp
>>>>>     No comments.
>>>>>
>>>>> src/share/vm/runtime/vmStructs.cpp
>>>>>     No comments.
>>>>>
>>>>> Thumbs up.
>>>>>
>>>>> Dan
>>>>>
>>>>>
>>>>>>
>>>>>>
>>>>>> Generally speaking release_store should be paired with 
>>>>>> load_acquire to
>>>>>> ensure correct memory visibility and ordering in lock-free code 
>>>>>> (often
>>>>>> the read path is what is lock-free). So based on some observations
>>>>>> from earlier bug fixes this bug was intended to examine the use of
>>>>>> release_store and see if we have the appropriate load_acquire as 
>>>>>> well.
>>>>>> The bug report lists all of the cases that were examined - some 
>>>>>> clear
>>>>>> cut correct, some complex correct, some fixed here and some split 
>>>>>> out
>>>>>> into separate issues.
>>>>>>
>>>>>> Here's a summary of the actual changes in the webrev:
>>>>>>
>>>>>>  src/share/vm/classfile/classLoader.hpp
>>>>>>
>>>>>> - next() accessor needs to use load_acquire.
>>>>>>
>>>>>> ---
>>>>>>
>>>>>> src/share/vm/classfile/verifier.cpp
>>>>>>
>>>>>> - load of _verify_byte_codes_fn needs to load_acquire to pair 
>>>>>> with use
>>>>>> of release_store
>>>>>> - release_store of _is_new_verify_byte_codes_fn is not needed
>>>>>>
>>>>>> ---
>>>>>>
>>>>>> src/share/vm/oops/arrayKlass.hpp
>>>>>> src/share/vm/oops/instanceKlass.cpp
>>>>>> src/share/vm/oops/instanceKlass.hpp
>>>>>> src/share/vm/oops/instanceKlass.inline.hpp
>>>>>> src/share/vm/oops/objArrayKlass.cpp
>>>>>> src/share/vm/oops/typeArrayKlass.cpp
>>>>>>
>>>>>> The logic for storing dimensions values was using a storeStore 
>>>>>> barrier
>>>>>> between the lower and higher dimensions. This is converted to use a
>>>>>> release-store setter for higher-dimension, with paired load-acquire
>>>>>> accessor. Plus the accessed fields are declared volatile.
>>>>>>
>>>>>> The methods_jmethod_ids_acquire() and its paired
>>>>>> release_set_methods_jmethod_ids(), are moved to the .inline.hpp file
>>>>>> where they belong.
>>>>>>
>>>>>> ---
>>>>>>
>>>>>> src/share/vm/runtime/vmStructs.cpp
>>>>>>
>>>>>> Updated declaration for _array_klasses  now it is volatile.
>>>>>>
>>>>>> ---
>>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>
>>>
>



More information about the hotspot-runtime-dev mailing list