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

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

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

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.

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

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

> > 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, cleans up the private flag stuff, 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.

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

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

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

> Does this sound reasonable?  Are you OK with doing steps 1 and 2 before
> the freeze, and do as much of 3) as possible before the freeze?

At the moment I'm feeling ambivalent about splitting any of thus work up
across a freeze.  But each step as described is sufficiently self-contained
that it's probably fine just to do each as it's ready (with very careful
and thorough review and testing) regardless of where that falls relative to
a freeze deadline.

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

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

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

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

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

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


Thanks,
Roland