RFR(m): 8220351: Cross-modifying code

[David Holmes]

On 20/03/2019 7:31 pm, Robbin Ehn wrote:
> Hi, v3 updated with Martin's comments.
> 
> Inc:
> http://cr.openjdk.java.net/~rehn/8220351/v3/inc/
> Full:
> http://cr.openjdk.java.net/~rehn/8220351/v3/
> 
> David, are you okay with this model which does cross-mod fence on 
> leaving a safe state? With the other model, always cross-mod fence
> going to java would mean we cannot disarm any thread since we don't
> know which state it's going to to next, if we want the same
> performance.

Sorry Robbin but I can't follow the new approach just from the code 
changes. As I wrote earlier I'd like to see the stat-space enumerated 
here to show why pushing this down to the "safe states" is sufficient 
(and not introducing redundant fences) - e.g we fence when going from 
_thread_blocked to _thread_in_vm which covers the case if we then return 
to _thread_in_java; but if we're returning to native then we don't need 
the fence.

I don't understand at all what your changes to the safepoint/handshake 
disarming logic are doing or how they relate to all this. Can you write 
up the design in the bug report please.

Thanks,
David
-----


> 
> Thanks, Robbin
> 
> On 3/19/19 5:43 PM, Robbin Ehn wrote:
>> 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