RFR: 8310239: Add missing cross modifying fence in nmethod entry barriers

[David Holmes]

On Mon, 19 Jun 2023 15:26:37 GMT, Erik Österlund <eosterlund at openjdk.org> wrote:

> In fact, there is a current race in the nmethod entry barriers, where what we are doing violates the AMD APM (cf. APM volume 2 section 7.6.1 https://www.amd.com/system/files/TechDocs/24593.pdf). 
> In particular, if the compare instruction of the nmethod entry barrier is not yet patched and we call a slow path on thread 1, then before taking the nmethod entry lock, another thread 2 could fix and disarm the nmethod. Then thread 1 will observe *data* suggesting the nmethod has been patched, but never re-executes the patched compare (which might indeed still be stale), hence not qualifying for asynchronous cross modifying code, and neither do we run a serializing cpuid instruction, qualifying for synchronous cross modifying code. In this scenario, we can indeed start executing the nmethod instructions, while observing inconsistent concurrent patching effects, where some instructions will be updated and some not.
> 
> The following patch ensures that x86 nmethod entry barriers execute cross modifying fence after calling into the VM, where another thread could have disarmed the nmethod. I also ensured the other platforms perform their fencing after the VM call, instead of before - including a cross_modify_fence in the shared code for OSR nmethod entries. While fencing before will flush out the instruction pipeline, and it shouldn't be populated with problematic instructions until after we start executing the nmethod again, it feels unnecessary to fence on the wrong side of the modifications it wishes to guard, and hence not strictly following the synchronous cross modifying fence recipe.
> 
> I'm currently running tier1-5 and running performance testing in aurora. In the interest of time, I'm opening this PR before getting the final result, and will report the results when they come in.

src/hotspot/cpu/x86/stubGenerator_x86_32.cpp line 3821:

> 3819:     __ jcc(Assembler::equal, deoptimize_label);
> 3820: 
> 3821:     // In case a concurrent thread disarmed the nmethod, we need to ensure the new instructinos

Typo: instructinos

src/hotspot/cpu/x86/stubGenerator_x86_64.cpp line 3166:

> 3164:   __ jcc(Assembler::equal, deoptimize_label);
> 3165: 
> 3166:   // In case a concurrent thread disarmed the nmethod, we need to ensure the new instructinos

Typo: instructinos

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

PR Review Comment: https://git.openjdk.org/jdk/pull/14543#discussion_r1234674273
PR Review Comment: https://git.openjdk.org/jdk/pull/14543#discussion_r1234674483