This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] Fix race between sem_post and semaphore destruction [BZ #12674]
- From: Torvald Riegel <triegel at redhat dot com>
- To: Rich Felker <dalias at libc dot org>
- Cc: Siddhesh Poyarekar <siddhesh at redhat dot com>, libc-alpha at sourceware dot org, carlos at redhat dot com
- Date: Thu, 22 May 2014 22:34:12 +0200
- Subject: Re: [PATCH] Fix race between sem_post and semaphore destruction [BZ #12674]
- Authentication-results: sourceware.org; auth=none
- References: <20140521110711 dot GA3598 at spoyarek dot pnq dot redhat dot com> <1400789406 dot 6984 dot 134 dot camel at triegel dot csb> <20140522202224 dot GJ507 at brightrain dot aerifal dot cx>
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.