RFR 8232733: Remove need to grab Threads_lock while processing handshakes

[Patricio Chilano]

Hi David,

On 12/9/19 12:12 AM, David Holmes wrote:
> Hi Patricio,
>
> On 7/12/2019 5:40 am, 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.
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8232733
>> Webrev: http://cr.openjdk.java.net/~pchilanomate/8232733/v01/webrev/
>
> src/hotspot/share/runtime/handshake.cpp
>
>  151       // We need to re-think this with SMR ThreadsList.
>  152       // There is an assumption in the code that the Threads_lock 
> should be
>  153       // locked during certain phases.
>  154       {
>  155         MutexLocker ml(Threads_lock);
>
> The way I read that comment is that the SMR code expects the 
> Threads_lock to be held during certain phases of the handshake code. 
> Is that not what it meant?
>
> Ditto for where we use the JavaThreadIteratorWithHandle. If we didn't 
> need the Threads_lock while using the JavaThreadIteratorWithHandle why 
> were we taking it? Were we just overly cautious/conservative?
The use of the Threads_lock was there from the very first commit when 
handshakes were introduced (8189941), before using 
ThreadsListHandle/JavaThreadIteratorWithHandle. My understanding based 
on having read the code and talking with Robbin is that the Threads_lock 
was there for two reasons. One reason was so that the JavaThread would 
not be freed while reading it (although looking at that first commit it 
seems that between releasing the Threads_lock after setting the 
handshake and acquiring it again before processing it, the JavaThread 
could have already processed the handshake and exited). The other reason 
was so that a suspended JavaThread would not be resumed in the middle of 
the handshake.
Now, the second commit to handshake.cpp (8167108) introduced the use of 
SMR lists, removing the need to acquire the Threads_lock for setting the 
handshake but not for processing it, since we still needed it to prevent 
a suspended JavaThread from being resumed.
Based on the last changes you made to suspend/resume code we know that 
even if a JavaThread is resumed it will still transition and stop for 
the handshake. So, more than conservative I would say it was a 
requirement back then but not anymore. Having said that it would be 
great If people that contributed to 8167108 can also review this patch 
and validate my claims : )

Thanks,

Patricio
> Thanks,
> David
>
>
>> Thanks,
>>
>> Patricio