RFR: 8186837: Memory ordering nmethod, _state and _stack_traversal_mark

[Roman Kennke]

OK :-)

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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20170830/12c2b166/attachment.html>