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

[Ivan Gerasimov]

Thanks everyone for the reviews!

With kind regards,

Ivan


On 11/10/18 2:39 PM, serguei.spitsyn at oracle.com wrote:
> 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!
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>
>
>

-- 
With kind regards,
Ivan Gerasimov