Re: [PATCH] Fix race between sem_post and semaphore destruction [BZ #12674]

[Torvald Riegel <triegel at redhat dot com>]

On Thu, 2014-05-22 at 16:22 -0400, Rich Felker wrote:
> On Thu, May 22, 2014 at 10:10:06PM +0200, Torvald Riegel wrote:
> > > - sem_getvalue is patched to lie about the actual value if it sees the
> > >   -1 and return 0.
> > 
> > OT: We should add a note there about having to clarify the ordering
> > guarantees that this gives.  This is effectively an mo_relaxed load, so
> > very weak ordering guarantees; OTOH, POSIX seems to want very strong
> > ordering guarantees for most of its sync operations.  So, I think we
> > either need to clarify in POSIX or make this at least an acquire load or
> > such.
> 
> AFAIK POSIX does not impose any synchronization on sem_getvalue.

Standard says:
  The updated value represents an actual semaphore value that occurred
  at some unspecified time during the call, but it need not be the
  actual value of the semaphore when it is returned to the calling
  process.

The second part is obvious (in an asynchronous system).  The first part
makes this, effectively, linearizable without saying that explicitly.
That's not what mo_relaxed guarantees.

> > > diff --git a/nptl/sysdeps/unix/sysv/linux/sem_post.c b/nptl/sysdeps/unix/sysv/linux/sem_post.c
> > > index 4906adf..0ff1699 100644
> > > --- a/nptl/sysdeps/unix/sysv/linux/sem_post.c
> > > +++ b/nptl/sysdeps/unix/sysv/linux/sem_post.c
> > > @@ -30,24 +30,35 @@ int
> > >  __new_sem_post (sem_t *sem)
> > >  {
> > >    struct new_sem *isem = (struct new_sem *) sem;
> > > +  int incr, is_private = isem->private;
> > >  
> > >    __typeof (isem->value) cur;
> > >    do
> > >      {
> > >        cur = isem->value;
> > > +      incr = 1 + (cur < 0);
> > >        if (isem->value == SEM_VALUE_MAX)
> > >  	{
> > >  	  __set_errno (EOVERFLOW);
> > >  	  return -1;
> > >  	}
> > >      }
> > > -  while (atomic_compare_and_exchange_bool_rel (&isem->value, cur + 1, cur));
> > > +  while (atomic_compare_and_exchange_bool_rel (&isem->value, cur + incr, cur));
> > >  
> > >    atomic_full_barrier ();
> > 
> > Why do you still need the full barrier?  AFAICS, it was necessary before
> > because of the Dekker-like synchronization between nwaiters and value --
> > but you've changed that (or not?).
> 
> Per POSIX, all functions which synchronize memory are full barriers.

But we haven't implemented them this way (e.g., see the existing
sem_wait, or mutexes), and that's The Right Thing To Do IMO.  Yes, I
know this should be taken to the Austin group, but it's a nontrivial
thing because it would, I believe, require them to adopt a more complex
memory model and/or at least spell out in detail what's actually
required.