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

[David Holmes]

On Fri, 10 Mar 2023 17:06:58 GMT, Patricio Chilano Mateo <pchilanomate at openjdk.org> wrote:

>> 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, "").

Thanks for clarifying - I was puzzled by the way `is_SR` was being used.

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

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