RFR: 8221554: aarch64 cross-modifying code

[Alan Hayward]

On Fri, 2 Oct 2020 07:42:28 GMT, Robbin Ehn <rehn at openjdk.org> wrote:

>> Agreed. With both patches, and with the JNI isbs removed, that leaves just one safepoint isb in the AArch64 code (plus
>> the other isb in emit_static_call_stub).
>> The test patch exists not just to test the AArch64 but the common code too (ideally it would be extended to other
>> targets too). Are we happy that the cross_modify_fence is called at all the required points? A hole in the common code
>> would fail only very rarely. It does require quite a bit of code to add this test though.
>
> 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.

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

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