<html><head></head><body>It sounds to me like LA/RL would be required and sufficient on _state ?<br>
<br>
Roman<br><br><div class="gmail_quote">Am 29. August 2017 16:50:03 MESZ schrieb Robbin Ehn <robbin.ehn@oracle.com>:<blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<pre class="k9mail">Hi Roman thanks for having a look,<br /><br />On 08/29/2017 03:00 PM, Roman Kennke wrote:<br /><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #729fcf; padding-left: 1ex;"> Hi Robin,<br /> <br /> 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 <br /> and storestore are rather special purpose: loadload ensures ordering between otherwise unrelated loads and storestore likewise with stores.<br /></blockquote><br />This exactly why I add loadload, to stop reordering of unrelated loads:<br /><br />#######################<br />The original code did:<br /><br />//nmethod::make_not_entrant_or_zombie<br />store _stack_traversal_mark<br />storestore<br />store _state<br /><br />//NMethodSweeper::process_compiled_method<br />load _state<br />load _stack_traversal_mark // this is a none-volatile load, can be reordered by both gcc and hardware<br /><br />#######################<br />Adding la/sr + volatile to _stack_traversal_mark:<br /><br />store _stack_traversal_mark release // release not needed, we have a following storestore for the unrelated stores<br />storestore<br />store _state<br /><br />load _state<br />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.<br /><br />#######################<br />So therefore my conclusion was that, in this particular case:<br /><br />store _stack_traversal_mark<br />storestore<br />store _state<br /><br />load _state<br />loadload<br />load _stack_traversal_mark<br /><br />would be correct, agree?<br /><br />And as I said I have created another jira issue for the concerns me, you and David share.<br /><br />Thanks Robbin<br /><br /><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #729fcf; padding-left: 1ex;"> <br /> 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 <br /> protected by the field access (think locks), I.e. what happens between the LA and RS. At least that is my understanding.<br /> <br /> 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.<br /> <br /> And thanks for revisiting this!<br /> <br /> Cheers, Roman<br /> <br /> Am 29. August 2017 12:31:17 MESZ schrieb Robbin Ehn <robbin.ehn@oracle.com>:<br /> <br />     Hi please review,<br /> <br />     The issue 8180932 - "Parallelize safepoint cleanup" changed _stack_traversal_mark to load acquire/store release, this is at least half wrong.<br />     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).<br /> <br />     Bug:<br />     <a href="https://bugs.openjdk.java.net/browse/JDK-8186837">https://bugs.openjdk.java.net/browse/JDK-8186837</a><br /> <br />     Code:<br />     <a href="http://cr.openjdk.java.net/~rehn/8186837/hotspot.01/webrev">http://cr.openjdk.java.net/~rehn/8186837/hotspot.01/webrev</a>/<br /> <br />     It's not clear in this code if there other concurrent dependent read/writes.<br />     Is true that only when reading/writing _state and _stack_traversal_mark proper memory ordering is needed?<br />     To track that I created:<a href="https://bugs.openjdk.java.net/browse/JDK-8186839">https://bugs.openjdk.java.net/browse/JDK-8186839</a><br /> <br />     Thanks Robbin<br /> <br /> <br /> -- <br /> Diese Nachricht wurde von meinem Android-Gerät mit K-9 Mail gesendet.<br /></blockquote></pre></blockquote></div><br>
-- <br>
Diese Nachricht wurde von meinem Android-Gerät mit K-9 Mail gesendet.</body></html>