[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-runtime-dev
Subject:    Re: RFR: 8266557: assert(SafepointMechanism::local_poll_armed(_handshakee)) failed: Must be
From:       Robbin Ehn <rehn () openjdk ! java ! net>
Date:       2021-05-28 7:45:33
Message-ID: 3m9fT7cUyrFRgwfTGc65DJBmwunAEHAHp02_FO8UK90=.84d37913-e9bf-4518-afb0-efdc0fbd00ed () github ! com
[Download RAW message or body]

> 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 four additional commits since the last \
revision:

 - Merge branch 'master' into handshakee
 - Small update
 - Merge branch 'master' into handshakee
 - Fix

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3973/files
  - new: https://git.openjdk.java.net/jdk/pull/3973/files/03bbb423..d5028125

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3973&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3973&range=01-02

  Stats: 37179 lines in 1546 files changed: 7523 ins; 26507 del; 3149 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3973.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3973/head:pull/3973

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


[prev in list] [next in list] [prev in thread] [next in thread]