RFR(s): 8210303: VM_HandshakeAllThreads fails assert with "failed: blocked and not walkable"

[Robbin Ehn]

On 04/10/2018 09:18, David Holmes wrote:
> +1

Thanks David!

/Robbin

> 
> Thanks for explaining about is_ext_suspended().
> 
> David
> 
> On 4/10/2018 2:42 AM, Daniel D. Daugherty wrote:
>> On 10/3/18 7:42 AM, Robbin Ehn wrote:
>>> Hi all,
>>>
>>> Notice that I missed the extended suspend case:
>>> http://cr.openjdk.java.net/~rehn/8210303/v2/full/
>>
>> src/hotspot/share/runtime/handshake.cpp
>>      Don't forget to update copyright before pushing.
>>
>>      L351:   if (target->is_ext_suspended()) {
>>      L352:     return true;
>>          The handshake can only be processed for a JavaThread with
>>          is_ext_suspended() == true when the Threads_lock is held.
>>          So you need:
>>
>>          assert(Threads_lock->owned_by_self(), "Not holding Threads_lock.");
>>
>>          at the top of the function (like you have in
>>          HandshakeState::vmthread_can_process_handshake()).
>>
>>          You should also add a comment above L352:
>>
>>            // An externally suspended thread cannot be resumed while the
>>            // Threads_lock is held so it is safe.
>>
>>      L354:   switch(target->thread_state()) {
>>          Nit - please add space before '('
>>
>> Thumbs up! I don't need to see a new webrev for the above tweaks.
>>
>> Dan
>>
>>
>>> http://cr.openjdk.java.net/~rehn/8210303/v2/inc/
>>>
>>> Re-running sanity with t1-5 on this.
>>>
>>> Thanks, Robbin
>>>
>>> On 10/3/18 1:10 PM, Robbin Ehn wrote:
>>>> Hi all, please review.
>>>>
>>>> VM thread checks if it can processes a handshake for a JavaThread. That
>>>> check will only return a stable value if the VM thread holds the handshake
>>>> semaphore (or at safepoint). To avoid an unnecessary grabbing of the semaphore
>>>> just to release it, the VM thread do an early check to see if there is any 
>>>> point
>>>> to do the stable check. But the method 
>>>> SafepointSynchronize::safepoint_safe() is
>>>> not suppose to handle unstable checks. This can causes a false positive from an
>>>> assert in safepoint_safe().
>>>>
>>>> This change-set adds a local function for doing the unstable check without 
>>>> asserts. I do not want to expose a generic method for doing unstable 
>>>> safepoint safe test.
>>>>
>>>> Since asserts are not in release builds, there is no indication of a bug in JDK
>>>> 11. But since 11 is a LTS, this should also be considered for back-porting.
>>>>
>>>> Note, in JDK 11 only ZGC uses handshakes, previously releases have no users 
>>>> of handshakes.
>>>>
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~rehn/8210303/webrev/index.html
>>>>
>>>> Bug:
>>>> https://bugs.openjdk.java.net/browse/JDK-8210303
>>>>
>>>> I could not reproduce it, sanity with t1-3 + handshake tests.
>>>>
>>>> Thanks, Robbin
>>