On Wed, 14 Apr 2021 16:27:10 GMT, Daniel D. Daugherty <dcu...@openjdk.org> wrote:
>> Robbin Ehn has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 13 commits: >> >> - Merge branch 'master' into SuspendInHandshake >> - Review fixes 4 >> - Fixed flag undef dep + spelling error >> - Obsolete unused flags >> - Review fixes 3 >> - Merge branch 'master' into SuspendInHandshake >> - Review fixes 2 >> - White space fixes >> - Merge branch 'master' into SuspendInHandshake >> - Review fixes >> - ... and 3 more: >> https://git.openjdk.java.net/jdk/compare/b224b566...27bf041c > > src/hotspot/share/runtime/handshake.cpp line 415: > >> 413: // Adds are done lock free and so is arming. >> 414: // Calling this method with lock held is considered an error. >> 415: assert(!_lock.owned_by_self(), "Lock should not be held"); > > I originally added this comment inside the "resolved" conversation and that > kept > this comment from showing up. Let's try it as a new comment. > > Doesn't that mean the comment on L415 needs updating? > Should it be something like: > > // Synchronous adds are done lock free and so is arming, but some > // asynchronous adds are done when we already hold the lock. > > I'm not sure about the "some asynchronous adds" part... > @dcubed-ojdk The mutex is unrelated to adds/inserts. Adds/inserts to the queue can always be done regardless of which locks a thread may or may not have. The reason for not allowing inserts while holding the HandshakeState lock was to eliminate that from the table, since it have other implications. As @pchilano found we needed to reverse the order in should_process() to make it work (which I missed). So now when we have looked at the case, it is okay to do it. Meaning that you can add a handshake to yourself from another handshake regardless of it is sync or async. ------------- PR: https://git.openjdk.java.net/jdk/pull/3191
