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

[Ivan Gerasimov]

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

Please find the updated webrev:
http://cr.openjdk.java.net/~igerasim/8213383/01/webrev/

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