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

[Zhengyu Gu]

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
>>>>
>>