RFR: 8186837: Memory ordering nmethod, _state and _stack_traversal_mark

[Robbin Ehn]

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.