RFR (XS) 8213383 : Wrap up pthread_cond_wait into a loop to workaround potential spurious wakeups

[serguei.spitsyn at oracle.com]

Hi Ivan,

Looks good.

Thanks,
Serguei


On 11/8/18 17:00, David Holmes wrote:
> Hi Ivan,
>
> On 9/11/2018 10:40 AM, Ivan Gerasimov wrote:
>> Thanks Serguei!
>>
>>
>> On 11/6/18 1:01 PM, serguei.spitsyn at oracle.com wrote:
>>> Hi Ivan,
>>>
>>> The fix looks good to me.
>>> It is also possible to rewrite it like this:
>>>    while (jvm_signal_installing && tid != pthread_self()) {
>>>      pthread_cond_wait(&cond, &mutex);
>>>    }
>>>
>> That's true.
>> I think that having the nested loop shows more clear for what exact 
>> condition we're waiting in the cond_wait.
>>
>>> We should not care about wasting on execution of the condition
>>> tid != pthread_self() as spurious wakeups have to be rare.
>>>
>> Also I just noticed that the comparison tid != pthread_self() is not 
>> quite accurate, according to the documentation.
>> Here's the except from the man 3 pthread_equals():
>> """
>> The  pthread_equal()  function  is  necessary  because  thread IDs 
>> should be considered opaque: there is no
>> portable way for applications to directly compare two pthread_t values.
>> """
>
> Yes that's quite true. We're guilty of doing this in the VM too ... we 
> treat pthread_t as a pointer type in places (for printing primarily)
>
>> Please find the updated webrev:
>> http://cr.openjdk.java.net/~igerasim/8213383/01/webrev/
>
> Looks good to me.
>
> Thanks,
> David
>
>> With kind regards,
>> Ivan
>>
>>> Thanks,
>>> Serguei
>>>
>>>
>>> On 11/5/18 5:20 PM, Ivan Gerasimov wrote:
>>>> Thank you Dean!
>>>>
>>>>
>>>> On 11/5/18 4:09 PM, dean.long at oracle.com wrote:
>>>>> Looks good, but do you want to use a while loop instead of 
>>>>> if-do-while?
>>>>>
>>>>>      if (tid != pthread_self()) {
>>>>>        while (jvm_signal_installing) {
>>>>>          pthread_cond_wait(&cond, &mutex);
>>>>>        }
>>>>>      }
>>>>>
>>>> This code will be executed before tid is initialized (see 
>>>> JVM_begin_signal_setting()), so jvm_signal_installing has to be 
>>>> tested first.
>>>>
>>>> With kind regards,
>>>> Ivan
>>>>
>>>>> dl
>>>>>
>>>>> On 11/5/18 11:59 AM, Ivan Gerasimov wrote:
>>>>>> Hello!
>>>>>>
>>>>>> In src/java.base/unix/native/libjsig/jsig.c:
>>>>>>
>>>>>> signal_lock() makes all the thread that are not jvm wait while 
>>>>>> the later is installing signal handlers.
>>>>>> This is done via a call to pthread_cond_wait().
>>>>>> Spurious wakeups from pthread_cond_wait() are allowed, so it 
>>>>>> needs to be wrapped up into a loop.
>>>>>>
>>>>>> Would you please help review the trivial fix?
>>>>>> Control build and testing of tier1,2,3 went fine on all supported 
>>>>>> platforms.
>>>>>>
>>>>>> BUGURL: https://bugs.openjdk.java.net/browse/JDK-8213383
>>>>>> WEBREV: http://cr.openjdk.java.net/~igerasim/8213383/00/webrev/
>>>>>>
>>>>>> Thanks!
>>>>>>
>>>>>
>>>>
>>>
>>>
>>