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

[David Holmes]

+1

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
>