[aarch64-port-dev ] Query regarding backport of memory order changes to JDK7

[Andrew Dinn]

On 14/11/14 16:42, Andrew Haley wrote:
> On 11/14/2014 04:40 PM, Andrew Dinn wrote:
>> Each assignment is followed by a call to OrderAccess::release(). So, why
>> is there a need to call OrderAccess::release() at line:
>>
>>   87 (in method set_last_Java_fp)
>>   . . .
>>   void set_last_Java_fp(intptr_t* fp) { OrderAccess::release();
>> _last_Java_fp = fp; }
>>   . . .
>>
>> Does some other code assign _last_Java_sp without calling
>> OrderAccess::release(); and then call set_last_Java_fp()? If not then
>> surely there is no need to do it in set_last_Java_fp()?
> 
> Probably not.  You understand the issue: please do the Right Thing.

The changes to JavaFrameAnchor methods set_last_Java_sp and
set_last_Java_fp are redundant. But that's only because these methods
are never called**. Anyway, the changes are inappropriate because -- as
I explain below -- there is no need to employ a release when
_last_Java_fp is written and a release may be needed either before or
after the write to _last_Java_sp depending upon the writer context.

[** Well, to be precise JavaFrameAnchor::set_last_Java_fp is called from
JavaThread::set_last_Java_fp() but that method of JavaThread never gets
called]

That means this change set really boils down to modifying methods
JavaFrameAnchor::copy() and JavaFrameAnchor::clear() to include
release() calls at strategic points.

The ppc version of copy() includes one variation on our version.

  void copy(JavaFrameAnchor* src) {
    // In order to make sure the transition state is valid for "this"
    // We must clear _last_Java_sp before copying the rest of the new data
    //
    // Hack Alert: Temporary bugfix for 4717480/4721647
    // To act like previous version (pd_cache_state) don't NULL
_last_Java_sp
    // unless the value is changing
    //
    // <== this hack is now redundant ==>
    if (_last_Java_sp != src->_last_Java_sp) {
      _last_Java_sp = NULL;
      OrderAccess::release();  // <== so this change is also redundant!
    }
    _last_Java_fp = src->_last_Java_fp;
    _last_Java_pc = src->_last_Java_pc;
    // Must be last so profiler will always see valid frame if
has_last_frame() is true
    OrderAccess::release(); // <== *extra release in ppc code* ==>
    _last_Java_sp = src->_last_Java_sp;
  }

That extra line provided in the ppc code is vital while the hack
conditional block and the release inside it are irrelevant. Note that
the missing release at the penultimate line is not important when copy()
is called from JavaCallWrapper() on the way into the Java call stub.
That's because at that point copy() is updating the anchor stored in a
JavaCallWrapper. This /wrapper/ anchor is inaccessible to other threads
until the call stub installs the wrapper in the base frame of a Java
stack segment. So, the important thing is that a release happen some
time before the stub is entered but it doesn't have to be here before
_last_Java_sp is assigned. Indeed, the immediately following call to
clear() in JavaCallWrapper() suffices for this purpose. It nulls out the
/thread/ anchor and executes a release.

  void clear(void) {
    // clearing _last_Java_sp must be first
    _last_Java_sp = NULL;
    OrderAccess::release();
    _last_Java_fp = NULL;
    _last_Java_pc = NULL;
  }

The positioning of the release in clear() /is/ critical when called on
the way in to the Java call stub (that's the only place it is called).
Profiler code running in another thread may try to walk the Java thread
stack starting from _last_Java_sp in the thread anchor. However, it will
only do so if _last_Java_sp is non-NULL. If so then it needs a valid,
corresponding _last_Java_pc. So, the NULLing of the thread anchor's
_last_Java_sp must be ordered before rewriting it's _last_Java_pc to
ensure consistency.

On the way out of the Java call stub the need for the extra release in
copy() in the ppc code becomes apparent. This time ~JavaCallWrapper()
calls  copy() to reinstate the values in the thread anchor from those
saved in the anchor located in the JavaCallWrapper. copy() must ensure
that the fp and pc values are visible before it restores a potentially
non-NULL sp. If the sp value were observed before the pc were updated
then the stack walk could go wrong. So, that's why a release is needed
between the assignments to _last_Javapc/fp and the assignment to
_last_Java_sp.

Of course, that is only one side of the equation. The writes of
_last_Java_fp/pc are only safe if the current sp in the thread anchor is
both NULL and visibly NULL. If not then the profiler still might try to
walk the stack and see an incompatible pc value. How is this avoided?
Before calling copy() ~JavaCallWrapper() NULLs the thread anchor
_last_Java_sp by calling zap() -- which writes /without/ any memory
barrier. It then performs a transition from in_java to in_vm calling
Thread::set_thread_state() and the latter performs a release store.

So, what does the hack achieve? Well, on the way into the Java call stub
it is irrelevant because it is writing a thread-private location ans the
assignment is overwritten some lines later. On the way out it certainly
achieves nothing by writing _last_Java_sp = NULL because _last_Java_sp
will already be NULL when copy() is called. Nor does it ensure that any
pending writes are ordered before the writes to pc and fp because this
ordering of previous writes has already been done. So, it can be removed.

Attached below is my proposed patch. Please review and respond if it is
ok to push.

I believe the same critique applies to the JDK8/9 AArch64 code. So, an
equivalent patch needs to be applied to bring JDK8 in line (it will
differ because it needs to undo existing changes). I will post that
patch separately but would also be grateful for as review and an ok to
push (hmm, can I push to JDK9-aarch64? -- maybe someone else needs to do
that).

regards,


Andrew Dinn
-----------


# HG changeset patch
# User adinn
# Date 1416481731 0
#      Thu Nov 20 11:08:51 2014 +0000
# Node ID f83ab0b76d433c7005c6a97862f6158fa54c7bc4
# Parent  ff8f304c9be48973c06d973a24746c10f95ac10f
Add frame anchor fences.

diff -r ff8f304c9be4 -r f83ab0b76d43
src/cpu/aarch64/vm/javaFrameAnchor_aarch64.hpp
--- a/src/cpu/aarch64/vm/javaFrameAnchor_aarch64.hpp	Fri Nov 14 16:44:37
2014 +0000
+++ b/src/cpu/aarch64/vm/javaFrameAnchor_aarch64.hpp	Thu Nov 20 11:08:51
2014 +0000
@@ -42,25 +42,16 @@
   void clear(void) {
     // clearing _last_Java_sp must be first
     _last_Java_sp = NULL;
-    // fence?
+    OrderAccess::release();
     _last_Java_fp = NULL;
     _last_Java_pc = NULL;
   }

   void copy(JavaFrameAnchor* src) {
-    // In order to make sure the transition state is valid for "this"
-    // We must clear _last_Java_sp before copying the rest of the new data
-    //
-    // Hack Alert: Temporary bugfix for 4717480/4721647
-    // To act like previous version (pd_cache_state) don't NULL
_last_Java_sp
-    // unless the value is changing
-    //
-    if (_last_Java_sp != src->_last_Java_sp)
-      _last_Java_sp = NULL;
-
     _last_Java_fp = src->_last_Java_fp;
     _last_Java_pc = src->_last_Java_pc;
     // Must be last so profiler will always see valid frame if
has_last_frame() is true
+    OrderAccess::release();
     _last_Java_sp = src->_last_Java_sp;
   }