RFR: 8221554: aarch64 cross-modifying code

[Alan Hayward]

On Fri, 2 Oct 2020 09:15:46 GMT, Alan Hayward <github.com+4146708+a74nh at openjdk.org> wrote:

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

Patch updated.

* cross_modify_fence now calls cross_modify_fence_impl as suggested.

* ISBs in the JNI calls have been removed. This means that it is currently unsafe to merge until
  https://github.com/openjdk/jdk/pull/296 has been merged.

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

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