RFR: 8254264: Remove redundant cross_modify_fence()

[David Holmes]

Hi Patricio,

On 16/10/2020 4:09 am, Patricio Chilano wrote:
> Hi David,
> 
> On 10/15/20 5:22 AM, David Holmes wrote:
>> Hi Patricio,
>>
>> On 15/10/2020 1:20 am, Patricio Chilano Mateo wrote:
>>> Hi all,
>>>
>>> Please review the following patch that removes some uneeded uses of 
>>> cross_modify_fence() in common code, in particular
>>> from ~ThreadBlockInVM(), ~ThreadBlockInVMWithDeadlockCheck() and 
>>> java_suspend_self_with_safepoint_check(). These fences
>>> were added before each JavaThread had to disarm itself (8230594). 
>>> After a safepoint/handshake each JavaThread will
>>> always call SafepointMechanism::process_if_requested_slow() when 
>>> transitioning out of the safe state and will execute a
>>> cross_modify_fence(). Tested with mach5 tiers1-7.
>>
>> As you have already done the analysis, could you show the exact call 
>> sequences that allow for the removal of the code as described? I'm 
>> trying to verify what you've stated but trying to reverse engineer the 
>> calling sequences is painful. :)
> So, before each JavaThread had to disarm itself you could have the 
> following sequence:
> 
> - JT T creates ThreadBlockInVM object and then blocks (usually in 
> park()/wait())
> - VMThread arms T for a safepoint operation. Since T is in 
> _thread_blocked state it's safe so safepoint starts.
> - Once safepoint operation is done VMThread disarms T
> - T wakes up and executes ~ThreadBlockInVM(). Since the poll word/page 
> is not armed it just continues execution. If we didn't have that 
> cross_modify_fence() in ~ThreadBlockInVM() we can transition back to 
> java later without executing the fence instruction.

And just to be clear any action that changes code such that a 
cross-modify-fence is needed by other threads, has to perform a global 
safepoint/handshake?

> Same sequence with ~ThreadBlockInVMWithDeadlockCheck and 
> java_suspend_self_with_safepoint_check(). So when transitioning out of 
> the _thread_blocked state a JT wouldn't know if a safepoint happened so 
> we had that fence there unconditionally.
> 
> Today disarming of the poll word/page is done by each JT, so if a 
> safepoint happened while the JT was blocked the poll word/page will 
> remain armed. The JT when transitioning out of the _thread_blocked will 
> call SafepointMechanism::process_if_requested() (in 
> ~TBIVM/~TBIVMWDC/java_suspend_self_with_safepoint_check()), will go 
> through process_if_requested_slow() to check if it needs to process some 
> operation and at the end will execute a cross_modify_fence().

Okay so ...

~ThreadBlockInVM()
-> trans(_thread_blocked, _thread_in_vm);
--> transition(...);
---> SafepointMechanism::process_if_requested(thread)
-----> if (!local_poll_armed(thread)) { // <- It is armed
          return;
        }
        process_if_requested_slow(thread); => OK


~ThreadBlockInVMWithDeadlockCheck()
-> if (SafepointMechanism::should_process(_thread)) {  // True as poll armed
      release_mutex();
      SafepointMechanism::process_if_requested(_thread);
------> if (!local_poll_armed(thread)) { // <- It is armed
           return;
         }
         process_if_requested_slow(thread); => OK

JavaThread::java_suspend_self_with_safepoint_check()
-> if (state != _thread_in_native) {
     SafepointMechanism::process_if_requested(this); => OK
   }

Those cases are all good as you stated.

So my only uncertainty now is the _thread_in_native case for suspension. 
But how can we be _thread_in_native at the time we call 
java_suspend_self_with_safepoint_check? When we call from 
check_safepoint_and_suspend_for_native_trans we are in 
_thread_in_native_trans. So that leaves 
handle_special_runtime_exit_condition, which is called from:

- JavaCallWrapper::JavaCallWrapper
   - thread must be _thread_in_vm here

- SafepointSynchronize::block
   - can't be _thread_in_native (fatal error)

- ~ThreadInVMfromJava()
  - thread must be _thread_in_Java

- ThreadToNativeFromVM
   - thread must be _thread_in_vm

- ~ThreadInVMfromJavaNoAsyncException
   - thread must be _thread_in_vm

So as far as I can see we can never be _thread_in_native when calling 
JavaThread::java_suspend_self_with_safepoint_check. So it seems I don't 
have to worry about that case after all :) (and in any case when leaving 
_thread_in_native we will call 
check_safepoint_and_suspend_for_native_trans so we'd get to the 
cross-modify-fence via that route.)

So long story short: this all looks good to me. :)

Thanks,
David
-----



> 
> Patricio
>> Thanks,
>> David
>>
>>> Thanks,
>>> Patricio
>>>
>>> -------------
>>>
>>> Commit messages:
>>>   - v1
>>>
>>> Changes: https://git.openjdk.java.net/jdk/pull/655/files
>>>   Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=655&range=00
>>>    Issue: https://bugs.openjdk.java.net/browse/JDK-8254264
>>>    Stats: 5 lines in 2 files changed: 0 ins; 5 del; 0 mod
>>>    Patch: https://git.openjdk.java.net/jdk/pull/655.diff
>>>    Fetch: git fetch https://git.openjdk.java.net/jdk 
>>> pull/655/head:pull/655
>>>
>>> PR: https://git.openjdk.java.net/jdk/pull/655
>>>
>