RFR: 8261391: ZGC crash - SEGV in RevokeOneBias::do_thread

[Robbin Ehn]

On Wed, 17 Feb 2021 10:21:06 GMT, Robbin Ehn <rehn at openjdk.org> wrote:

>> src/hotspot/share/runtime/handshake.cpp line 541:
>> 
>>> 539:       }
>>> 540: 
>>> 541:       op->prepare(_handshakee, current_thread);
>> 
>> Okay, just I'm clear on this fix:
>> - You are replacing this single `StackWatermarkSet::start_processing` call on the handshakee with a call to the new prepare() function.
>> - The new prepare() function always does the `StackWatermarkSet::start_processing` call on the `current_target` which is the handshakee.
>> - The new prepare() function does a `StackWatermarkSet::start_processing` call on executioner/executer/executing_thread when that thread is different than the requester/handshaker thread.
>> 
>> So you have multiple names for the "target": `target`, `handshakee`,  and `current_target`. Is there a reason to create the new name `current_target` instead of using either `target` or `handshakee` that you've used before? Since `current_target` is new name in this fix, I would like to see that one renamed to either `target` or `handshakee` in this fix.
>> 
>> I think you also have multiple names for the "requester": `requester` and `handshaker`, but I'm less confused by that naming. In this fix, you're now adding a new name: `executioner` which could be called `executer` or `executing_thread`.
>> 
>> Perhaps you could clean up this naming confusion is follow-up RFE that _only_ fixes naming.
>
> 1: Request the handshake
>   - The thread doing this is the owner of the handshake operation
>   - Handles in the handshake operation belongs to this thread and we must do SWS::start_proc() on it
> 
> 2: Target thread(s)
>   - The thread(s) which the operation is performed on
>   - We must do SWS::start_proc() on it
> 
> 3: Executer of the handshake
>   - The thread the executes the code that the handshake operation contains.
> 
> Handshake operation:
>  _requester is the thread requested the operation
>  _target is NULL if we target all threads, otherwise it is the target thread.
> 
> The prepare() method is part of the handshake operation and need some additional information.
> It needs to know the thread executing the handshake and it needs the current target (since _target may be NULL).
> 
> The HandshakeState name for the thread owning the handshake state is handshakee, which is a private implementation detail.
> When the handshake state is processing handshakes it calls prepare with _handshakee as current_target and current_thread as parameters to prepare().
> 
> target may be NULL
> current target may not be NULL
> handshakee refers to the handshake state owner (not the target of a handshake operation)
> 
> They are thus not interchangeable, but at the moment of execution they can all refer to the same thread.

I now see that I missed call prepare when executing the handshake by handshakee, fixing!

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

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