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

Patricio Chilano patricio.chilano.mateo at oracle.com
Mon Apr 6 14:57:49 UTC 2020 

Hi Martin,

On 4/6/20 7:15 AM, Doerr, Martin wrote:
> Hi Patricio,
>
> I'm very sorry for the late review. I couldn't make it last week.
>
> Thanks a lot for taking care of the ordering semantics. I'm a big fan of having it explicit, precise, clear and well documented.
Yes, I also think it's more clear now with the explicit orderings. : )

> VM_HandshakeOneThread and VM_HandshakeAllThreads work without the barrier because HandshakeState::try_process with _processing_sem.signal() is always called?
> I think it would be better to have the explicit acquire barrier there, too. Do you agree?
I've been thinking about the VM_HandshakeOneThread and 
VM_HandshakeAllThreads cases since the other day and although I don't 
think there are issues in the use cases we have today it's probably a 
good idea to add the fence there also in case that changes. So for the 
VMThread it is safe without adding the barrier because upon returning 
from the do_it() method it will just notify the Handshaker that the 
operation has been processed and it will proceed to the main loop of 
VMThread::loop() to process the next operation. I think there could be 
an issue if we have one of this non-direct handshakes inside an 
operation that allows nested operations inside it. So when the VMThread 
returns from the handshake, it will go back to the first operation where 
it could try to read some data changed during the handshake. If the 
handshake was actually processed by the handshakee then we could have 
this issue of reading stale data.

> check_state() is pointless in your new version.
Removed.

> I've also looked at your test. Shouldn't we set -XX:+UseBiasedLocking explicitly so the test still works as it should after BiasedLocking deprecation?
Yes, thanks for noticing it. Since I'm planning to push this first I 
will change it now so I don't have to fix it later.


Here is v4 which includes also the memory ordering changes I sent before:

Full:
http://cr.openjdk.java.net/~pchilanomate/8240918/v4/webrev/
Inc:
http://cr.openjdk.java.net/~pchilanomate/8240918/v4/inc/webrev/


Thanks!

Patricio
> Best regards,
> Martin
>
>
>> -----Original Message-----
>> From: David Holmes <david.holmes at oracle.com>
>> Sent: Montag, 6. April 2020 00:40
>> To: Patricio Chilano <patricio.chilano.mateo at oracle.com>; Doerr, Martin
>> <martin.doerr at sap.com>; Robbin Ehn <robbin.ehn at oracle.com>; hotspot-
>> runtime-dev at openjdk.java.net
>> Subject: Re: RFR 8240918: [REDO] Allow direct handshakes without
>> VMThread intervention
>>
>> On 4/04/2020 12:57 am, Patricio Chilano wrote:
>>> On 4/3/20 11:40 AM, Doerr, Martin wrote:
>>>> Hi Patricio,
>>>>
>>>> thanks for the quick reply.
>>>>
>>>> So should we also use precise barriers in
>>>> add_target_count(int count) { Atomic::add(&_pending_threads, count,
>>>> memory_order_relaxed); }
>>>> ?
>>>> Relaxed would fit to your explanation.
>>> Exactly. I was about to send you the diff with those two changes before
>>> answering David and Dan about the acquire fence.   : )
>>>
>>>
>>> diff --git a/src/hotspot/share/runtime/handshake.cpp
>>> b/src/hotspot/share/runtime/handshake.cpp
>>> --- a/src/hotspot/share/runtime/handshake.cpp
>>> +++ b/src/hotspot/share/runtime/handshake.cpp
>>> @@ -56,7 +56,7 @@
>>>        assert(val >= 0, "_pending_threads=%d cannot be negative", val);
>>>        return val == 0;
>>>      }
>>> -  void add_target_count(int count) { Atomic::add(&_pending_threads,
>>> count); }
>>> +  void add_target_count(int count) { Atomic::add(&_pending_threads,
>>> count, memory_order_relaxed); }
>>>      bool executed() const { return _executed; }
>>>      const char* name() { return _handshake_cl->name(); }
>>>
>>> @@ -229,9 +229,8 @@
>>>      // When this is executed by the Handshakee we need a release store
>>>      // here to make sure memory operations executed in the handshake
>>>      // closure are visible to the Handshaker after it reads that the
>>> -  // operation has completed. Atomic::dec() already provides a full
>>> -  // memory fence.
>>> -  Atomic::dec(&_pending_threads);
>>> +  // operation has completed.
>>> +  Atomic::dec(&_pending_threads, memory_order_release);
>>>
>>>      // It is no longer safe to refer to 'this' as the
>>> VMThread/Handshaker may have destroyed this operation
>>>    }
>> The above seems okay. We don't generally try to optimise the barriers
>> associated with these atomic rmw ops due to the complexity of the
>> reasoning around the different types of barriers. But in this case the
>> reasoning seems quite straight-forward. The increment does not indicate
>> anything to any other thread, it simply sets the right value in the
>> counter (which may already have been decremented). The decrement
>> indicates a completion token and so needs at least a release-barrier.
>> The final check that watches for the count to be zero, needs the
>> matching acquire barrier.
>>
>> Thanks,
>> David
>> -----
>>
>>> Thanks Martin!
>>>
>>> Patricio
>>>> Best regards,
>>>> Martin
>>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: Patricio Chilano <patricio.chilano.mateo at oracle.com>
>>>>> Sent: Freitag, 3. April 2020 16:33
>>>>> To: Doerr, Martin <martin.doerr at sap.com>; David Holmes
>>>>> <david.holmes at oracle.com>; Robbin Ehn <robbin.ehn at oracle.com>;
>>>>> hotspot-runtime-dev at openjdk.java.net
>>>>> Subject: Re: RFR 8240918: [REDO] Allow direct handshakes without
>>>>> VMThread intervention
>>>>>
>>>>> Hi Martin,
>>>>>
>>>>> On 4/3/20 11:20 AM, Doerr, Martin wrote:
>>>>>> Hi David,
>>>>>>
>>>>>> the acquire belongs to the load of _pending_threads in is_completed()
>>>>> (handshake.cpp:270).
>>>>>> It's moved below the loop because we don't need to repeat the
>> barrier.
>>>>>> The release barrier is integrated in Atomic::dec(&_pending_threads)
>>>>> (handshake.cpp:234).
>>>>>> Should we better use Atomic::dec(&_pending_threads,
>>>>> memory_order_release) to make it precise and explicit?
>>>>> As you stated in your other email too we can specify explicit memory
>>>>> ordering to Atomic::dec() which I forgot. I agree with explicitly
>>>>> changing it to Atomic::dec(&_pending_threads,
>> memory_order_release).
>>>>>> Release / acquire semantics are exactly what I was missing in the
>>>>>> earlier
>>>>> version.
>>>>>> I don't understand why "add_target_count" is called after the
>>>>>> handshakes
>>>>> were activated (handshake.cpp:171).
>>>>>> Couldn't it happen that the first handshake completes before that
>>>>>> (it would
>>>>> set _pending_threads to 0)?
>>>>> Yes, _pending_threads can even go negative for the
>>>>> VM_HandshakeAllThreads case. But we only check _pending_threads in
>> the
>>>>> VMThread so for the Handshakees it doesn't matter when they
>> decrement
>>>>> the counter what's the actual value.
>>>>>
>>>>> Thanks,
>>>>> Patricio
>>>>>> Best regards,
>>>>>> Martin
>>>>>>
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: David Holmes <david.holmes at oracle.com>
>>>>>>> Sent: Freitag, 3. April 2020 15:22
>>>>>>> To: Patricio Chilano <patricio.chilano.mateo at oracle.com>; Doerr,
>>>>>>> Martin
>>>>>>> <martin.doerr at sap.com>; Robbin Ehn <robbin.ehn at oracle.com>;
>>>>> hotspot-
>>>>>>> runtime-dev at openjdk.java.net
>>>>>>> Subject: Re: RFR 8240918: [REDO] Allow direct handshakes without
>>>>>>> VMThread intervention
>>>>>>>
>>>>>>> Hi Patricio,
>>>>>>>
>>>>>>> On 3/04/2020 6:00 pm, Patricio Chilano wrote:
>>>>>>>> Hi Martin,
>>>>>>>>
>>>>>>>> On 4/2/20 5:24 PM, Doerr, Martin wrote:
>>>>>>>>> Hi Patricio,
>>>>>>>>>
>>>>>>>>> right, you got my point. Sorry for not explaining it in greater
>>>>>>>>> detail
>>>>>>>>> at the beginning. I was busy with other things.
>>>>>>>>>
>>>>>>>>> There should be a memory barrier ordering read of
>> _pending_threads
>>>>>>>>> (which is observed as 0) and whatever gets done when returning
>> from
>>>>>>>>> execute_direct.
>>>>>>>>> I don't like relying on barriers which are hidden somewhere and
>>>>>>>>> may be
>>>>>>>>> subject to change.
>>>>>>>> Yes, I agree with that. I added an explicit acquire fence at the
>>>>>>>> end of
>>>>>>>> Handshake::execute_direct() with a comment. Thanks for noticing it.
>>>>>>> This is unclear to me:
>>>>>>>
>>>>>>> 281   // Prevent future loads from floating above the load of
>>>>>>> _pending_threads
>>>>>>>      282   // in is_completed(). This prevents reading stale data
>>>>>>> modified
>>>>>>> in the
>>>>>>>      283   // handshake closure by the Handshakee.
>>>>>>>      284   OrderAccess::acquire();
>>>>>>>      285
>>>>>>>      286   return op.executed();
>>>>>>>
>>>>>>> An acquire() by itself has no meaning, it has to be associated with a
>>>>>>> release() and both have to be associated with the load, respectively,
>>>>>>> store, of the same variable. From the comment it sounds like you just
>>>>>>> want a loadload() barrier here.
>>>>>>>
>>>>>>> Other updates seem fine.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> David
>>>>>>> -----
>>>>>>>
>>>>>>>>> I'd also prefer explicit barriers on the writers side over the
>>>>>>>>> implicit ones from Atomic::add, because explicit barriers with
>>>>>>>>> comments help a lot understanding the code.
>>>>>>>>> (Not sure if you want to change that. Just my opinion.)
>>>>>>>> I added a comment on top of Atomic::dec() on why we need a
>> memory
>>>>>>>> barrier there and the fact that we are already using a full memory
>>>>>>>> fence.
>>>>>>>> If you still think I should add an explicit barrier for code
>>>>>>>> readability
>>>>>>>> I can add it though.
>>>>>>>>
>>>>>>>>
>>>>>>>> Here is v3 which also addresses Dan's comments.
>>>>>>>>
>>>>>>>> Full:
>>>>>>>> http://cr.openjdk.java.net/~pchilanomate/8240918/v3/webrev/
>>>>>>>> Inc:
>>>>>>>> http://cr.openjdk.java.net/~pchilanomate/8240918/v3/inc/webrev/
>>>>>>>>
>>>>>>>>
>>>>>>>> Thanks!
>>>>>>>> Patricio
>>>>>>>>> Best regards,
>>>>>>>>> Martin
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> -----Original Message-----
>>>>>>>>>> From: Patricio Chilano <patricio.chilano.mateo at oracle.com>
>>>>>>>>>> Sent: Donnerstag, 2. April 2020 19:56
>>>>>>>>>> To: Robbin Ehn <robbin.ehn at oracle.com>; Doerr, Martin
>>>>>>>>>> <martin.doerr at sap.com>; hotspot-runtime-
>> dev at openjdk.java.net
>>>>>>>>>> Subject: Re: RFR 8240918: [REDO] Allow direct handshakes
>> without
>>>>>>>>>> VMThread intervention
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> 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

More information about the hotspot-runtime-dev mailing list