This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


On Tue, 2016-06-14 at 16:44 +0200, Florian Weimer wrote:
> 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?

The loop above is just the retry loop should the CAS fail.  If
cond->__data.__g1_start changes, we're done because somebody closed the
group.  If it doesn't change, then we'll keep trying to add a signal
until the group gets closed.  This will eventually succeed (or the group
will be closed, in which case we do wake up and break out of the loop).

> Or 
> alternatively, why is the s variable not reloaded?

It is.  C11's compare_exchange updates the expected value if the it the
comparison failed (that's why we pass in the address of s).

> I haven't really reviewed this patch.

Thanks for taking a look nonetheless.

> 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 (

What's the expected solution, parenthesis on the next line?

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

I considered this, but kind of preferred the short numbers.  If we
prefer constants with names, I'll change that.

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

There are many cases of C files being included across all of glibc.  Are
there precise rules for when to pick .h vs. .c?  pthread_cond_common.c
doesn't have just declarations or small helper functions, so .c seemd to
be the right choice.



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]