RFR(m): 8220351: Cross-modifying code

[Doerr, Martin]

Hi Robbin,

some feedback on the platform code:

orderAccess_aix_ppc.hpp should be same as for linux:
inline void OrderAccess::cross_modify_fence()
                                      { inlasm_isync();  }

Not sure if the inline assembler code on x86 necessarily needs a "clobber memory" effect.
I don't know what a C++ compiler is allowed to do if it doesn't know that the code has some kind of memory effect.

For ebx...edx, you could also use clobber if you want to make it shorter.
E.g. with "+a" to use eax as input and output:
  int idx = 0;
  __asm__ volatile ("cpuid " : "+a" (idx) : : "ebx", "ecx", "edx", "memory");

Just a minor suggestion. Not important.

My first impression of the shared changes is good, but I will need more time to think about it.
Thanks again for implementing lazy disarm to get rid of the performance penalty.

Best regards,
Martin




-----Original Message-----
From: Robbin Ehn <robbin.ehn at oracle.com> 
Sent: Dienstag, 19. März 2019 17:44
To: hotspot-dev at openjdk.java.net
Cc: David Holmes <david.holmes at oracle.com>; Andrew Haley <aph at redhat.com>; Erik Österlund <erik.osterlund at oracle.com>; Doerr, Martin <martin.doerr at sap.com>; Reingruber, Richard <richard.reingruber at sap.com>
Subject: Re: RFR(m): 8220351: Cross-modifying code

Hi all, here is v2:

Full:
http://cr.openjdk.java.net/~rehn/8220351/v2/webrev/
Inc:
http://cr.openjdk.java.net/~rehn/8220351/v2/inc/webrev/

The deprecated non-TLH path is missing the correct cross-mod fence, but since 13
will be the last release containing that code path I think that is okay.

We just leave native/native trans armed, and let the thread disarm on the
transition. If it disarms a new safepoint, we re-arm and stop at next polling
site, that avoids recursion.

Passes stress and hs-t1-5.
As in the previous mail, performance numbers look good.

Thanks, Robbin

On 3/8/19 4:24 PM, Robbin Ehn wrote:
> Hi all, please review.
> 
> Issue:
> https://bugs.openjdk.java.net/browse/JDK-8220351
> Changeset:
> http://cr.openjdk.java.net/~rehn/8220351/webrev/
> 
> After a JavaThread have been in a safepoint/(handshake) safe state it can start
> executing updated code. E.g. an oop in the instruction stream can have been
> updated.
> 
> Most hardware's require a barrier or that the code cross modified is far away to
> guarantee that the thread executing the updated instruction stream sees the
> modification.
> 
> What far away is and how far an update instruction stream is from a safepoint
> safe state is not clear.
> 
> To be compliant with those architectures an instruction stream barrier must be
> added when leaving the safepoint safe state.
> 
> There may be crashes today due to this missing barrier.
> A new CPU with deeper pipeline or changes to the VM which moves a safepoint safe
> state closer to a nmethod can increase changes of a crash.
> 
> A few benchmarks will see a performance regression with the extra serializing,
> such as Octane-Crypto -5% (x86).
> 
> With a much more elaborate fix, such as changing the local poll to have more
> than two states (armed/disarmed), it would be possible to only emit such
> instruction stream barrier when the poll have been armed while the thread was
> safepoint safe.
> 
> Passes t1-3 on our platforms, and test built what I can.
> 
> Thanks, Robbin