RFR 8232733: Remove need to grab Threads_lock while processing handshakes

[Robbin Ehn]

Hi,

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

Exactly what was needed to remove Threads_lock was unclear to me at this point.
Hence the comment is vague.

/Robbin

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