RFR: 8186837: Memory ordering nmethod, _state and _stack_traversal_mark

[Robbin Ehn]

On 08/30/2017 05:03 PM, Roman Kennke wrote:
> OK :-)

Thanks, Robbin

> 
> Cheers, Roman
> 
> 
> Am 30. August 2017 16:50:26 MESZ schrieb Robbin Ehn <robbin.ehn at oracle.com>:
> 
>     Hi Roman,
> 
>     On 08/30/2017 12:14 PM, Roman Kennke wrote:
> 
>         It sounds to me like LA/RL would be required and sufficient on _state ?
> 
> 
>     Yes, but
>     - Using la/rs just sometimes can be confusing, changing to get/set with la/rs semantics also means we need to change assembly to use proper memory ordering,
>         e.g. lda instead of ldr on aarch64 and armv7 add e.g. ldr, dmb, etc...
>         (I asked compiler, their thoughts was dirty reads are most likely okay in all other cases since _state only goes 'up')
>     - Using la/rs in just this case means either duplicate methods with acquire semantic or adding a bool to a lot of methods since these are accessed in deep call hierarchies.
>     - The write side is using storestore today
> 
>     For those reasons I said:
> 
>         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).
> 
> 
>     I'm well aware this no near perfect, but we need that loadload fence, so can you can live with my proposed change?
>     And we can discussed the bigger picture in 8186839?
> 
>     Thanks, Robbin
> 
> 
>         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.
> 
> 
> -- 
> Diese Nachricht wurde von meinem Android-Gerät mit K-9 Mail gesendet.