RFR: 8254264: Remove redundant cross_modify_fence()

[Patricio Chilano]

Hi David,

On 10/16/20 1:58 AM, David Holmes wrote:
> 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?
I'm not sure of that. But we added these fences for the case where the 
code got patched during a safepoint. Maybe Robbin knows the answer.  : )

>> 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. 
Only with ThreadToNativeFromVM, because we switch to native and then 
call has_special_runtime_exit_condition(). Which by the way, I don't see 
why we need to make that call since we are going from vm->native. We 
don't deliver async exceptions and we don't need to check for suspend 
since we are going to native. If we remove that call we can remove that 
conditional in java_suspend_self_with_safepoint_check() and always call 
SafepointMechanism::process_if_requested().

> 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.)
Exactly.

> \So long story short: this all looks good to me. :)
Thanks David!


Patricio
> 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
>>>>
>>