RFR: 8364819: Post-integration cleanups for JDK-8359820 [v9]
Albert Mingkun Yang
ayang at openjdk.org
Tue Aug 19 10:56:44 UTC 2025
On Tue, 19 Aug 2025 07:34:30 GMT, Anton Artemov <duke at openjdk.org> wrote:
>> Hi, please consider the following changes:
>>
>> This is a cleanup after https://github.com/openjdk/jdk/pull/26309
>>
>> 1) `_handshake_timed_out_thread `and `_safepoint_timed_out_thread` are now `Thread*` and not `intptr_t`, no conversions `p2i <-> i2p` needed.
>>
>> 2) Added a missed brace in the error message.
>>
>> 3) Updates are done with `Atomic::replace_if_null()` to address possible multiple updates and visibility among all threads.
>>
>> Trivial change.
>
> Anton Artemov has updated the pull request incrementally with one additional commit since the last revision:
>
> 8364819: Addressed reviewer's comments
> Arguably, as stated much much earlier, we don't need any explicit memory ordering here in practice as the signal implementation must provide its own ordering guarantees to actually function - and there is no way a compiler would ever re-order the store with the pthread_kill call!
I see. I think I misunderstood the problem a bit. Hopefully, I get it this time -- the non-inline function-call should prevent compiler-reordering and the system-call inside `pthread_kill` should prevent CPU-reordering. The only thing needed here is to ensure the store-to-the-shared-var becomes visible on other CPUs, e.g. not hidden inside store-buffer. Conceptually, `pthread_kill` performs a "store" as well, so the invariant is that the two stores should be ordered. x64 uses FIFO on store-buffer, so everything is fine. OTOH, aarch64 needs some amending to enforce FIFO for visibility of these two stores.
`Atomic::replace_if_null` should be enough to provide the ordering.
-------------
Marked as reviewed by ayang (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/26656#pullrequestreview-3131772906
More information about the hotspot-dev
mailing list