RFR: 8266557: assert(SafepointMechanism::local_poll_armed(_handshakee)) failed: Must be [v7]

[Patricio Chilano Mateo]

On Tue, 8 Jun 2021 08:46:50 GMT, Robbin Ehn <rehn at openjdk.org> wrote:

>> Please consider this change-set which address the issue on hand.
>> 
>> I identified two problems:
>> 
>> - is_locked() uses the _owner field which is unordered (no storestore|storeload) on store-side.
>> Fixed by leaving the handshakee being processed in queue until completed.
>> And remove looping, since if ever the queue is empty the handshakee may processed.
>> If ever want to loop again, we must make sure queue is not empty before removing the processed handshake.
>> But there is, at this moment, no performance benefit to that, so I chosse the simple, easy to reason about version. (some crazy stress test can see a difference)
>> 
>> Note that I'll do a follow-up and make is_locked() ifdef ASSERT only.
>> 
>> - have_non_self_executable_operation() only provide correct acquire if first in queue matched, if second item matched it could be re-orderd with reading the poll state.
>> Fixed by adding a loadload.
>> 
>> I could at first reproduce by checking _active_handshaker in update_poll (~1/50) and an increase in the test time by ten.
>> (real reprod ~1/400 with increased test time)
>> If we are updating the poll there should not be an active handshaker.
>> The above fixed the issue.
>> But after a rebase when I was trying to pin point the issue I could no longer reproduce even without any changes.
>> 
>> Added Atomic store/load to _active_handshaker since it may be concurrently loaded (it may only be used to ask if current thread is active handshaker).
>> 
>> Passes stressing relevant test on aarch64 and t1-7.
>> 
>> Thanks, Robbin
>
> Robbin Ehn has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 11 additional commits since the last revision:
> 
>  - Fixed CW header
>  - Merge branch 'master' into handshakee
>  - Merge branch 'master' into handshakee
>  - Merge branch 'master' into handshakee
>  - Added comment
>  - Small review updates
>  - Merge branch 'master' into handshakee
>  - Merge branch 'master' into handshakee
>  - Small update
>  - Merge branch 'master' into handshakee
>  - ... and 1 more: https://git.openjdk.java.net/jdk/compare/c00b4e29...7a85a13a

Hi David,

> On 7/06/2021 1:49 pm, Patricio Chilano Mateo wrote:
> 
> > On Fri, 4 Jun 2021 22:46:08 GMT, Robbin Ehn <rehn at openjdk.org> wrote:
> > > > As long as we have a release() fence after the handshake closure the reordering of decrementing _pending_threads and removing the operation from the queue shouldn't matter since the target doesn't read _pending_threads.
> > > 
> > > 
> > > "release operation: no reads or writes in the current thread can be reordered after this store."
> > > But stores happened after this store may be reordered before. Which means the removal might happen even before the handshake operation was executed.
> > > So we need to stop the store ("the relaxed CAS") from floating above the dec.
> > > Dec with rel+acq gives: "No memory reads or writes in the current thread can be reordered before or after this store."
> > 
> > 
> > By release() fence I was referring to the definition in orderAccess (LoadStore | StoreStore), sorry for the confusion,
> 
> Those "definitions" are not definitions - the semantics of combining
> loadstore|storestore are sufficient to implement release but not
> necessary. If all you have is loadstore and storestore style barriers
> then that combination is what you need to implement release semantics.
> But release semantics (and acquire semantics) are weaker than that.
> 
I'm talking about standalone fences not the definition of release semantics which is weaker as you pointed out. The equivalent of storeFence from Unsafe.java or std::atomic_thread_fence(std::memory_order_release) with C++ atomics.

Patricio

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

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