RFR(m): 8220351: Cross-modifying code

[Robbin Ehn]

Hi Martin,

On 3/19/19 5:52 PM, Doerr, Martin wrote:
> 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();  }

Sorry, missed it, thanks.

> 
> 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");

Yes, much better thanks!

/Robbin

> 
> 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