RFR: 8257831: Suspend with handshakes [v2]

[David Holmes]

On 7/04/2021 6:37 pm, Robbin Ehn wrote:
> On Wed, 31 Mar 2021 06:08:49 GMT, David Holmes <dholmes at openjdk.org> wrote:
> 
>>> Robbin Ehn has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains two commits:
>>>
>>>   - Merge branch 'master' into SuspendInHandshake
>>>   - 8257831: Suspend with handshake (review baseline)
>>
>> src/hotspot/share/runtime/objectMonitor.cpp line 411:
>>
>>> 409:     OrderAccess::storestore();
>>> 410:     for (;;) {
>>> 411:       current->set_thread_state(_thread_blocked);
>>
>> So IIUC because ThreadblockInVM  is now responsible for handling thread suspension checks, but those checks need to be done at a lower level, we can no longer use ThreadBlockInVM in some places. That both undermines the value of ThreadBlockInVM and make this manual management code much more complex.
>> I agree with Richard that a TBIVM that took an unlock function would make this kind of code much clearer.
> 
> The problem is the we lock the OM from blocked state and then regret it.
> Since we should not execute 'bytecode' during safe state but this implementation does that we end up in this mess.
> If this implementation did the correct thing we could use ThreadblockInVM straight forward.
> As I suggested if we are only blocked around parker it fixes it.

Pushing the thread-state transition closer to the actual part where we 
park/block might seem to avoid this issue, but if we were unparked and 
find ourselves suspended then we still have to wake another waiter, 
which we aren't allowed to do without owning the lock so ...

> The reason for grabbing the lock is that we cannot call exit on the OM which wakes next thread.
> Only the owner may call exit, this makes succession more complicate because you might wake a suspended thread.
> Now the suspended thread needs to wake another thread without owning the lock.

... moving the scope of the TBIVM doesn't fix it.

You've made the TBIVM responsible in part for suspension checks but the 
scope of the TBIVM as used and the scope where we need suspension checks 
don't match.

> And yes adding a method ThreadBlockInVM can help here.

So bottom line is that we can define a new TBIVM that can work in these 
cases and hide all the explicit state machinations?

Thanks,
David

> -------------
> 
> PR: https://git.openjdk.java.net/jdk/pull/3191
>