RFR(m): 8220351: Cross-modifying code

[Doerr, Martin]

Hi Robbin,

the rebased version looks good to me.

I'm only missing barrier implementations for aarch64 and SPARC.
I'd expect isb on aarch64 and at least a compiler barrier on SPARC. But I'm not an expert for these platforms.

Best regards,
Martin


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

Hi all, please review.

This is v4, which is just an rebase on-top of "RFR: 8221207: Redo JDK-8218446 -
SuspendAtExit hangs". The commit simplified S/R code a bit, making this patch
simpler.

Full:
http://cr.openjdk.java.net/~rehn/8220351/v4/webrev/
Inc (not complete since v3 did not apply proper, one of the reject wasn't
needed):
http://cr.openjdk.java.net/~rehn/8220351/v4/inc/webrev/

To summarize, before 8221207 we had 4 safe states:
*Off threads_list (_thread_new)
*Blocked          (_thread_blocked)
*In native        (_thread_in_native)
*Suspend          (*ANY*)
*After handshake  (X)

After 8221207 we have:
*Off threads_list (_thread_new) (far from cross-mod code)
*Blocked          (_thread_blocked)
*In native        (_thread_in_native)
*After handshake  (X)

-We have one instruction barrier for when going from _thread_new.
-We have four instruction barrier for when going from blocked:
	- ThreadBlockInVM
	- ThreadBlockInVMWithDeadlockCheck
	- The manual blocked transition for suspended blocked case.
	- Blocked in SS:block() uses the barrier in block_if_requested_slow.
-We let threads doing their own handshake hit the barrier in
block_if_requested_slow.
-We let threads in native stay armed so they hit the barrier in
block_if_requested_slow.

Not disarming threads in native works because, false positives already happens
today and are handled. Both handshakes and safepoints disarms in correct order,
so a JavaThread can notice that is should be armed, but with false positives.
Before first check of handshake/safepoint the JavaThread is in a unsafe state.
After that check a new handshake/safepoint cannot start/progress since the
JavaThread is unsafe. Thus re-arming and continue to next polling site is okay.

Patch only adds barrier for cross-modifying code that happens during a
handshake/safepoint and implements this barrier for those platforms which have a
simple implementaion for accomplish it. sparc/arm32/aarch64 needs additional
work.

The deprecated global poll path does not contain proper barriers, and will be
obsoleted in JDK 14.

Thanks, Robbin

On 2019-03-08 16:24, 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