RFR: 8186837: Memory ordering nmethod, _state and _stack_traversal_mark

[Robbin Ehn]

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.