Question about GetObjectMonitorUsage() JVMTI function

[Daniel D. Daugherty]

On 6/15/20 3:26 AM, David Holmes wrote:
> 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.

Which code is wrong?

Yes, a rogue resume can happen when the GetObjectMonitorUsage() caller
has started the process of gathering the information while not at a
safepoint. Thus the information returned by GetObjectMonitorUsage()
might be stale, but that's a bug in the agent code.

Dan


>
>
>> 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 
>>>>