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

[David Holmes]

Thanks Dan!

David

On 30/08/2016 6:41 AM, Daniel D. Daugherty wrote:
> 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
>>>>>>
>>>>
>>
>