RFR: 8306767: Concurrent repacking of extra data in MethodData is potentially unsafe [v16]

[Roland Westrelin]

On Thu, 4 Jan 2024 12:52:52 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> As explained in a [comment below](https://github.com/openjdk/jdk/pull/16840#issuecomment-1833529561), we have to ensure that reading/writing/cleaning the extra data all needs to be guarded by the `extra_data_lock`, and that no safepoint should happen while holding that lock, so that the lock is not broken.
>> 
>> I introduced `check_extra_data_locked`, where I check that we hold the lock, and if we are a java thread (only those ever safepoint), that we currently are in a `NoSafepointVerifier` scope, hence we verify that no safepoint will be taken.
>> 
>> I placed `check_extra_data_locked` in all the places where we access the extra data, and then placed locks and no-safepoint-verifiers at the call-site of those places.
>> 
>> I also needed to change the rank of `extra_data_lock` to `nosafepoint` and set the `Mutex::_no_safepoint_check_flag` when taking the lock. Otherwise I could not take the lock from a VM thread.
>> 
>> **Complications with ttyl**
>> There were a few places in printing code, where did `ttyLocker ttyl;`, and then in that scope we would access the extra data. Now that I introduced locking with `extra_data_lock`, this ran into asserts which check the lock ranks: `ttyl` has a very low rank, and `extra_data_lock` a rather high lock. Hence, we cannot lock `extra_data_lock` inside a `ttyl` scope.
>> 
>> If we were to simply remove the `ttyl` locking, then the many print lines inside that scope might be interrupted and another thread can insert other printing in between. To avoid that, I now first buffer all lines in a `stringStream`, and then print that buffered stream to `tty` all at once, which means no other printing can be injected in between.
>> 
>> **Testing**
>> Testing: tier1-3 and stress.
>
> Emanuel Peter has updated the pull request incrementally with one additional commit since the last revision:
> 
>   fixed typo

src/hotspot/share/ci/ciMethodData.cpp line 96:

> 94:     // a safepoint. We temporarily release the lock and allow
> 95:     // safepoints, and revert that at the end of the scope:
> 96:     MutexUnlocker mu(_mdo->extra_data_lock(), Mutex::_no_safepoint_check_flag);

Why is this safe?

src/hotspot/share/ci/ciMethodData.cpp line 135:

> 133: 
> 134:   // Lock to read ProfileData, and ensure lock is not unintentionally broken by a safepoint
> 135:   MutexLocker ml(mdo->extra_data_lock(), Mutex::_no_safepoint_check_flag);

Is there anyway to have MutexLocker take care of verifying that the there's no safepoint? It would be nice to replace:


MutexLocker ml();
NoSafepointVerifier no_safepoint;


by: 


MutexLocker ml();


only.

src/hotspot/share/runtime/deoptimization.cpp line 2406:

> 2404:         reprofile = true;
> 2405:       }
> 2406: 

Why is it safe to move this here?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16840#discussion_r1448886184
PR Review Comment: https://git.openjdk.org/jdk/pull/16840#discussion_r1448883447
PR Review Comment: https://git.openjdk.org/jdk/pull/16840#discussion_r1448873612