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

[Robbin Ehn]

On Tue, 16 Feb 2021 18:15:29 GMT, Daniel D. Daugherty <dcubed at openjdk.org> wrote:

>> Handshakes may contain oop Handles from the requesting thread.
>> A handshake may be executed by another thread than the requester.
>> We must make sure any such Handle is safe to use by other threads before executing the handshake.
>> This change-set adds SWS::start_process() for the requesting thread also (we already do this for the target JavaThread).
>> 
>> Passes t1-5
>
> 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.

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

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