Question about GetObjectMonitorUsage() JVMTI function

[Yasumasa Suenaga]

On 2020/06/15 22:38, Daniel D. Daugherty wrote:
> 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.

I don't think so.

For example, JVMTI agent might attempt to get monitor owner from sleeping thread or in native (e.g. during socket operation) thread, and it might resume during GetObjectMonitorUsage() call.
Agent code can (should) not control application threads as an observer.

IMHO GetObjectMonitorUsage() should perform at safepoint or should start direct handshake immediately after getting owner thread from monitor.


Yasumasa


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