RFR(m): 8220351: Cross-modifying code

[Doerr, Martin]

Hi David and Robbin,

> 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
I was also struggling a bit with the placement of the cross_modify_fence.
But there's the idea to replace the suspend flags by handshakes.
After that, it seems to me that there will be very few cross_modify_fence usages left which look nicely placed to me.
So if this is the plan, I should be happy with it ��

> I don't understand at all what your changes to the safepoint/handshake 
> disarming logic are doing or how they relate to all this.
That was basically my suggestion.
The first proposal was to introduce CPUID in native wrapper which is costly from performance point of view.
Every Java thread returning from native would have to execute it even if there was no safepoint. And there was no way to detect if there was one.
So my idea was to change the disarming logic such that every thread returning from native must detect if there was a safepoint.
This is done by moving the task of disarming from the VM thread to the thread returning from native.

I guess Robbin will also add a description.

Best regards,
Martin


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

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