[PATCH] New condvar implementation that provides stronger ordering guarantees.

[Florian Weimer]

On 05/26/2016 03:21 PM, Torvald Riegel wrote:
> +	  /* We have to conservatively undo our potential mistake of stealing
> +	     a signal.  We can stop trying to do that when the current G1
> +	     changes because other spinning waiters will notice this too and
> +	     __condvar_quiesce_and_switch_g1 has checked that there are no
> +	     futex waiters anymore before switching G1.
> +	     Relaxed MO is fine for the __g1_start load because we need to
> +	     merely be able to observe this fact and not have to observe
> +	     something else as well.
> +	     ??? Would it help to spin for a little while to see whether the
> +	     current G1 gets closed?  This might be worthwhile if the group is
> +	     small or close to being closed.  */
> +	  unsigned int s = atomic_load_relaxed (cond->__data.__g_signals + g);
> +	  while (__condvar_load_g1_start_relaxed (cond) == g1_start)
> +	    {
> +	      /* Try to add a signal.  We don't need to acquire the lock
> +		 because at worst we can cause a spurious wake-up.  If the
> +		 group is in the process of being closed (LSB is true), this
> +		 has an effect similar to us adding a signal.  */
> +	      if (((s & 1) != 0)
> +		  || atomic_compare_exchange_weak_relaxed (
> +		       cond->__data.__g_signals + g, &s, s + 2))
> +		{
> +		  /* If we added a signal, we also need to add a wake-up on
> +		     the futex.  We also need to do that if we skipped adding
> +		     a signal because the group is being closed because
> +		     while __condvar_quiesce_and_switch_g1 could have closed
> +		     the group, it might stil be waiting for futex waiters to
> +		     leave (and one of those waiters might be the one we stole
> +		     the signal from, which cause it to block using the
> +		     futex).  */
> +		  futex_wake (cond->__data.__g_signals + g, 1, private);
> +		  break;
> +		}
> +	      /* TODO Back off.  */
> +	    }

I don't quite understand the exit condition for this while loop.  How do 
we know that __g1_start will change (again) in a timely fashion?  Or 
alternatively, why is the s variable not reloaded?

I haven't really reviewed this patch.  One small nit: GNU style does not 
use a parenthesis at the end of a line, as in (among others):

+  uint64_t r = __condvar_fetch_add_64_relaxed (

+  return __condvar_load_64_relaxed (


There are some magic numbers 2/4/8, which might better use symbolic 
constants.

pthread_cond_common.c should be a header file (pthread_cond_common.h).

Thanks,
Florian