Re: Is this pthread_cond_wait() race possible

[Torvald Riegel <triegel at redhat dot com>]

On Thu, 2017-06-08 at 09:39 +0200, Sebastian Andrzej Siewior wrote:
> On 2017-06-07 21:41:38 [+0200], Torvald Riegel wrote:
> > On Wed, 2017-06-07 at 18:48 +0200, Sebastian Andrzej Siewior wrote:
> > > Hi Torvald,
> > > 
> > > I've been staring at the new pthread_cond_* code. Could you please tell
> > > if this race possible (two wait threads looking at the same __g_signals
> > > value while a third thread signals a wake before going to futex_wait()):
> > > 
> > > T1                               T2                       T3
> > > 
> > > __pthread_cond_wait_common()                            
> > >  __condvar_fetch_add_wseq_acquire (cond, 2);
> > >  __pthread_mutex_unlock_usercnt();                       
> > > 
> > >                                 __pthread_cond_wait_common()
> > >                                  __condvar_fetch_add_wseq_acquire (cond, 2);
> > >                                  __pthread_mutex_unlock_usercnt();
> > > 
> > >                                                         __pthread_cond_signal()
> > >                                                           if ((cond->__data.__g_size[g1] != 0) …
> > > 
> > >                                                            /* Add a signal. */
> > >                                                            atomic_fetch_add_relaxed (cond->__data.__g_signals + g1, 2);
> > > 
> > >  signals = atomic_load_acquire (cond->__data.__g_signals + g);
> > >                                  signals = atomic_load_acquire (cond->__data.__g_signals + g);
> > > 
> > >   signals = 2                     signals = 2
> > > 
> > >           /* If there is an available signal, don't block.  */
> > >   if (signals != 0)               if (signals != 0)
> > >       break;                         break;
> > > 
> > >   while (!atomic_compare_exchange_weak_acquire (cond->__data.__g_signals + g,
> > >                                                  &signals, signals - 2));
> > >   signals = 0
> > >                                   while (!atomic_compare_exchange_weak_acquire (cond->__data.__g_signals + g,
> > >                                                 &signals, signals - 2));
> > >                                    signals = (int) -2
> > 
> > The second CAS would expect a value of 2 in *(cond->__data.__g_signals +
> > g).  Because the first CAS succeeded in your example, the second CAS
> > will fail, and T2 will update it's view of __g_signals and store that in
> > signals.
> 
> So T1 stores 2 -> 0 in __g_signals + g as expected.
> T2 tries to store 2 -> 0 in __g_signals + g which fails because
> __g_signals + g != signals (0 != 2). However
> atomic_compare_exchange_weak_acquire() will update signals to 0 right?
> So the second iteration of the while loop will update __g_signals + g to
> 0 -> -2. T2 won't loop until __g_signals becomes 2 again, right?

No, the second iteration of the outer do-while loop will just run the
inner while loop; the inner loop doesn't exit until signals is non-zero
(or the group is closed or a timeout occurs, but then the CAS isn't
executed).