RFR 8240918: [REDO] Allow direct handshakes without VMThread intervention

[Patricio Chilano]

On 4/2/20 1:16 PM, Patricio Chilano wrote:
> Hi Martin,
>
> Thanks for looking at this.
>
> On 4/2/20 10:15 AM, Robbin Ehn wrote:
>> Hi Martin,
>>
>> On 2020-04-02 14:55, Doerr, Martin wrote:
>>> Hi all,
>>>
>>> first of all, thanks, Patricio, for doing this.
>>>
>>> One question:
>>> _pending_threads is used for synchronizing threads.
>>> Don't we need release / acquire semantics?
>>> I guess otherwise handshaker can read stale data from handshakee 
>>> after observing is_completed.
>>> Or did I miss anything?
>>
>> Reading stale should be fine in this case, since the counter goes 
>> from n->0, thus you can only get false negatives from "bool 
>> is_completed()" ?
> Right, as Robbin points out at most we will just have to loop again. 
> Also, when the handshakee processes the handshake, right after 
> updating _pending_threads, it will either signal the _processing_sem 
> if there are no direct operations, or it will signal the 
> _handshake_turn_sem, so that will provide the release already. The 
> next read in the loop should read the updated value. Are you okay with 
> that?
I realize you might be thinking of a different case now. You mean the 
handshaker could execute loads from code after 
Handshake::execute_direct() while it hasn't yet read _pending_threads 
from is_completed()? Although there is a load_acquire() in 
~ThreadsListHandle() that will prevent those issues you might argue that 
we can't rely on code which might change.
I think (not counting that load_acquire from ~ThreadsListHandle) the 
issue could appear for example if you created a ThreadsListHandle first, 
then called Handshake::execute_direct(), and when returning you try to 
read some field from the handshakee (you might want to assert something 
for example). If there wasn't any ThreadsListHandle created before 
calling execute_direct() I think you are not allowed to access other 
JavaThread's data anyway since the thread might have exited already, 
unless you create a ThreadsListHandle which will already provide the 
acquire. So I think you are right and we might need a fence before 
returning from execute_direct() or add a comment that we are relying on 
the load_acquire from ~ThreadsListHandle(). Is that what you were thinking?

Thanks,
Patricio
> Thanks!
>
> Patricio
>> Thanks, Robbin
>>
>>>
>>> Best regards,
>>> Martin
>>>
>>>
>>>> -----Original Message-----
>>>> From: hotspot-runtime-dev <hotspot-runtime-dev-
>>>> bounces at openjdk.java.net> On Behalf Of Robbin Ehn
>>>> Sent: Donnerstag, 2. April 2020 13:36
>>>> To: Patricio Chilano <patricio.chilano.mateo at oracle.com>; 
>>>> hotspot-runtime-
>>>> dev at openjdk.java.net
>>>> Subject: Re: RFR 8240918: [REDO] Allow direct handshakes without
>>>> VMThread intervention
>>>>
>>>> Hi Patricio,
>>>>
>>>> Thanks for fixing, looks good.
>>>>
>>>> /Robbin
>>>>
>>>> On 2020-04-01 19:45, Patricio Chilano wrote:
>>>>> Hi all,
>>>>>
>>>>> Please review this redo for 8230594. Since the reason behind the 
>>>>> Windows
>>>>> timeouts turned out to be 8240902, this patch is almost identical 
>>>>> to the
>>>>> reverse of the backout modulo the rebase.
>>>>>
>>>>> There is only one small fix for an issue not identified in the 
>>>>> original
>>>>> patch which I explained in the comments. To avoid this possible 
>>>>> issue of
>>>>> deleting a JavaThread that called Thread::destroy_vm() while there 
>>>>> are
>>>>> handshakers blocked in the handshake_turn_sem semaphore, I added a
>>>> check
>>>>> to verify the JavaThread in question is not protected by some
>>>>> ThreadsList reference before attempting the delete it. In case it is
>>>>> protected, we need to at least clear the value for _thread_key to 
>>>>> avoid
>>>>> possible infinite loops at thread exit (this can happen in Solaris).
>>>>>
>>>>> The only other change is that I removed the check for local polls in
>>>>> Handshake::execute_direct() since after the backout, thread-local
>>>>> handshakes was implemented for arm32 which was the only remaining
>>>> platform.
>>>>>
>>>>> I tested it several times in mach5 tiers 1-6 and once in t7 and 
>>>>> saw no
>>>>> failures.
>>>>>
>>>>> Bug:
>>>>>      https://bugs.openjdk.java.net/browse/JDK-8240918
>>>>>
>>>>> Webrev:
>>>>> http://cr.openjdk.java.net/~pchilanomate/8240918/v1/webrev/
>>>>>
>>>>>
>>>>> Thanks,
>>>>> Patricio
>