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

[Robbin Ehn]

On Thu, 3 Jun 2021 16:56:07 GMT, Patricio Chilano Mateo <pchilanomate at openjdk.org> wrote:

>> Maybe it should be added in the comment that the fence is also used to prevent removing of the operation to float above marking the operation as completed so we keep the invariant: "If the handshake operation is not completed it must be on at least one queue".
>
> I still think we can keep the previous memory_order_release order, but if this stricter order makes it easier to read for others then I'm okay.

Added comment.

But that would be relying on implementation details of a generic queue.
This would be a perfectly fine change-set:

diff --git a/src/hotspot/share/utilities/filterQueue.inline.hpp b/src/hotspot/share/utilities/filterQueue.inline.hpp
index 2fa199f2b04..ecb13e57db7 100644
--- a/src/hotspot/share/utilities/filterQueue.inline.hpp
+++ b/src/hotspot/share/utilities/filterQueue.inline.hpp
@@ -94 +94 @@ E FilterQueue<E>::pop(MATCH_FUNC& match_func) {
-      if (Atomic::cmpxchg(&_first, match, match->_next) == match) {
+      if (Atomic::cmpxchg(&_first, match, match->_next, memory_order_relaxed) == match) {



CAS will happened after we load _first, since it's loaded with acquire, but _both_ operation may float above decrement!

Now handshake will break because it would be using a undocumented side affect.
If we are removing any ordering, we should remove it where it is not needed. (the CAS)
Not the other way around!

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

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