RFR 8232733: Remove need to grab Threads_lock while processing handshakes

[Patricio Chilano]

Hi Dan,

On 12/10/19 12:29 PM, Daniel D. Daugherty wrote:
> 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.
Sure, I'll do that.

>> 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.
Great, thanks for digging up the history behind the use of the Threads_lock!

> 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.
Thanks for reviewing this Dan. I'll run those tests before pushing.

Patricio
> Dan
>
>
>>
>> Thanks,
>>
>> Patricio
>