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

[Daniel D.Daugherty]

On Mon, 15 Feb 2021 10:23:24 GMT, Robbin Ehn <rehn 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

No code/technical issues with this fix. Just possible naming changes.

src/hotspot/share/runtime/handshake.cpp line 289:

> 287:   StackWatermarkSet::start_processing(current_target, StackWatermarkKind::gc);
> 288:   if (_requester != NULL && _requester != executioner && _requester->is_Java_thread()) {
> 289:     // The handshake closure may contains oop Handles from the _requester.

nit typo: s/contains/contain/

src/hotspot/share/runtime/handshake.cpp line 282:

> 280: };
> 281: 
> 282: void HandshakeOperation::prepare(JavaThread* current_target, Thread* executioner) {

I don't think `executioner` is good name here since it doesn't look
like the Thread passed via that parameter is killing anything.
Perhaps you mean `executer` - as in the Thread that is executing
some specific code/function.

Or maybe go with `executing_thread` to be very clear.

src/hotspot/share/runtime/handshake.cpp line 66:

> 64:     _requester(requester) {}
> 65:   virtual ~HandshakeOperation() {}
> 66:   void prepare(JavaThread* current_target, Thread* executioner);

I don't think `executioner` is good name here since it doesn't look
like the Thread passed via that parameter is killing anything.
Perhaps you mean `executer` - as in the Thread that is executing
some specific code/function.

Or maybe go with `executing_thread` to be very clear.

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.

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

Marked as reviewed by dcubed (Reviewer).

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