[PATCH 1/2] Add futex wrappers with error checking

[Torvald Riegel]

On Thu, 2014-12-04 at 16:33 -0800, Roland McGrath wrote:
> I'm not entirely clear on why this is separate from the lll_futex_*
> layer rather than replacing it.  I understand the benefits of
> incremental change, of course.  Is that the only reason?

Incremental change is one reason.

The second is that I haven't looked through all the lowlevellock cases
yet, so didn't want to touch that; it seemed moving lll_futex* callers
over to futex* callers wouldn't be an issue later on.

Third, some of the lll_futex* definitions are in headers that are also
used from asm files; I guess that would mean I'd need to use macros
instead of C functions.

Fourth, I need some way to get to the arch-specific futex syscalls.  I
didn't know whether sysdeps/unix/sysv/linux/lowlevellock-futex.h would
work on every arch, so I just used what works for the locks.

> I don't
> think we want to have both layers as such in the long run,

Maybe not.  If we want to expose our own futex abstraction to users,
we'd need a separate version that does less of the error checking we do,
as there may be cases where certain errors would need to be handled
differently.  You point out something similar below; checking that the
kernel (or whatever below provides the futex functionality) didn't
return errors we haven't specified in our futex abstraction.

> and it's
> not clear to me what the end state of this cleanup will be in your
> vision.

I didn't think about clean-up as much.  What I wanted is something we
can use today to get the futex error handling correct in pthread_once
and the the semaphores I'm about to submit, for example.

> Perhaps we should do some more thorough design of the final
> internal API we want to have, and then figure out the incremental path
> to get there with bite-sized changes.

Fine with me.  From my perspective, it seemed best to start with an
abstraction with well-defined semantics (that's what I tried to do in
futex-internal.h at least), so that all clients of it are taken care of.
Any clean-up under the hood of it could be then done independently.

I think I have a pretty good understanding for what the futex semantics
of the abstraction that we use internally should be.  I don't have a
good feel for how to best clean up all the existing code we have related
to that.

> This is dovetailing (or colliding, to be pessimistic) with more
> cleanup and refactoring that I'm starting to do as part of my Native
> Client port.  So I'll start by just throwing out there all the issues
> I'm aware of off hand, in hopes that solving each might naturally be
> folded into what you're doing.
> 
> The internal API of lowlevellock-futex.h needs to be cleaned up and
> specified more thoroughly in a few ways.

Agreed.  That was my goal for futex-internal.h too.

> * FUTEX_PRIVATE_FLAG is not part of the stated API (as described in
>   the stub file, sysdeps/nptl/lowlevellock-futex.h).  But it is used
>   in "generic" NPTL code, with implicit assumptions about its
>   relationship to LLL_PRIVATE and LLL_SHARED.  What I'd like to see is
>   both use of FUTEX_PRIVATE_FLAG and __ASSUME_PRIVATE_FUTEX disappear
>   from generic code.

Agreed.  That's why I introduce FUTEX_PRIVATE and FUTEX_SHARED as
constants in my patch, and have the futex_private_if_supported helper.
The current code is often hard to follow, including when to xor
FUTEX_PRIVATE_FLAG and when not to.  Using just two constants everywhere
seems better.  And all the futex calls are on the slow path anyway.

> Instead, we can have some sysdeps-defined
>   inlines akin to your futex_private_if_supported, but both sides of
>   the coin: one for the cases like pthread_barrier_init and one for
>   the cases like pthread_barrier_wait.

I'm not sure we actually need something for pthread_barrier_wait.  All
uses of the private field are xor'd with FUTEX_PRIVATE_FLAG.  It seemed
to me that just having a properly set up value for the private field in
the first place would suffice.

> Ideally, this would also cover
>   detection of support in the opposite direction from what we've ever
>   dealt with before: when shared is the unavailable option, so that
>   pthread_barrierattr_setpshared et al can fail with ENOTSUP for any
>   request for pshared semantics.
> 
>   Today we actually do not have any configurations where the answer is
>   dynamic, though I'd like to support them for the future.  On Linux,
>   we always set __ASSUME_PRIVATE_FUTEX.  (Older Linux kernels did not
>   always support private, but shared can always stand in for private
>   so it's not a user-visible distinction.  Nowadays our minimum
>   supported kernel version is one that supports private.)  On NaCl,
>   all futexes are private and shared is just not available.  Hence I'd
>   like to make setpshared fail properly rather than lie.  But it's not
>   unlikely that at some point in the future, NaCl will support shared
>   and then it would be a dynamic check to determine whether it's
>   available or not.

Interesting.  I haven't thought about the case where shared is not
supported.

I suppose "dynamic" would still mean that this is stable throughout the
lifetime of the process?

>   I'm sure this can be done in a way that does not change the compiled
>   code at all for Linux.

Probably, but personally I wouldn't worry about that.  When we do the
transition to the new internal futex API, we'd change code anyway
because of adding more error checking and such.

Or are you worried about having to potentially test versioned code?

Also, I have the new semaphores I'm about to submit, have a new condvar
implementation that is also close to being finished, and rwlocks on my
todo list.  They would all be C11 atomics based, and use a futex API
with proper error checking.  So, there would be a lot of code change in
the major uses of futexes anyway (when ignoring mutexes).

> I'm even sure it can be done in a way that
>   would not change the compiled code for Linux cases without
>   __ASSUME_PRIVATE_FUTEX, if there still were such.  But I haven't
>   thought up the right API for that off hand.  And frankly, I get a
>   bit dizzy every time I try to think through all the XORs and which
>   places store the bit in which sense.

Yes, me too :)
I think the patch is correct for (the new) semaphores and pthread_once
at least -- but I haven't gone through all the other clients yet.

> * We haven't properly specified the exact types of pointer arguments
>   in lll_futex_* calls.  In NaCl these are implemented by eventually
>   calling actual C functions with similar signatures, as opposed to
>   many layers of macro turning into asm with operands that just have
>   to be pointer-sized.  Our uses are actually inconsistent about
>   whether it's an 'int *' or an 'unsigned int *' and about whether
>   it's volatile or not.

I also though about int vs. unsigned int for a while.  The Linux kernel
has int, but when I look at the synchronization code I'm using futexes
for, in most of the cases it's an unsigned you want to work with.
Therefore, I picked unsigned.

> So my build has lots of volatileness and
>   pointee signedness warnings that are not easily vanquished without
>   resorting to casts that could mask real bugs.

Nothing needs to be volatile there, IMO.  If anything, this should be an
atomic type.

> The Linux
>   implementations indirectly have casts that could indeed mask real
>   bugs.  It would be far better to have inlines with specific types
>   and clean up all our usage.

Agreed.

> * I really want to completely excise the inane "negated errno" return
>   value convention from all our internal APIs.  That is not even a
>   true Linuxism, it's a style copied from Linux kernel internals that
>   does not even map to what the user ABI for syscall errors is on all
>   machines.  All new or cleaned-up interfaces should just use the
>   straightforward POSIXy "errno or zero" convention instead.  (That's
>   what the underlying OS interface C functions in NaCl do, so today my
>   macros all do -function(...) and I get oodles of -Wunused-value
>   warnings for all the places we lack error checking today.)

Fine with me.
  
>   The only reason not to use that convention is if you needed some
>   content in the return value other than error indication.  The Linux
>   futex syscall interface does return such values (FUTEX_WAKE), but we
>   do not actually use them anywhere at all.

And in particular in FUTEX_WAKE, I see no error that we actually need to
return given the way we use futexes.  With things like the mutex
destruction issue, FUTEX_WAKE can really hit anything, reused memory,
unmapped memory, etc.
That's probably something to watch out for in the NaCl implementation
too.

> If we ever did need them,
>   I'd advocate for using out parameters in our internal APIs instead,
>   even if the Linux implementation transmutes part of the return value
>   space into the out parameter.  (Such layers will be all inlines
>   anyway, so it shouldn't even make a microoptimization difference.)

Fine with me.

> > It is implemented on top of lll_futex_*, so that we can expose the raw
> > futex to users via a syscall wrapper (or external futex_wait,...
> > functions), should we want to do that in the future.
> 
> I'm not sure how that potential future relates to the layering
> choices.  At any rate, I don't think we should choose our internal
> layering based on speculation about such future uses if it results in
> deciding on more complex internals (extra layers and the like) now.
> We can always refactor in the future when everything is clearer.
> 
> > 	* nptl/futex-internal.h: New file.
> 
> Just as a procedural matter, I'm inclined to say that a new file like
> this should come in the same commit as the first use of it.

Okay.  Do you want me to merge the two patches?

> Otherwise, even total build-breaking errors in the file wouldn't be
> noticed until the next change.  Likewise, it seems best to leave out
> things (e.g. futex_private_if_supported) until the commit that
> actually introduces a use.
> 
> > +#include <lowlevellock.h>
> 
> Include only what you need: lowlevellock-futex.h here.  That changes
> which code you're getting today, because all the machine-specific
> lowlevellock.h files still need to be removed.  But we should be
> finishing that cleanup this cycle anyway (though everyone seems to
> have forgotten).

I tried that now, but that doesn't work because it redefines lll_futex*,
and it's hard to avoid including lowlevellock.h through some other
header.  Therefore, I left this unchanged for now.

> > +  abort();
> 
> Space before paren.  Also, need #include <stdlib.h> to use abort.
> 
> So, now I'm seeing a potential reason to have this layer exist
> distinct from the OS-encapsulation layer.  Perhaps we should have the
> checks for expected errno values be in an OS-independent layer rather
> than just saying in the specification of the OS-encapsulation layer
> that it must yield only the particular set.

I'm not sure I can quite follow you.  I could see why the
OS-encapsulation layer would want to check that the set of return values
is only those we support in higher layers, but that's not what you're
after, or is it?

Updated patch is attached.  Is this one okay, or do you want to see
further changes to it and/or more of the full problem being addressed?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: futex-internal.patch
Type: text/x-patch
Size: 8665 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/libc-alpha/attachments/20141205/b136ae86/attachment.bin>