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

[Daniel D.Daugherty]

On Mon, 7 Jun 2021 09:07:09 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 ten additional commits since the last revision:
> 
>  - 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
>  - Fix

I did a crawl thru review of the v05 full webrev.
Thumbs up.

There are a few copyright years that need to be updated.

src/hotspot/share/utilities/filterQueue.hpp line 68:

> 66:   // exclusive to other contains and pops calls.
> 67:   template <typename MATCH_FUNC>
> 68:   bool contains(MATCH_FUNC& match_func);

Need to update the copyright for this file.

src/hotspot/share/utilities/filterQueue.inline.hpp line 114:

> 112:   } while (true);
> 113: }
> 114: 

Need to update the copyright for this file.

test/hotspot/gtest/utilities/test_filterQueue.cpp line 58:

> 56: static void is_empty(FilterQueue<uintptr_t>& queue) {
> 57:   EXPECT_EQ(queue.is_empty(), true) << "Must be empty.";
> 58:   EXPECT_EQ(queue.contains(match_1), false) << "Must be empty.";

Need to update the copyright for this file.

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

Marked as reviewed by dcubed (Reviewer).

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