Question about GetObjectMonitorUsage() JVMTI function

[David Holmes]

On 15/06/2020 4:02 pm, Yasumasa Suenaga wrote:
> Hi David,
> 
> On 2020/06/15 14:15, David Holmes wrote:
>> Hi Yasumasa,
>>
>> On 15/06/2020 2:49 pm, Yasumasa Suenaga wrote:
>>> Hi all,
>>>
>>> I wonder why JvmtiEnvBase::get_object_monitor_usage() (implementation 
>>> of GetObjectMonitorUsage()) does not perform at safepoint.
>>
>> GetObjectMonitorUsage will use a safepoint if the target is not 
>> suspended:
>>
>> jvmtiError
>> JvmtiEnv::GetObjectMonitorUsage(jobject object, jvmtiMonitorUsage* 
>> info_ptr) {
>>    JavaThread* calling_thread = JavaThread::current();
>>    jvmtiError err = get_object_monitor_usage(calling_thread, object, 
>> info_ptr);
>>    if (err == JVMTI_ERROR_THREAD_NOT_SUSPENDED) {
>>      // Some of the critical threads were not suspended. go to a 
>> safepoint and try again
>>      VM_GetObjectMonitorUsage op(this, calling_thread, object, info_ptr);
>>      VMThread::execute(&op);
>>      err = op.result();
>>    }
>>    return err;
>> } /* end GetObject */
> 
> I saw this code, so I guess there are some cases when 
> JVMTI_ERROR_THREAD_NOT_SUSPENDED is not returned from 
> get_object_monitor_usage().
> 
> 
>>> Monitor owner would be acquired from monitor object at first [1], but 
>>> it would perform concurrently.
>>> If owner thread is not suspended, the owner might be changed to 
>>> others in subsequent code.
>>>
>>> For example, the owner might release the monitor before [2].
>>
>> The expectation is that when we find an owner thread it is either 
>> suspended or not. If it is suspended then it cannot release the 
>> monitor. If it is not suspended we detect that and redo the whole 
>> query at a safepoint.
> 
> I think the owner thread might resume unfortunately after suspending check.

Yes you are right. I was thinking resuming also required a safepoint but 
it only requires the Threads_lock. So yes the code is wrong.

> JavaThread::is_ext_suspend_completed() is used to check thread state, it 
> returns `true` when the thread is sleeping [3], or when it performs in 
> native [4].

Sure but if the thread is actually suspended it can't continue execution 
in the VM or in Java code.

> 
>> This appears to be an optimisation for the assumed common case where 
>> threads are first suspended and then the monitors are queried.
> 
> I agree with this, but I could find out it from JVMTI spec - it just 
> says "Get information about the object's monitor."

Yes it was just an implementation optimisation, nothing to do with the spec.

> GetObjectMonitorUsage() might return incorrect information in some case.
> 
> It starts with finding owner thread, but the owner might be just before 
> wakeup.
> So I think it is more safe if GetObjectMonitorUsage() is called at 
> safepoint in any case.

Except we're moving away from safepoints to using Handshakes, so this 
particular operation will require that the apparent owner is 
Handshake-safe (by entering a handshake with it) before querying the 
monitor. This would still be preferable I think to always using a 
safepoint for the entire operation.

Cheers,
David
-----

> 
> Thanks,
> 
> Yasumasa
> 
> 
> [3] 
> http://hg.openjdk.java.net/jdk/jdk/file/76a17c8143d8/src/hotspot/share/runtime/thread.cpp#l671 
> 
> [4] 
> http://hg.openjdk.java.net/jdk/jdk/file/76a17c8143d8/src/hotspot/share/runtime/thread.cpp#l684 
> 
> 
> 
>> However there is still a potential bug as the thread reported as the 
>> owner may not be suspended at the time we first see it, and may 
>> release the monitor, but then it may get suspended before we call:
>>
>>   owning_thread = 
>> Threads::owning_thread_from_monitor_owner(tlh.list(), owner);
>>
>> and so we think it is still the monitor owner and proceed to query the 
>> monitor information in a racy way. This can't happen when suspension 
>> itself requires a safepoint as the current thread won't go to that 
>> safepoint during this code. However, if suspension is implemented via 
>> a direct handshake with the target thread then we have a problem.
>>
>> David
>> -----
>>
>>>
>>>
>>> Thanks,
>>>
>>> Yasumasa
>>>
>>>
>>> [1] 
>>> http://hg.openjdk.java.net/jdk/jdk/file/76a17c8143d8/src/hotspot/share/prims/jvmtiEnvBase.cpp#l973 
>>>
>>> [2] 
>>> http://hg.openjdk.java.net/jdk/jdk/file/76a17c8143d8/src/hotspot/share/prims/jvmtiEnvBase.cpp#l996 
>>>