RFR 8232733: Remove need to grab Threads_lock while processing handshakes

[Patricio Chilano]

Hi David,

On 12/10/19 5:01 AM, David Holmes wrote:
> Hi Patricio,
>
> On 10/12/2019 9:12 am, Patricio Chilano wrote:
>> 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 for the history! My concern is that 8167108 is the change that 
> added the comment:
>
> +      // We need to re-think this with SMR ThreadsList.
> +      // There is an assumption in the code that the Threads_lock 
> should be
> +      // locked during certain phases.
>
> so it would be good to here from those involved to know why that 
> comment was added.
Are you okay with this patch?


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