RFR 8232733: Remove need to grab Threads_lock while processing handshakes

[Daniel D. Daugherty]

On 12/6/19 2:40 PM, Patricio Chilano wrote:
> Hi all,
>
> Please review the following patch that removes the requirement of 
> having to acquire the Threads_lock while processing handshakes. The 
> guarantee that the JavaThread being handshaked will not exit and its 
> memory be freed is already provided by using SMR ThreadsLists. With 
> respect to the comments that the Threads_lock is needed to avoid a 
> suspended JavaThread from being resumed, that will not be a problem 
> since the JavaThread will still make the proper transition after being 
> resumed and will block if needed for the handshake.
>
> Tested with mach5, tiers1-7.

Robbin has some handshake tests that stress suspend and thread exit paths:

test/hotspot/jtreg/runtime/handshake/HandshakeSuspendExitTest.java
test/hotspot/jtreg/runtime/handshake/HandshakeWalkExitTest.java
test/hotspot/jtreg/runtime/handshake/HandshakeWalkOneExitTest.java

I think running these in a loop for thousands of iterations would
be a good idea.

> Bug: https://bugs.openjdk.java.net/browse/JDK-8232733
> Webrev: http://cr.openjdk.java.net/~pchilanomate/8232733/v01/webrev/

src/hotspot/share/runtime/handshake.hpp
     No comments.

src/hotspot/share/runtime/handshake.cpp
     old L151:       // We need to re-think this with SMR ThreadsList.
     old L152:       // There is an assumption in the code that the 
Threads_lock should be
     old L153:       // locked during certain phases.
         This comment came from the Thread-SMR project (8167108). First
         appearance was in this webrev which came from the 2017.11.15
         rebase of the project done by Robbin:

http://cr.openjdk.java.net/~dcubed/8167108-webrev/jdk10-08-delta/src/hotspot/share/runtime/handshake.cpp.frames.html

         I tracked it back a little further to Robbin's original webrev
         that I used to update the Thread-SMR project repo:

http://rehn-ws.se.oracle.com/share/for_dan/smr-tlh-merge/full/webrev/src/hotspot/share/runtime/handshake.cpp.frames.html

         I checked my email archive and I can't find any discussion
         about the comment other than fixing some wording in it.

     old L206:           // We need to re-think this with SMR ThreadsList.
     old L207:           // There is an assumption in the code that the 
Threads_lock should
     old L208:           // be locked during certain phases.
         Same analysis as above.

     Now let's look at the Threads_lock grabs:

     old L135:     ThreadsListHandle tlh;
     old L155:         MutexLocker ml(Threads_lock);
     old L156:         by_vm_thread = 
_target->handshake_try_process_by_vmThread();
         Since the VMThread is executing this VMop not at a safepoint,
         prior to Thread-SMR we had to worry about a JavaThread exiting
         while the VMThread was checking to see if it could process the
         handshake on behalf of the JavaThread.

         With the ThreadsListHandle, all of the checks made in:

         _target->handshake_try_process_by_vmThread()
           HandshakeState::try_process_by_vmThread(JavaThread* target)
             possibly_vmthread_can_process_handshake(JavaThread* target)

         should be safe from a bad dereference without the Threads_lock.
         Yes, the JavaThread might exit, but the JavaThread data structure
         will hang around until all ThreadsListHandles that contain it are
         freed. So this code will be able to see the terminated state
         without risking a crash.

         We also had to worry about a suspended JavaThread being resumed
         and racing with the VMThread for processing the handshake request.
         With the Threads_lock being held, a target JavaThread could not
         be resumed so that potential race was also handled.

         It looks to me like the handshake code already handles a possible
         race between the VMThread and a JavaThread in handshake handling
         by claiming of the semaphore (via claim_handshake_for_vmthread).
         Only one thread can win that race...

         So the original worry must have actually been about a suspended
         JavaThread being seen as "safe" for the VMThread to handle its
         handshake only to have the JavaThread resumed and then run quickly
         to exit... In other words, I think the resume worry was a slight
         variation of the regular JavaThread exit worry. Again, with the
         ThreadsListHandle, I think that is no longer an issue.

     old L181:     JavaThreadIteratorWithHandle jtiwh;
     old L209:           jtiwh.rewind();
     old L210:           MutexLocker ml(Threads_lock);
     old L214:             if (thr->handshake_try_process_by_vmThread()) {
         Same analysis as for the previous Threads_lock grab:

         Again, with the JavaThreadIteratorWithHandle (which has a builtin
         ThreadsListHandle), I think that is no longer an issue.

src/hotspot/share/runtime/mutexLocker.hpp
     No comments.

src/hotspot/share/runtime/mutexLocker.cpp
     No comments.

src/hotspot/share/runtime/safepoint.cpp
     No comments.

src/hotspot/share/runtime/thread.hpp
     No comments.

src/hotspot/share/runtime/thread.cpp
     No comments.


Thumbs up!

Thanks for removing these two Threads_lock uses.

Dan


>
> Thanks,
>
> Patricio