RFR: 8364819: Post-integration cleanups for JDK-8359820 [v5]

[Albert Mingkun Yang]

On Thu, 7 Aug 2025 07:34:50 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. 
>> 
>> Trivial change.
>
> Anton Artemov has updated the pull request incrementally with one additional commit since the last revision:
> 
>   8364819: Addressed reviewer's comments

> I should have picked this up previously but volatile generally serves no purpose for inter-thread consistency.

True, but I thought we denote a variable `volatile` if it can be accessed by multiple threads, mostly for documentation purpose. In this case, it should be e.g. `Thread* volatile VMError::_handshake_timed_out_thread`.

> (I note that pthread_kill is not specified as a memory synchronizing operation, but I strongly suspect it has to have its own internal memory barriers that we would be piggy-backing on.)

How about making the ordering explicit by using `OrderAccess::acquire/release` in reader/writer side? Sth like:

Writer side:

    VMError::set_handshake_timed_out_thread(target);
    OrderAccess::release();
    if (os::signal_thread(target, SIGILL, "cannot be handshaked")) {


Reader side:

      if (_siginfo != nullptr && os::signal_sent_by_kill(_siginfo)) {
        OrderAccess::acquire();
        if (get_handshake_timed_out_thread() == _thread) {


> Here we have a very simple situation:
>    Thread 1: set field; signal target thread
>    Target thread: receive signal; read field

I think for this to work, we need ordering btw set-field and sending-signal and btw reading-signal and read-field. Therefore, the acquire/release "operation" should be performed on the "signal" variable. (Ofc, such "signal" variable is not directly accessible, hence, the suggested `OrderAccess` above.)

I have to say it's indeed a bit odd that "setters" (e.g. `set_handshake_timed_out_thread`) have `fence()` there for no obvious reasons.

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

PR Comment: https://git.openjdk.org/jdk/pull/26656#issuecomment-3174462135