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

[Robbin Ehn]

Hi Dan,

On 03/10/2018 18:42, 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.

Fixed!

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

Since this method is allowed to produce false positives I extend the comment to:
+  // An externally suspended thread cannot be resumed while the
+  // Threads_lock is held so it is safe.
+  // Note that this method is allowed to produce false positives.
+  assert(Threads_lock->owned_by_self(), "Not holding Threads_lock.");

Hope that's fine.

> 
>      L354:   switch(target->thread_state()) {
>          Nit - please add space before '('

Fixed!

> 
> Thumbs up! I don't need to see a new webrev for the above tweaks.

Thanks!

Passed t1-5.

/Robbin

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