RFR: 8221554: aarch64 cross-modifying code

[Alan Hayward]

On Fri, 2 Oct 2020 08:06:16 GMT, Alan Hayward <github.com+4146708+a74nh at openjdk.org> wrote:

>> The only reason for having cmf() on thread is the validation in debug builds, right?
>> If you do it the opposite way:
>> 
>> inline void OrderAccess::cross_modify_fence_non_arch_specific() {
>>     OrderAccess::cross_modify_fence_arch_impl();
>> #ifdef ASSERT
>>     if (VerifyCrossModifyFence) {
>>       Thread::current()->set_requires_cross_modify_fence(false);
>>     }
>> #endif
>> }
>> 
>> You only need the boolean in debug builds on JavaThreads and you don't need to move the cmf() from OA and create the
>> new one?
>
>> _Mailing list message from [David Holmes](mailto:david.holmes at oracle.com) on
>> [hotspot-dev](mailto:hotspot-dev at openjdk.java.net):_
>> Hi Alan,
>> 
>> On 1/10/2020 2:30 am, Alan Hayward wrote:
>> 
>> > The AArch64 port uses maybe_isb in places where an ISB might be required
>> > because the code may have safepointed. These maybe_isbs are very conservative
>> > and are used in many places are used when a safepoint has not happened.
>> > cross_modify_fence was added in common code to place a barrier in all the
>> > places after a safepoint has occurred. All the uses of it are in common code,
>> > yet it remains unimplemented on AArch64.
>> > This set of patches implements cross_modify_fence for AArch64 and reconsiders
>> > every uses of maybe_isb, discarding many of them. In addition, it introduces
>> > a new diagnostic option, which when enabled on AArch64 tests the correct
>> > usage of the barriers.
>> > Advantage of this patch is threefold:
>> > * Reducing the number of ISBs - giving a theoretical performance improvement.
>> > * Use of common code instead of backend specific code.
>> > * Additional test diagnostic options
>> > Patch 1: Split cross_modify_fence
>> > =================================
>> > This is simply refactoring work split out to simplify the other two patches.
>> > instruction_fence() is provided by each target and simply places
>> > a fence for the instruction stream.
>> > cross_modify_fence() is now a member of JavaThread and just calls
>> > instruction_fence. This function will be extended in Patch 3.
>> 
>> I don't agree with the change here. The cross_modify_fence() is not
>> related to thread API imo, it belongs in OrderAccess. The name was
>> deliberately selected to abstract away from the specific details of why
>> a given platform may need this fence:
>> 
>> http://mail.openjdk.java.net/pipermail/hotspot-dev/2019-March/037153.html
>> 
>> "The name "instruction_pipeline" seems a bit implementation specific
>> about what HW architectural features need to be taken care of due to
>> cross-modifying code, which may or may not apply to a given platform.
>> Perhaps cross_modify_fence(), or something along those lines, would be
>> better. That makes it more clear what we are protecting against, as
>> opposed to what HW architectural features that might concern on a given
>> platform."
>> 
>> @robehn , @fisk please chime in here. :)
>> 
>> Thanks,
>> David
> 
> I have no strong feeling on the names of these functions.
> The reason for moving it was that in the third part (the verification testing) it needs JavaThread.
> In an earlier version I simply had OrderAccess::cross_modify_fence(JavaThread *thread). But then OrderAccess is
> dependant on JavaThread, which felt wrong. Obvious solution was to add a wrapper in JavaThread that calls down to
> OrderAccess. Alternatively, I could switch the name of instruction_fence back to cross_modify_fence, and then think of
> a name for the added function in JavaThread. Alternatively alternatively, they could both be call cross_modify_fence.
> If the verification test patch was removed from the set, then most of the first patch wouldn't be needed either.

> _Mailing list message from [Andrew Haley](mailto:aph at redhat.com) on [hotspot-dev](mailto:hotspot-dev at openjdk.java.net):_
> 
> On 02/10/2020 09:08, Alan Hayward wrote:
> 
> > I have no strong feeling on the names of these functions.
> > The reason for moving it was that in the third part (the verification testing) it needs JavaThread.
> 
> Right, but you can get the JavaThread efficiently any time you want,
> so you don't need to pass it to cross_modify_fence().
> 

Oh, ok, didn't spot that.
This would result in code in OrderAccess.cpp calling a function in JavaThread.
It feels that OrderAccess should be much lower level than JavaThread. But, that might be ok.

-------------

PR: https://git.openjdk.java.net/jdk/pull/428