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

[Torvald Riegel <triegel at redhat dot com>]

On Tue, 2014-12-16 at 17:07 -0800, Roland McGrath wrote:
> > I want to push back on this a little for two reasons.  First, while I
> > agree that we don't want to have two interfaces for the same thing, this
> > isn't exactly the case: lll_futex_ has no error checking and is just a
> > wrapper for whatever the underlying kernel/... provides.  The futex
> > wrappers I introduced do have error checking.
> 
> The lack of error checking in lll_futex_* (and their callers) is a bug, as
> agreed here previously in discussion with kernel folks.  Frankly, I thought
> that introducing error checking uniformly to our futex uses was the
> motivation for your changes.  

It is.  I do want to have the low-level locks check the return values of
their futex calls.  To me, that doesn't conflict with having another
interface that is just what the underlying kernels/... provide.  We can
also get rid of this other interface and do error checking in each of
the per kernel/... implementations.  Does that clarify what I meant?

> Do you actually think that we want to reach an end state in which we have
> some uses of futex doing error checking and some uses not doing it?
> I find it hard to imagine we want that.

No, but having layered interfaces, where only the top-most one does
error checking and is used throughout glibc, doesn't seem obviously bad
to me.

> > Second, it's hard to easily commit to doing something that isn't clearly
> > defined, especially so close to the freeze.  I agree that we shouldn't
> > let introduce junk or duplicated functionality, but it may be
> > unrealistic to try to get all-or-nothing instead of incremental changes.
> 
> I didn't say the changes should be all-or-nothing.  
> Almost certainly they should not be.  
> 
> I said I wanted you to commit to changing all futex uses as part of the
> whole effort.  That has nothing to do with making it all one change.  It's
> simply about your personal commitment to keep working on the issue until
> all the changes are finished.
> 
> As to the proximity of the freeze, firstly we don't yet have any actual
> date for the freeze.

The message Carlos sent out states otherwise.  I believed we do want to
have fixed release dates, and I assumed that the release date would be
Dec 30.  The Jan 9 date Carlos mentioned gives a bit more time, but not
much.

> Secondly, I think this kind of cleanup work is best
> done without big delays in the middle and so probably should be all done in
> the same cycle.  If it's too much for you to finish in this cycle, then
> perhaps the whole thing should wait until the next cycle.

What would this mean for the new semaphore I posted?  Are you okay with
it using the current lll_futex_* operations and custom error checking?
Or do you want something like the futex-with-builtin-error-checking
interface that I posted?  In the latter case, this would block the
semaphore from going in.

> Personally, I'd
> rather we just get on with it now and if that pushes the next release
> freeze out a few weeks, that's fine with me.
> 
> > There is assembly that calls futex syscalls, which needs at least the
> > macros for the different futex ops, and the syscall number in certain
> > cases.  Those things are currently in lowlevellock.h.
> > lowlevellock-futex.h needs the same information, and it should not
> > include lowlevellock.h.
> 
> Do we have assembly code making futex syscalls that we actually want to
> keep?  My impression was that we wanted to get rid of pretty much all of
> the assembly files for this stuff, and it's simply us moving slowly for
> each machine that's why we haven't done that yet.

I would also hope that we can remove all of them.  But actually doing
that may be a bigger project.  The futex-using assembly files I see are
on x86, x86_64, and sh.  Its the low-level lock stuff (including
robust), pthread barrier, pthread condvar, pthread rwlock, pthread
semaphore, cancellation.

I've posted a new semaphore, and as I've said, am working on condvar and
rwlock.

Cancellation may be something that ends up doing futex calls from
assembly.  But given that futexes are on the slow path, the assembly
could probably just call C futex function instead.

> > It doesn't work currently on i386 due to six-argument syscalls not being
> > supported, AFAIU.  If someone can add that I'd appreciate it; it would
> > save me finding out how to do that properly.
> 
> Ah.  I think there are several of us who could tackle that.

Thanks.

> > > If you start with a strawman proposal for the complete new internal API,
> > > then we can all work together to figure out how to clean up existing code.
> > 
> > I'd start with just futex (timed)wait and wake, so what's in my patch.
> > That covers most of the uses.  The other ops are mostly for the
> > low-level locks, and I need to make a pass over the error handling for
> > these (but this was already discussed in the futex error handling
> > thread).
> 
> As long as you have a rough idea how everything else will fit in so that we
> can be confident that dealing with the other operations won't make us want
> to reconsider the whole organization of the internal API, that seems OK.
> 
> > I agree that the existance custom lowlevellock.h and the related
> > assembly files is an issue we want to fix.  But I think we can start
> > making the futex facility more generic independently of that.
> 
> Clearly we can start.  But the existence of the old assembly code seems to
> be a significant barrier to doing all the cleanup in the most
> straightforward ways.  I think it is probably just going to be net easier
> at the end of the day if we take some time first to clean out all the old
> assembly code and just don't have those wrinkles to think about when
> revamping the rest.
> 
> > 1) Have an OS-call interface that just does that.  This is what
> > lowlevellock-futex.h is currently, AFAIU.  This is implemented
> > differently on each OS.
> 
> That's roughly what it is currently.  But it's underspecified and it
> doesn't have a very clean internal API.  This should be replaced by
> something that isn't organized and named as an implementation detail of the
> lll layer,

Agreed that the name needs to be changed.

> cleans up the private flag stuff,

Yes, that would be good.

> and cleans up the API overall
> wrt things I've mentioned before like the stupid negated errno protocol and
> using type-checking inlines rather than loosy-goosy macros.
> 
> That's not to say that this cleanup is what must happen first.  But an end
> state where we still have lowlevellock-futex.h at all in anything like its
> current form is not desireable.

OK.

One thing we need to agree on is whether we need this interface in the
end at all, or whether we make the interface from 2) (see below) the one
that each OS implements.  What do you think?

Keeping 1) would make it easier for us to expose it to users in the
future (I do remember you not wanting to care about this now, though).
Keeping 1) isn't helping for internal use if the errors that it can
return are incompatible between the different OS implementations.

What is NaCl's error specification for the futex ops?  Given that much
of this refactoring is to make non-Linux futex possible, it would be
helpful if you can summarize what you need and want.

> > Make sure that there is a lowlevellock-futex.h on each arch.  The
> > attached patch does this for x86_64 and i386.  More details below.
> 
> I'm not clear on what that patch is accomplishing that's materially
> different from just using the existing linux/lowlevellock-futex.h on x86_64
> and i386.  But please post that patch in its own thread so it can get
> proper review and discussion separate from the rest of this discussion.

OK.

> > 2) Have an OS-independent internal futex interface, with error checking.
> > That's what's in my patch.  This uses the interface from 1).
> 
> OK.
> 
> > 3) Move generic, non-low-level-lock code over to using the interface
> > from 2).  The new semaphore does that.  I have a new condvar
> > implementation which is just missing the PI handling, which would do the
> > same.  I'm working on a new rwlock that would use it. Etc.
> 
> OK.
> 
> > 4) Use the interface from 2) in generic low-level lock.  This should be
> > fine and without significant performance implications because all futex
> > ops are on the slow path, with the exception of the PI mutexes.  But if
> > you're doing a syscall anyway, doing a few more instructions more or
> > less won't matter that much.
> 
> OK.  3 and 4 could happen in either order, or in parallel, right?

Yes.

> > 5) Remove custom low-level lock implementations after reviewing the
> > performance implications of such removals.
> 
> This need not wait for any other step.  Aside from the i386 issue, this can
> be done today and IMHO the sooner it's done the better.

That's true, but in contrast to the other steps, this may require more
time to assess the impact on performance.

> > This uses generic lowlevellock-futex.h for x86_64.  It also adds a few
> > #ifdef __ASSEMBLER__ to the generic Linux one to make this work.
> 
> Post that separately by itself.  Sounds trivial.

OK.

> > With i386, this doesn't work because of lack of support for six-argument
> > syscalls (see above); thus, this patch just splits out all the
> > lll_futex_* calls and related stuff into a i386-specific
> > lowlevellock-futex.h file.  This has fewer features than the generic
> > one, but the only users are the current interface from 2), which just
> > have futex (timed)wait and wake.  And it works currently, so this should
> > be fine and we can add stuff as necessary later on (or, better, move to
> > the generic lowlevellock-futex.h).
> 
> Post that separately by itself.  It sounds like it might be a fine
> intermediate change we could do right away.  Making i386 INTERNAL_SYSCALL
> handle the 6-argument case will probably be fiddly enough that it takes
> a bit longer.

OK.

> > * microblaze, s390, hppa all use INTERNAL_SYSCALL;  I believe those
> > could just use generic Linux lowlevellock-futex.h.
> 
> Yes.  I think the right approach here is just to post a patch doing the
> removal and let the machine maintainers agree or raise issues.  If you are
> in a position to do a performance comparison, either empirically and/or by
> eyeballing generated code, then I'm sure that's helpful to report.  But
> ultimately the responsibility is with each machine maintainer.  I suspect
> that at least e.g. microblaze and hppa maintainers will be happy to do the
> removal just based on no-semantic-regressions testing and not bother to
> fret about feared performance issues.

OK.  The futex ops are on the slow path, so performance differences
won't matter much (in contrast to lowlevellock.h, which contains the
fast paths too).

> > * ia64 uses DO_INLINE_SYSCALL.  Not sure whether that could use generic
> > Linux futexes too.
> 
> ia64's INTERNAL_SYSCALL is a pretty trivial wrapper around
> DO_INLINE_SYSCALL, so I think this is almost certainly just fine.

OK.

> > * sh has custom asm.  Not sure what to do about that.
> 
> Suggest removing it and see if the maintainer even cares to worry about
> whether the performance is affected.

OK.

> > * sparc uses INTERNAL_SYSCALL, so could be moved to generic Linux, but
> > there is a change (whose purpose/motivation I currently don't
> > understand):
> 
> Bug Dave to clarify the requirements for that.

OK.