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

[David Holmes]

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