[PATCH v4] Add and use new glibc-internal futex API.

[Torvald Riegel]

On Wed, 2015-06-24 at 16:22 -0700, Roland McGrath wrote:
> > @@ -987,7 +988,8 @@ setxid_mark_thread (struct xid_command *cmdp, struct pthread *t)
> >    if (t->setxid_futex == -1
> >        && ! atomic_compare_and_exchange_bool_acq (&t->setxid_futex, -2, -1))
> >      do
> > -      lll_futex_wait (&t->setxid_futex, -2, LLL_PRIVATE);
> > +      futex_wait_simple ((unsigned int *) &t->setxid_futex, -2,
> > +			 FUTEX_PRIVATE);
> >      while (t->setxid_futex == -2);
> 
> All these casts are ugly.  Where possible, we should just change the types
> of the fields to unsigned int.  For setxid_futex, it appears to use exactly
> four values (0, 1, -1, -2) and the particular choice of values does not
> seem to matter (except perhaps 0 as initialization state).  So really these
> should be in an enum rather than hard-coded magic numbers.
> 
> Cleaning that up could be done either before or after the futex API
> changes.  But I tend to think that if we have these explicit casts around,
> we'll fail to remove them all later when we should.

I've removed the casts.  The clean-up to enums should be done with the
patch that uses atomics to access setxid_futex.

(Note that the lack of atomic accesses to futex words is a recurring
problem, and not one I intended to solve with this patch.)

> > --- a/nptl/cancellation.c
> > +++ b/nptl/cancellation.c
> > @@ -19,6 +19,7 @@
> >  #include <setjmp.h>
> >  #include <stdlib.h>
> >  #include "pthreadP.h"
> > +#include <futex-internal.h>
> >  
> >  
> >  /* The next two functions are similar to pthread_setcanceltype() but
> > @@ -93,7 +94,7 @@ __pthread_disable_asynccancel (int oldtype)
> >    while (__builtin_expect ((newval & (CANCELING_BITMASK | CANCELED_BITMASK))
> >  			   == CANCELING_BITMASK, 0))
> >      {
> > -      lll_futex_wait (&self->cancelhandling, newval, LLL_PRIVATE);
> > +      futex_wait_simple (&self->cancelhandling, newval, FUTEX_PRIVATE);
> 
> This fails to build because of int vs unsigned int.  x86_64 has assembly
> code for this file, so you didn't test it in your build.  Please do at
> least an i686 build too to catch more cases.
> 
> Here too, the field really should just be unsigned int rather than adding
> casts everywhere.

ISTM the cancelhandling change to unsigned int requires more pervasive
code changes than in the patch you posted (see my reply to that).
I've kept the casts for now, with the expectation that we'll change to
unsigned int in the patch that uses atomics for all accesses to
cancelhandling.

> > +  int futex_private = (lll_private == LLL_PRIVATE)
> > +		      ? FUTEX_PRIVATE : FUTEX_SHARED;
> 
> There's no need for parens around a comparison like that.
> The operator precedence of comparison operators for ?: is well-known.
> However, you should use parens differently here so that the second
> line's indentation is done right (and stays that way):
> 
>   int futex_private = (lll_private == LLL_PRIVATE
> 		       ? FUTEX_PRIVATE : FUTEX_SHARED);

Changed to not use parens except where it's a multi-line case like
above.

> > @@ -31,6 +32,10 @@ pthread_mutexattr_setpshared (attr, pshared)
> >        && __builtin_expect (pshared != PTHREAD_PROCESS_SHARED, 0))
> >      return EINVAL;
> >  
> > +  int err = futex_supports_pshared (pshared);
> > +  if (err != 0)
> > +    return err;
> 
> It wouldn't hurt to consolidate the EINVAL check and futex_supports_pshared
> into one function that these several places could all call instead of
> duplicating the code.

futex_supports_pshared now checks whether pshared is one of the two
allowed values, too.

> > +    /* No other errors are documented at this time.  */
> > +    default:
> > +      abort ();
> 
> I think all these abort calls should be __libc_fatal, or perhaps
> __libc_message that shows the actual unexpected error code value.
> (Actually, they should all use a common convenience function for that.)

Added a convenience function called futex_fatal_error.

> > +/* See sysdeps/nptl/futex-internal.h for details.  */
> > +static __always_inline void
> > +futex_wait_simple (unsigned int *futex_word, unsigned int expected,
> > +		   int private)
> > +{
> > +  ignore_value (futex_wait (futex_word, expected, private));
> > +}
> 
> This obviously is not actually system-dependent and does not need to be
> copied.  But I'm not sure where we'd put it.  Perhaps there should be a
> common header that defines this and the convenience function for the abort
> cases (and maybe other things).  It could be a wrapper header that includes
> the sysdeps header.  It could be a little header that each variant
> includes.  It could be just in sysdeps/nptl/futex-internal.h and all the
> variants include that first; that might be best, in that the common file
> pre-declares all the inlines and thus we'd get compile-time checking that
> each variant is actually using all the correct signatures in its
> implementation.

I'm now including sysdeps/nptl/futex-internal.h from the
platform-specific futex-internal.h files.  futex_wait_simple goes there
as well as futex_fatal_error.

> >  #define AIO_MISC_NOTIFY(waitlist) \
> >    do {									      \
> >      if (*waitlist->counterp > 0 && --*waitlist->counterp == 0)		      \
> > -      lll_futex_wake (waitlist->counterp, 1, LLL_PRIVATE);		      \
> > +      futex_wake ((unsigned int *) waitlist->counterp, 1, FUTEX_PRIVATE);     \
> >    } while (0)
> 
> Another field whose type should change.

Can't do that right now because it still needs to cast away the volatile
that's still present after your recent change that makes it volatile
unsigned int.  I don't want to just remove the volatile either, because
this way make the synchronization work (it really should use atomics
though).  This can be cleaned up when we transform this to using
atomics.

> > +/* futex operations for glibc-internal use.  Stub version.
> > +   Copyright (C) 2014-2015 Free Software Foundation, Inc.
> 
> If you didn't copy any of this content from other files that had a 2014
> copyright, then a new file should just have a 2015 copyright.

I had posted this last year, and I remember that this is considered the
"publication" of the code.

> > +/* Returns 0 if PSHARED is supported, which can be either
> > +   PTHREAD_PROCESS_PRIVATE or PTHREAD_PROCESS_SHARED.  Otherwise, returns
> > +   ENOTSUP.  */
> > +static __always_inline int
> > +futex_supports_pshared (int pshared);
> 
> If the interface is defined this tightly, then I don't see any benefit over
> making it bool(void).

Which would have been roughly the interface I had in a previous version
of this patch :)

Anyway, it now also checks that pshared is one of
PTHREAD_PROCESS_PRIVATE or PTHREAD_PROCESS_SHARED too (see above), so
returning an actual error code is fine now.

Updated patch is attached.  Once this is OK, I'll send an updated patch
that includes the (updated) sparc changes so that Dave can test (per
Joseph's request, to allow for bisect-ability, we can't have a separate
patch for the sparc bits).

Tested on x86_64-linux.  No regressions except that I am getting a
check-local-headers failure:
*** $(common-objpfx)stdio-common/scanf15.o:
uses /usr/include/bits/syscall.h
*** /usr/include/bits/syscall.h: uses /usr/include/bits/syscall.h:
*** $(common-objpfx)stdio-common/scanf17.o:
uses /usr/include/bits/syscall.h
*** /usr/include/bits/syscall.h: uses /usr/include/bits/syscall.h:
Any advice on how to fix that?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: futex-api.patch
Type: text/x-patch
Size: 69776 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/libc-alpha/attachments/20150701/7b90345f/attachment.bin>