Re: [PATCH v3] Add and use new glibc-internal futex API.

[Roland McGrath <roland at hack dot frob dot com>]

> @@ -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.

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

> +  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);


> --- a/nptl/pthread_create.c
> +++ b/nptl/pthread_create.c
> @@ -414,7 +415,7 @@ START_THREAD_DEFN
>  	  this->__list.__next = NULL;
>  
>  	  atomic_or (&this->__lock, FUTEX_OWNER_DIED);
> -	  lll_futex_wake (&this->__lock, 1, /* XYZ */ LLL_SHARED);
> +	  futex_wake (&this->__lock, 1, /* XYZ */ FUTEX_SHARED);

Needs a cast.  Again, the field should probably change.  But as this one is
a type defined in a public header, we should probably just do casts and let
things lie for now.

> @@ -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.

> @@ -100,8 +100,10 @@ __pthread_once_slow (pthread_once_t *once_control, void (*init_routine) (void))
>  	     is set and __PTHREAD_ONCE_DONE is not.  */
>  	  if (val == newval)
>  	    {
> -	      /* Same generation, some other thread was faster. Wait.  */
> -	      lll_futex_wait (once_control, newval, LLL_PRIVATE);
> +	      /* Same generation, some other thread was faster.  Wait and
> +		 retry.  */
> +	      futex_wait_simple ((unsigned int *)once_control,
> +				 (unsigned int) newval, FUTEX_PRIVATE);

Space after cast.

> --- a/sysdeps/nacl/exit-thread.h
> +++ b/sysdeps/nacl/exit-thread.h
> @@ -18,7 +18,7 @@
>  
>  #include <assert.h>
>  #include <atomic.h>
> -#include <lowlevellock.h>
> +#include <futex-internal.h>
>  #include <nacl-interfaces.h>
>  #include <nptl/pthreadP.h>
>  
> @@ -64,7 +64,7 @@ __exit_thread (void)
>        assert (NACL_EXITING_TID > 0);
>  
>        atomic_store_relaxed (&pd->tid, NACL_EXITING_TID);
> -      lll_futex_wake (&pd->tid, 1, LLL_PRIVATE);
> +      futex_wake (&pd->tid, 1, FUTEX_PRIVATE);

Needs cast.

> --- /dev/null
> +++ b/sysdeps/nacl/futex-internal.h
> @@ -0,0 +1,257 @@
[...]
> +/* FUTEX_SHARED is not yet supported.  */
> +static __always_inline int
> +futex_supports_pshared (int pshared)
> +{
> +  if (pshared == PTHREADS_PROCESS_SHARED)
> +    return ENOTSUP;

Typo: s/PTHREADS_PROCESS_SHARED/PTHREAD_PROCESS_SHARED/

> +    /* 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.)

> +/* 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.

> +/* See sysdeps/nptl/futex-internal.h for details.  */
> +static __always_inline int
> +futex_reltimed_wait (unsigned int* futex_word, unsigned int expected,
> +		     const struct timespec* reltime, int private)

The * should be next to the variable name, not next to the type.
There are other instances of this in the file too.

> +  int err = __nacl_irt_futex.futex_wait_abs ((volatile int *) futex_word,
> +      expected, abstime);

The second line of arguments should be indented to just past the open paren.
There are other instances of this in the file too.

>  #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.

> +/* 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.

> +#include <errno.h>
> +#include <lowlevellock-futex.h>
> +#include <sys/time.h>
> +#include <stdlib.h>
> +#include <stdbool.h>
> +#include <nptl/pthreadP.h>

The only #include's the stub header should have are the ones necessary for
the API it defines.  I think that's just <stdbool.h> (for bool) and
<sys/time.h> (for struct timespec).

> +/* This file defines futex operations used internally in glibc.  A futex
> +   consists of the so-called futex word in userspace, which is of type int

But in this API it's of type 'unsigned int'.

> +/* Defined this way for interoperability with lowlevellock.
> +   FUTEX_PRIVATE must be zero because the initializers for pthread_mutex_t,
> +   pthread_rwlock_t, and pthread_cond_t initialize the respective field of
> +   those structures to zero, and we want FUTEX_PRIVATE to be the default.  */
> +#define FUTEX_PRIVATE LLL_PRIVATE
> +#define FUTEX_SHARED  LLL_SHARED

Encode these requirements in the code:

#if FUTEX_PRIVATE != 0
# error blah blah
#endif

> +/* 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).

> +/* Like futex_wait but cancelable.  */
> +static __always_inline int
> +futex_wait_cancelable (unsigned int *futex_word, unsigned int expected,
> +		       int private);

If "cancelable" means "is a cancellation point" in POSIX terminology,
then just write that.

> +/* Like futex_wait, but will eventually time out (i.e., stop being
> +   blocked) after the duration of time provided (i.e., RELTIME) has
> +   passed.  The caller must provide a normalized RELTIME.  RELTIME can also
> +   equal NULL, in which case this function behaves equivalent to futex_wait.
> +
> +   Returns 0 if woken by a futex operation or spuriously.  (Note that due to
> +   the POSIX requirements mentioned above, we need to conservatively assume
> +   that unrelated futex_wake operations could wake this futex; it is easiest
> +   to just be prepared for spurious wake-ups.)
> +   Returns EAGAIN if the futex word did not match the expected value.
> +   Returns EINTR if waiting was interrupted by a signal.
> +   Returns ETIMEDOUT if the timeout expired.

Perhaps it would be better just to say the return values are all the same
ones as for futex_wait, plus the ETIMEDOUT case.  The same text repeated
in several comments is always likely to bit-rot later.

sysdeps/unix/sysv/linux/futex-internal.h has several of the trivial style
issues I cited above for the other variants.  I won't call them out in
detail.

> --- a/sysdeps/unix/sysv/linux/lowlevellock-futex.h
> +++ b/sysdeps/unix/sysv/linux/lowlevellock-futex.h
> @@ -22,6 +22,7 @@
>  #ifndef __ASSEMBLER__
>  #include <sysdep.h>
>  #include <tls.h>
> +#include <sys/time.h>
>  #include <kernel-features.h>
>  #endif

I don't see the need for this.


I will look into changing some internal fields from int to unsigned int
so as to reduce the number of casts required in calls to futex-internal.h
functions.


Thanks,
Roland