RFR: 8303908: Add missing check in VTMS_transition_disable_for_all() for suspend mode

[Patricio Chilano Mateo]

On Fri, 10 Mar 2023 04:34:13 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> Please review this small fix. A suspender is a JvmtiVTMSTransitionDisabler monopolist, meaning VTMS_transition_disable_for_all() should not return while there is any active jvmtiVTMSTransitionDisabler. The code though is checking for active "all-disablers" but it's missing the check for active "single disablers".
>> I attached a simple reproducer to the bug which I used to test the patch. Not sure if it was worth adding a test so the patch contains just the fix.
>> 
>> Thanks,
>> Patricio
>
> src/hotspot/share/prims/jvmtiThreadState.cpp line 372:
> 
>> 370:   java_lang_Thread::dec_VTMS_transition_disable_count(vth());
>> 371:   Atomic::dec(&_VTMS_transition_disable_for_one_count);
>> 372:   if (_VTMS_transition_disable_for_one_count == 0 || _is_SR) {
> 
> Sorry I don't understand why this `_is_SR` check was removed.  I admit I can't really figure out what this field means anyway, but there is nothing in the issue description that suggests this also needs changing - and it is now different to `VTMS_transition_enable_for_all`.

A JvmtiVTMSTransitionDisabler instance that is a "single disabler" only blocks other virtual threads trying to transition or JvmtiVTMSTransitionDisabler monopolists. Both of them will check for _VTMS_transition_disable_for_one_count (the JvmtiVTMSTransitionDisabler monopolist was missing that check) so just checking when that counter is zero is enough. In fact, for a "single disabler" _is_SR is always false so that check wasn't doing anything. Yes, this is not actually needed for the fix, but when looking at which condition we use to wait and which one to notify I caught this, sorry for not explaining that part.

And looking closer at VTMS_transition_enable_for_all() now I see the check for _is_SR is not doing anything too, because if _VTMS_transition_disable_for_all_count was not zero after the decrement then this can't be a JvmtiVTMSTransitionDisabler monopolist, i.e _is_SR will be false. When a monopolist is running all other "disable all" JvmtiVTMSTransitionDisabler instances if any will be waiting in the first "while (_SR_mode)" loop in VTMS_transition_disable_for_all(), so _VTMS_transition_disable_for_all_count will be one through the monopolist run. So this should be an assert after the decrement: assert(!_is_SR || _VTMS_transition_disable_for_all_count == 0, "").

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

PR: https://git.openjdk.org/jdk/pull/12956