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

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

> 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.  It avoids the
gratuitous churn whose only benefit is making the intermediate state
look well-layered.  I think it's better to have things go as quickly as
possible to what their expected end state is, with fewer intermediate
steps.  I agree (1) is bad because we'd have duplicated code/logic and
because it's better to migrate implementation and users in one step (or
closer to one).  To make things incremental, I think it makes most sense
to address one macro at a time (or small sets that go together): replace
lll_futex_foo macro with futex_foo function and change all users;
iterate.  But we can play it by ear.  I just think that adding new
things we know we'll remove soon is the wrong kind of churn.

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

Most code just checks for failure and reports all errors the same way
(using strerror et al) rather than examining errno for specific cases.

> If you think that's helpful for NaCl, we can do that.  

I think it is the right choice, yes.

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

We don't have documentation for these functions at all.  If we did, I
think I'd probably leave out any NaCl-specific issues for now anyway.

> Annotating the types used for atomic accesses is something I considered.

My remark about that was an aside and I don't think we should do
anything like this until later (after the futex cleanup is complete, and
perhaps other things).  So let's not muddy this thread by discussing
details further now.

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

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

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.)  That says
that any stored values should be kept in internal form so that the
actual futex calls are just using them directly.  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).

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

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

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

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

Sensible enough.

> Have you looked at the actual patch yet?

I skimmed it and didn't see deep issues beyond what I raised.  There
were a couple of trivial style things (whitespace and the like) that I
didn't bother to point out.  You'll probably see them if you just read
over the whole patch.  I'll do the fine-tooth comb review on the next
iteration once we've agreed on the details we're discussing here.


Thanks,
Roland