RFR: 8261391: ZGC crash - SEGV in RevokeOneBias::do_thread [v3]

[David Holmes]

On 18/02/2021 5:39 pm, Robbin Ehn wrote:
> On Thu, 18 Feb 2021 02:10:56 GMT, David Holmes <dholmes at openjdk.org> wrote:
> 
>>> Robbin Ehn has updated the pull request incrementally with one additional commit since the last revision:
>>>
>>>    Tiny spelling error
>>
>> src/hotspot/share/runtime/handshake.cpp line 84:
>>
>>> 82:  public:
>>> 83:   AsyncHandshakeOperation(AsyncHandshakeClosure* cl, JavaThread* target, jlong start_ns)
>>> 84:     : HandshakeOperation(cl, target, NULL), _start_time_ns(start_ns) {}
>>
>> Not at all clear why requester is always NULL here. There is obviously a requester thread even in the async case. If that thread created a Handle in the op then don't we need to call start_processing on it?
> 
> The asynchronous handshake may never be executed, thus having an infinity life-time. (thread never leaves native/blocked)
> Referring to a the requesting thread means we must keep it alive forever also.
> If you refer to stack-based resources from a thread, means that thread should not touch it's stack before you are done.
> If the requester can't use it stack until you are done, you essentially have a synchronous handshake.
> The only way we have to keep the requesting thread alive is to put in a ThreadsList, which means we would keep all threads alive potentially forever (until end of VM-life). But still a Handle would be unsafe because it's stack-based.

I've been confusing Handles with Resources - not realising a Handle is 
stack-local and so shouldn't really be shared with another thread. So 
are we solving the wrong problem here? Rather than making it "safe" for 
one thread to access another's Handle, should we not be using a 
different mechanism to pass the oop between threads?

> Therefore it's hard-coded to NULL. (are we suppose to use nullptr now ?)

Yes nullptr (when we remember :) )

Thanks,
David

>> src/hotspot/share/runtime/handshake.cpp line 328:
>>
>>> 326:
>>> 327: void Handshake::execute(HandshakeClosure* hs_cl) {
>>> 328:   HandshakeOperation cto(hs_cl, NULL, Thread::current());
>>
>> It is unfortunate that we need to manifest the current thread here (and elsewhere).
> 
> Yes, I did that since I did not want to change the API.
> I was thinking of:
> Handshake::execute(HandshakeClosure* hs_cl, Thread* requester = NULL)
> And then go over calls site and see if we have the current thread in a variable.
> But I felt that would be better off in another change, if wanted.
> 
> -------------
> 
> PR: https://git.openjdk.java.net/jdk/pull/2571
>