RFR: 8186837: Memory ordering nmethod, _state and _stack_traversal_mark

[Roman Kennke]

It sounds to me like LA/RL would be required and sufficient on _state ?

Roman

Am 29. August 2017 16:50:03 MESZ schrieb Robbin Ehn <robbin.ehn at oracle.com>:
>Hi Roman thanks for having a look,
>
>On 08/29/2017 03:00 PM, Roman Kennke wrote:
>> Hi Robin,
>> 
>> I doubt that we can assume a symmetry between loadload and storestore
>like there is with load-acquire and release-store. This doesn't seem
>right. In my experience loadload 
>> and storestore are rather special purpose: loadload ensures ordering
>between otherwise unrelated loads and storestore likewise with stores.
>
>This exactly why I add loadload, to stop reordering of unrelated loads:
>
>#######################
>The original code did:
>
>//nmethod::make_not_entrant_or_zombie
>store _stack_traversal_mark
>storestore
>store _state
>
>//NMethodSweeper::process_compiled_method
>load _state
>load _stack_traversal_mark // this is a none-volatile load, can be
>reordered by both gcc and hardware
>
>#######################
>Adding la/sr + volatile to _stack_traversal_mark:
>
>store _stack_traversal_mark release // release not needed, we have a
>following storestore for the unrelated stores
>storestore
>store _state
>
>load _state
>load _stack_traversal_mark acquire // acquire not needed since we
>already loaded _state and any following writes/reads will be done after
>we have taken a Mutex.
>
>#######################
>So therefore my conclusion was that, in this particular case:
>
>store _stack_traversal_mark
>storestore
>store _state
>
>load _state
>loadload
>load _stack_traversal_mark
>
>would be correct, agree?
>
>And as I said I have created another jira issue for the concerns me,
>you and David share.
>
>Thanks Robbin
>
>> 
>> And even symmetric use of load-acquire and release-store are often
>done wrong: those are not meant to protect concurrent access to the
>field, but to the stuff that is 
>> protected by the field access (think locks), I.e. what happens
>between the LA and RS. At least that is my understanding.
>> 
>> I suggest to do what David said and try to understand what concurrent
>accesses to which fields we have, and which fences are actually needed
>to ensure correct ordering.
>> 
>> And thanks for revisiting this!
>> 
>> Cheers, Roman
>> 
>> Am 29. August 2017 12:31:17 MESZ schrieb Robbin Ehn
><robbin.ehn at oracle.com>:
>> 
>>     Hi please review,
>> 
>>     The issue 8180932 - "Parallelize safepoint cleanup" changed
>_stack_traversal_mark to load acquire/store release, this is at least
>half wrong.
>>     Instead for simplicity the write side storestore fence should be
>match with loadload on read side and the changes to
>_stack_traversal_mark undone (kept it volatile).
>> 
>>     Bug:
>>     https://bugs.openjdk.java.net/browse/JDK-8186837
>> 
>>     Code:
>>     http://cr.openjdk.java.net/~rehn/8186837/hotspot.01/webrev/
>> 
>>     It's not clear in this code if there other concurrent dependent
>read/writes.
>>     Is true that only when reading/writing _state and
>_stack_traversal_mark proper memory ordering is needed?
>>     To track that I
>created:https://bugs.openjdk.java.net/browse/JDK-8186839
>> 
>>     Thanks Robbin
>> 
>> 
>> -- 
>> Diese Nachricht wurde von meinem Android-Gerät mit K-9 Mail gesendet.

-- 
Diese Nachricht wurde von meinem Android-Gerät mit K-9 Mail gesendet.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20170830/7c62a8c7/attachment.html>