RFR(m): 8220351: Cross-modifying code

[Robbin Ehn]

Hi David,

For simplicity I'll talk about x86, which have instruction cache coherency.

Immediate oop example, JavaThread going from native -> java.
The JavaThread starts in native, thus safe, and any safepoint or handshake may 
be executed:

store thread_state = _thread_in_native_trans (unsafe)
StoreLoad // e.g. lock addl
load thread_poll
test thread_poll
jcc slow_path
store thread_state = _thread_in_Java
ret

return site:
load <immediate oop> // No data

If the <immediate oop> is updated we are not guaranteed to see it without
mfence. The lock instruction, according to x86 manuals, have no effect on
instruction stream.
Even if the lock instruction would have effect on instruction stream, an
on-going safepoint can update the <immediate oop> and finishes between the
StoreLoad and test:

JavaThread:             | VMThread
StoreLoad               |
                         | update <immediate oop>
                         | disarm
load thread_poll        |

This can also happen in the slow path (so there are is a bigger window also).

By adding an instruction stream barrier (mfence) *after* the poll we are 
guaranteed to always see any updates to the instruction stream.

Does this help?

I notice I forgot to remove some out commented lines, I'll update the webrev 
in-place.

Thanks, Robbin

On 2019-03-11 06:05, David Holmes wrote:
> Hi Robbin,
> 
> Sorry but I find it hard to understand how we suddenly need to sprinkle 
> instruction stream synchronization operations all around the VM. I can't even 
> see how some of these locations relate to "code modification". Can you please 
> explain in more detail exactly what the problem is.
> 
> Thanks,
> David
> 
> On 9/03/2019 1:24 am, 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