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

[Torvald Riegel <triegel at redhat dot com>]

On Thu, 2015-06-18 at 15:26 -0700, Roland McGrath wrote:
> > 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.
> 
> OK, I understand your thinking now.  I think (2) is best.

Alright.

> > 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.
> 
> Yes, we had strong consensus about sanity-checking the errors reported
> by the kernel.  That seems orthogonal to sanity-checking values in
> internal APIs before calling into the kernel, which I think is the only
> thing we're talking about here.

I'm talking about some input to the futex functions resulting in a call
to abort, either before it goes to the kernel / nacl runtime / ..., or
after because the kernel / ... returns a certain error.

> > 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).
> 
> I do agree that private must always be supported.  But I'm not at all
> sure I'm following exactly what kind of "distinguishing" you mean.
> 
> I had forgotten about the *_getpshared functions.  Clearly to support
> those the attributes object has to actually store something on
> configurations where shared is ever supported.  But on NaCl there is no
> need to store anything; the function to extract the flag from the
> internal form would just yield a constant.

But that's only possible because NaCl doesn't support shared, so no
conversion is necessary.

If we had a platform that would support private through translation to
shared (or whatever internal form it had), the attributes would still
have to use two separate values:  If one calls setphared(private), a
subsequent getpshared() has to return private -- not the merged internal
form (shared in this case).  That's similar to the lock elision issues
we had.

> This is the only sense of
> distinguishing private from shared that I think is necessary.
> 
> pthread_mutex_init and pthread_cond_init are far more common than the
> attribute-fiddling calls.  So they should not do any extra work that
> could be offloaded to the attribute-fiddling calls.  Since the
> attribute-fiddling calls need to validate their argument, it's optimal
> to roll the validation in with the conversion to internal form and so do
> it there.  (For sem_init, there is no such distinction.)

A single branch, or even a load of some config flag, is irrelevant
compared to the cost of actually sharing the mutex/condvar/... with
another thread -- which you need to do to actually have a need for the
mutex.  There's really no need to optimize this.  And we don't even have
a need to do anything there currently, for Linux or NaCl.

> Actual use of the objects, that leads to the futex calls, is of course
> the most common thing.  So it should not do any extra work at all that
> can be avoided.  (I realize it's a slow path, but still.)

Yes, it is a slow path.  But consider futex_wake: The rest of the slow
path enters the kernel, grabs a lock, likely cache misses on the lock
and/or the waitqueue (if there's an actual waiter, or has been), and
then potentially wakes another thread or just releases a lock, and
returns.  In the NaCl runtime, this will be similar except the syscall
overhead.
Some minor conversion like a branch is irrelevant compared to that.
NaCl wouldn't have any conversion; Linux may just replace the values. 

> That says
> that any stored values should be kept in internal form so that the
> actual futex calls are just using them directly.

But we have plenty of futex calls that are not used for PThreads
synchronization data structures.  I don't think it clarifies the code if
for those, we can't simply pass in FUTEX_PRIVATE, but have to first do
another call that transforms it to whatever futex_wake or such would
like to see internally.  So now we have those calls littered throughout
the code, and it would be cleanest to call this on every callsite.  That
just seems wrong.

> Any assertions to
> validate arguments to futex-internal.h calls are just checking for
> memory clobberation or libc bugs, so I don't see a real motivation for
> having them (though, like all assertions, it's never completely
> unreasonable to have them).

See above.  It's not assertions on input, it's just that the function
can abort on bad input.  You can simply decide to not do that in NaCl.

> > 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.
> 
> I don't think that's at all desireable.  (This is academic, since we
> don't actually have any configurations that need to "degrade" private to
> shared in this way.  But I'll elaborate anyway.)  Remember, assertions
> can always be disabled (and this is what most Linux distros do for
> production builds).

Well, the "assertions" that we have in the patch currently are not
assert(), but "if (unexpected_error) abort();".  IIRC having abort()
instead of assert() was one request regarding the error checking.

> So you could have an assertion inside futex_wake or
> whatnot, fine (though as I've said above, I don't see a good reason to
> want one).  But it's wrong logic to think that because you'll have an
> assertion looking at the value you might as well have other logic on the
> value in the same place.  With assertions disabled, there would be no
> need for any examination of the value inside futex_wake beyond just
> OR'ing it into the operation argument to the syscall.

See above.

> If we did need to handle this case, then I think the best thing would be
> to have another sysdeps call that transforms an internal form stored by
> *_setpshared into the internal form actually used in futex_wake et al.
> Then pthread_mutex_init et al would use that call instead of simply
> copying the field from attributes object to synchronization object.  (I
> don't have any objection to structuring it this way now even though all
> the sysdeps implementations today would in fact just copy the value.)
> 
> > 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.
> 
> I disagree.  This pair of functions is very straightforward to define.
> I've explained above why delaying transformation any later than necessary
> is suboptimal.
> 
> > Anyway, I don't care strongly about that.  We're talking about 4
> > occurrences of setpshared.  Which option do you want to have?
> 
> error = futex_init_pshared (&attr->pshared, pshared_argument);
> pshared = futex_get_pshared (&attr->pshared);

As mentioned above, the internal form will have to preserve two values,
if a user can set either successfully.

It also doesn't handle the default attribute.  We can have a macro /
constexpr that transform the default attribute in place, or need a
special case for when NULL is passed as the attribute.  It would be
simpler to transform at this time not getpshared/setpshared time, if we
can't agree on simply doing it on futex_* call time.

> sem_init can just pass (pshared ? PTHREAD_PROCESS_SHARED :
> PTHREAD_PROCESS_PRIVATE) as the argument, and inlining/constant-folding
> should turn it back into optimal.
> 
> So, just what I said before, but expanded to cover getpshared.
> (I don't care about the functions' names or their exact signatures.)

What would you do for the other calls to futex_* that pass in
FUTEX_PRIVATE directly?  (There are about 20 such calls in the patch
that I sent...).

I'll prepare a patch that does (2) above and the ENOTSUP for NaCl, but
keeps the remaining checks as is.  I think it would be worthwhile if you
would look at this patch specifically and check whether you do have
concerns about any of the actually generated code (regarding runtime
overheads).