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

[Torvald Riegel <triegel at redhat dot com>]

On Wed, 2015-06-17 at 15:46 -0700, Roland McGrath wrote:
> > Waiting with absolute or relative timeouts is split into separate
> > functions.  This allows for removing a few cases of code duplication in
> > pthreads code, which uses absolute timeouts; also, it allows us to put
> > platform-specific code to go from an absolute to a relative timeout into
> > the platform-specific futex abstractions.  The latter is done by adding
> > lll_futex_abstimed_wait.  I expect that we will refactor this later on,
> > depending on how we do the lll_ parts.
> 
> I don't understand the motivation for adding lll_futex_abstimed_wait now
> at all.  Its only users are in futex-internal.h implementations, which
> are already OS-specific.  So why make the new files have identical
> copies of the wrappers around the implementations living in the moribund
> files?  What is the downside to simply having each futex-internal.h's
> futex_abstimed_wait{,_cancelable} be the real implementation?

There's no downside, it's simply a matter of which incremental steps we
take, and how we organize the work.  Remember that futex-internal.h
wasn't OS-specific before.  There are three options now that it is:
(1) Merge in all of lowlevellock-futex.h into futex-internal.h.
lowlevellock-futex.h has to still remain until we moved over all the
uses (lowlevellock itself is the largest and will take more time).
Thus, quite a bit of code duplication until we'd finished all of this.
(2) Move just lll_futex_abstimed_wait into futex-internal.h.  Then we
have a mix of lll_futex_* usage and direct futex syscall / NaCl uses in
futex-internal.h
(3) Do as we have now, and merge + remove lowlevellock-futex.h later on.

I didn't like option (2) too much because of the mixed merge / futex
usage, and thought (3) would be a cleaner intermediate step.  That's why
I picked that option.  I don't mind doing (2) now, or even (1), though
-- I think it's mostly a question of which steps / churn we prefer.

> > There are separate versions for both Linux and NaCl; while they
> > currently differ only slightly, my expectation is that the separate
> > versions of lowlevellock-futex.h will eventually be merged into
> > futex-internal.h when we get to move the lll_ functions over to the new
> > futex API.
> 
> This is not just an expectation, it's the core plan and the whole effort
> would be pointless if we failed to actually do this in the future.

That's what I'd think as well, but I we have no patches for this yet, so
I didn't want to make foregone conclusions ... :)

> > The sanity checks regarding whether shared futexes are supported abort
> > instead of returning errors because POSIX error specs don't really
> > consider that there could be no support for shared futexes.  Aborting is
> > better than returning an unspecified error or an error specified for a
> > different condition; only NaCl has no support for shared futexes.
> 
> This is not the right behavior.  It is indeed improper to use errno code
> E in function F for condition X when POSIX specifies that F returns E
> for condition Y.  It is also improper to use errno code E in function F
> for condition X when POSIX specifies errno code E2 for condition X in
> function F.  But it is entirely proper to return an errno code that
> POSIX does not specify for a given function to diagnose a condition that
> POSIX does not specify shall or may be diagnosed (see 2.3 Error Numbers).

That's what POSIX allows, I agreed.  But I thought that your motivation
was to make existing software robust on NaCl.  If you add a new error
code, how much of the existing software do you think will be prepared to
handle it sensibly, or check for it at all?  I don't remember seeing any
code that would do something useful on errors not specified by POSIX
(e.g., an "else { puts("unkown error"); exit(...); }" everywhere...). 

Thus, if a new error code is simply most likely to be not acted upon,
that's not better than failing fast, and in a way that can't simply be
ignored by the program.

> For pthread_*attr_setpshared, the sensible thing to do for an
> unsupported (but valid) value is to return ENOTSUP.  That's what these
> functions should do for PTHREAD_PROCESS_SHARED on NaCl.

If you think that's helpful for NaCl, we can do that.  Can you please
provide the documentation patch that mentions this additional error
condition on NaCl, or do you want it to remain undocumented?  (This
should be documented as a NaCl-only error condition, IMO.)

> Conversely,
> it's entirely reasonable not to account for any possibility of
> PTHREAD_PROCESS_PRIVATE not being supported, because to request that
> is to request the default behavior.
> 
> > Interacting with futex words requires atomic accesses, which isn't done
> > by most of glibc's current futex callers. [...]
> 
> I certainly concur with leaving this until later.  What I think will be
> reasonable to do eventually is to use the <stdatomic.h> type names in
> all our internal interfaces (probably only atomic_int or atomic_uint
> should be used for futex stuff).  When building with older compilers
> that don't have those, our own internal headers can typedef the ones
> we use to the simple types (or perhaps volatile-qualified ones?).

Joseph commented already on potential practical issues with that
(although I think we may be able to solve them or have them not trigger
in practice).

Annotating the types used for atomic accesses is something I considered.
We could do it for data not exposed to users (e.g., on internal
interfaces as you say), but then we have this weird (IMHO) mix of some
data being atomic-typed and some not.  This would mean that a variable
not having an atomic type wouldn't be sufficient to infer that it
doesn't need atomic accesses.

The compromise that I thought would be useful as a first step was to
require explicitly atomic accesses (through atomic_*) for all data that
needs it, and don't change the types for now.  We want to have the
explicit accesses anyway to have explicit MO choices as I mentioned
elsewhere in the thread, so doing that is something we'd keep doing
anyway.
Changing the types of (some of) all atomically accessed data later on
would be a fairly mechanical change, I suppose.

Nonetheless, if there is a preference in the project to use atomic types
where possible right from the start, I wouldn't be opposed to that.

> > Roland, okay for NaCl?  I decided to not try to "optimize" the
> > shared/private setting at data structure initialization time because I
> > didn't see a good way to specify the error conditions for the futex_*
> > functions then: We do want those to sanity check shared/private and not
> > just rely on the shared/private initialization to do the right thing --
> > but if we do that, we can as well transform private into shared if
> > that's actually necessary.
> 
> I don't follow your logic here.  Why do you think we want any
> argument-validity checks applied inside internal interfaces?  It's
> reasonable enough to have asserts inside the futex-internal.h
> functions if you feel like it.

Yes, the sanity checks I was talking about are assertions, not checks
that alter the conditions under which the futex wrappers return to the
caller.  Carlos specifically requested that the futex API calls abort
when the futex syscall returns an error that can only happen if glibc or
the program are buggy.

> Those can only fire when other libc
> code has a bug or there is memory clobberation.  Those are both cases
> where it's nice to fail catastrophically rather than mysteriously, but
> also both cases where we don't spend extra effort to diagnose the
> (supposedly) impossible situations and certainly where we never
> propagate such an error back to the user as a return or errno value.

Yeah, no (new) errors.  That's what I meant to refer to when speaking
about "error conditions".

But I think it's worth distinguishing between private and shared.  I
suppose we agree that private must always be supported, potentially
through the implementation picking shared instead (though that would be
unlikely in practice).

What the existing code does is to select SHARED early if PRIVATE isn't
natively supported (but that's not the case anymore neither for Linux
nor NaCl).  We can expect that all callers do that, but if we then add
an assertion (private != PRIVATE), we as well do this inside of this
hypothetical futex function:  if (private == PRIVATE) private = SHARED;
That's what I meant when saying that if we do an assertion, we can do
the conversion as well in this case.

SHARED is different, because we don't expect that all platforms support
it.  Therefore, the patch has futex_supports_shared, and if that returns
false, futex_* can assert that private != SHARED.
This makes sense because SHARED isn't the common case, so handling it
asymmetrically works well.

Does that explain my reasoning?

> What I have in mind is (names and signatures are straw men):
> 
> * In pthread_*attr_setpshared:
> 	error = futex_pshared (pshared, &iattr->pshared);
> 	if (__glibc_unlikely (error))
> 	  return error;
>   This transforms the public ABI values PTHREAD_PROCESS_{PRIVATE,SHARED}
>   in PSHARED into the internal form in IATTR->pshared.

PTHREAD_PROCESS_* is used by *_setpshared, but not by semaphore.
If we transform into the internal form, we need to have another function
that transforms back for _getpshared, so it's better to do the
transformation when initializing using the attributes.

Then the check at attr setting time would be:
  if (pshared == PTHREAD_PROCESS_SHARED && !futex_supports_shared ())
    return ENOTSUP;
and when using the attr it would be:
  foo = iattr->pshared == PTHREAD_PROCESS_SHARED ? FUTEX_SHARED :
                                                   FUTEX_PRIVATE;
(i.e., same as in the patch).

The only substantial benefit I can see is to have the return of ENOTSUP
be part of a NaCl sysdep.  Having a futex_check_shared that does the
check above would be good for that.

Anyway, I don't care strongly about that.  We're talking about 4
occurrences of setpshared.  Which option do you want to have?

> On Linux the
>   internal form would be FUTEX_PRIVATE_FLAG or 0, to be simply OR'd
>   into the operation in the syscall.  On NaCl the internal form would
>   just be some constant value, only stored at all to avoid confusing
>   valgrind et al.

This abstraction already exists.  See FUTEX_PRIVATE and FUTEX_SHARED.

> * In *_init:
> 	object->private = iattr->pshared;
>   There's really no need for checks here, since it's undefined
>   behavior to pass a bogus attributes object.  

Yes, except that I'm not sure having FUTEX_PRIVATE/SHARED in
iattr->pshared already is a real improvement.

>   It can't be literally that alone since there has to be a default for
>   a null attributes pointer.  If it seems worthwhile, we could have it
>   sanity check the value in the attributes struct so as to detect
>   clobbered or uninitialized attributes; that would be done by a
>   futex-internal.h function to transfer attributes-field format into
>   object-field format (it would be up to the particular implementation
>   to decide if those are the same or different).  But I don't see a
>   real need for that.
> 
> * In the actual uses:
> 	futex_wake (&obj->futex, nw, obj->private);
>   That is, nothing special.  All the checking was done before.
>   If the futex-internal.h implementation cares to do redundant checks
>   for clobberation, it can assert inside there.  There should be no
>   provision for returning errors to indicate the private field has an
>   invalid value.

No errors, but assertion in case of NaCl when it's passed FUTEX_SHARED.
(I mean, I don't really care about NaCl details at that level, but
that's the scheme I had in mind.)

Have you looked at the actual patch yet?