RFR: 8293613: need to properly handle and hide tmp VTMS transitions [v3]

[Serguei Spitsyn]

On Mon, 19 Sep 2022 20:41:50 GMT, Chris Plummer <cjplummer at openjdk.org> wrote:

>> Serguei Spitsyn has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   fixed typo in VirtualThread.c
>
> src/hotspot/share/runtime/javaThread.cpp line 1174:
> 
>> 1172: #if INCLUDE_JVMTI
>> 1173:   // Suspending a JavaThread in VTMS transition or disabling VTMS transitions can cause deadlocks.
>> 1174:   assert(!is_in_non_tmp_VTMS_transition(), "no suspend allowed in VTMS transition");
> 
> IMHO, a non tmp VTMS transition should be considered a type of VTMS transition, so the assert check here does not really match the assert text. Also see my related comments below on naming and use of these flags and APIs.

Thanks. Accepted.

> src/hotspot/share/runtime/javaThread.hpp line 650:
> 
>> 648:   void set_is_in_VTMS_transition(bool val);
>> 649:   bool is_in_tmp_VTMS_transition() const         { return _is_in_tmp_VTMS_transition; }
>> 650:   void toggle_is_in_tmp_VTMS_transition()        { _is_in_tmp_VTMS_transition = !_is_in_tmp_VTMS_transition; };
> 
> The naming of the flags does not match up with the naming of the APIs, which is confusing.  An API named  is_in_VTMS_transition() should return _is_in_VTMS_transition. I think part of problem is that _is_in_VTMS_transition is not a superset of _is_in_tmp_VTMS_transition. The flags are never both set true, although both can be false. Maybe this should be changed to make it an invariant that if _is_in_tmp_VTMS_transit is true, then _is_in_tmp_VTMS_transition is true.

I agree, this naming is not good. However, I was reluctant to do required renaming because it can potentially impact many spots and make review harder. Let me think a little bit more on this.

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

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