RFR(XS): 8207334: VM times out in VM_HandshakeAllThreads::doit() with RunThese30M
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
Thu Aug 16 12:02:04 UTC 2018
On 8/16/18 5:12 AM, Robbin Ehn wrote:
> Same mail with corrected subject.
>
> On 08/16/2018 11:03 AM, Robbin Ehn wrote:
>> Hi Dan, I took your suggestion on comment, thanks!
>>
>> Full patch looks like:
>>
>> diff -r 6bb7b8d0da76 src/hotspot/share/runtime/handshake.cpp
>> --- a/src/hotspot/share/runtime/handshake.cpp Thu Aug 16 09:46:09
>> 2018 +0200
>> +++ b/src/hotspot/share/runtime/handshake.cpp Thu Aug 16 10:56:27
>> 2018 +0200
>> @@ -340,3 +340,9 @@
>> bool HandshakeState::vmthread_can_process_handshake(JavaThread*
>> target) {
>> - return SafepointSynchronize::safepoint_safe(target,
>> target->thread_state());
>> + // SafepointSynchronize::safepoint_safe() does not consider an
>> externally
>> + // suspended thread to be safe. However, this function must be
>> called with
>> + // the Threads_lock held so an externally suspended thread cannot be
>> + // resumed thus it is safe.
>> + assert(Threads_lock->owned_by_self(), "Not holding Threads_lock.");
There's a assert_lock_strong(Threads_lock); that does the same thing,
which I like better. The name of the assert could be better though,
like assert_lock_held().
But Dan and David are still your reviewers (don't add me).
thanks,
Coleen
>> + return SafepointSynchronize::safepoint_safe(target,
>> target->thread_state()) ||
>> + target->is_ext_suspended();
>> }
>
> David, you also think this comment is better?
>
> /Robbin
>
>>
>> /Robbin
>>
>> On 08/15/2018 10:34 PM, Daniel D. Daugherty wrote:
>>> On 8/15/18 4:21 PM, Robbin Ehn wrote:
>>>> Hi Dan, thanks,
>>>>
>>>> On 2018-08-15 22:02, Daniel D. Daugherty wrote:
>>>>> So if HandshakeState::vmthread_can_process_handshake() is only
>>>>> called by the VMThread while at a safepoint, then the target
>>>>> JavaThread cannot be externally resumed while that function is
>>>>> running so this new check:
>>>>
>>>> No we are not at safepoint, this method is never called at safepoint.
>>>
>>> Okay I definitely misinterpreted what you were saying in this
>>> email thread... let's try this again.
>>>
>>>
>>>> All
>>>> other threads should be considered running. Therefore I need to
>>>> stop resuming targeted suspended thread while VM thread is
>>>> executing the handshake on behalf of the suspended thread.
>>>>
>>>> Does my comment/assert make more sense in the light of that?
>>>
>>> Hmmm.... Here's your comment:
>>>
>>> + // We must block the S/R protocol to resume a thread while the vm
>>> thread
>>> + // is executing the operation, by holding the Threads_lock.
>>> + // This is problematic, since we now can't remove the
>>> Threads_lock in handshakes.
>>>
>>> Mostly, but I still suggest a rewrite:
>>>
>>> // SafepointSynchronize::safepoint_safe() does not consider an
>>> externally
>>> // suspended thread to be safe. However, this function must be
>>> called with
>>> // the Threads_lock held so an externally suspended thread cannot be
>>> // resumed thus it is safe.
>>>
>>> Of course, this comment assumes that the Threads_lock is held in the
>>> caller
>>> over the appropriate interval of code...
>>>
>>> Dan
>>>
>>>
>>>
>>>>
>>>> /Robbin
>>>>
>>>>>
>>>>> + return SafepointSynchronize::safepoint_safe(target,
>>>>> target->thread_state()) ||
>>>>> + target->is_ext_suspended();
>>>>>
>>>>> is okay because that externally suspended thread is also safepoint
>>>>> safe.
>>>>>
>>>>> This assert:
>>>>>
>>>>> + assert(Threads_lock->owned_by_self(), "Not holding
>>>>> Threads_lock.");
>>>>>
>>>>> doesn't quite match your intent. If
>>>>> HandshakeState::vmthread_can_process_handshake()
>>>>> should only be called by the VMThread at a safepoint, then a
>>>>> different
>>>>> check would be better:
>>>>>
>>>>> assert(SafepointSynchronize::is_at_safepoint() &&
>>>>> Thread::current()->is_VM_thread(),
>>>>> "must be VMThread at a safepoint");
>>>>>
>>>>> Perhaps this for your comment:
>>>>>
>>>>> // SafepointSynchronize::safepoint_safe() does not consider an
>>>>> externally
>>>>> // suspended thread to be safe. However, if we are at a
>>>>> safepoint, then
>>>>> // the externally suspended thread cannot be resumed so it is
>>>>> safe.
>>>>>
>>>>> Dan
>>>>>
>>>>>
>>>>>>
>>>>>> Thanks, Robbin
>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>> David
>>>>>>>
>>>>>
>>>>
>>>
More information about the hotspot-runtime-dev
mailing list