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

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

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

Yes, I understand that.  I mentioned this case below.

> That's similar to the lock elision issues we had.

I don't know what this refers to, but perhaps it's not really relevant.

> A single branch, or even a load of some config flag, is irrelevant [...]

I understand that the cost is lost in the noise.  That does mean that
it's not important to spend effort on avoiding that cost.  But it does
not mean that when you have multiple choices with similar amounts of
work to implement and all else being equal, you should not choose one
that has less cost.  Less is still less.

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

There are different classes of checks.  For things that could only be
due to libc bugs or memory clobberation, assert is right.  The only
places we want explicit checks even under -DNDEBUG are those where we
are checking return values from other components of the system such as
the kernel.  (Carlos posted a more thorough taxonomy of cases/checks
and I think we had consensus about how to treat each one.)

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

What I was calling "internal form" is exactly FUTEX_PRIVATE et al--the
arguments to futex-internal.h functions.  I never proposed that direct
uses of futex-internal.h would do anything different.  (For NaCl, the
FUTEX_{PRIVATE,SHARED} macros might well just have the same value,
since the values would never actually be examined.)

It sort of seems like you missed this paragraph, though you quoted it:
> > 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.)

As I said there, if we were to handle the "degrade to shared"
situation, then there could be a third form (a second internal form).
For such a configuration, that form would be the only one that needs
to distinguish requested-private from requested-shared just so that
*_getpshared can work.  In configurations without that need (which is
all we have today), then presumably the sysdeps call mentioned above
would simply use "final" internal form (i.e. FUTEX_* values passed to
futex-internal.h functions) as the intermediate internal form
(i.e. what's stored in attributes objects).

Since we don't need to handle that case today, I think this is all
moot.  For now, just having final internal form (FUTEX_* values) be
what's stored in attributes objects as well as synchronization objects
will work fine.

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

Let's simplify things by not considering the case we don't actually
have.  So it's universal that the only things that get stored are the
final FUTEX_* values.  That means it's simple to just do:

	object->private = attr == NULL ? FUTEX_PRIVATE : attr->pshared;

(Remember, futex_get_pshared is only there for the *_getpshared
implementations to use.  It's never involved in either initialization
or use of synchronization objects.)

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

I hope I've now convinced you of two more things.  But sure, I will
look at the concrete patch and try not to quibble overmuch.


Thanks,
Roland