This is the mail archive of the
mailing list for the glibc project.
Re: [PATCH v4] Add and use new glibc-internal futex API.
- From: Roland McGrath <roland at hack dot frob dot com>
- To: Torvald Riegel <triegel at redhat dot com>
- Cc: GLIBC Devel <libc-alpha at sourceware dot org>
- Date: Wed, 1 Jul 2015 18:27:47 -0700 (PDT)
- Subject: Re: [PATCH v4] Add and use new glibc-internal futex API.
- Authentication-results: sourceware.org; auth=none
- References: <1434987160 dot 25759 dot 26 dot camel at localhost dot localdomain> <20150624232258 dot 9A74C2C3B00 at topped-with-meat dot com> <1435749621 dot 4216 dot 64 dot camel at localhost dot localdomain>
> I've removed the casts. The clean-up to enums should be done with the
> patch that uses atomics to access setxid_futex.
> (Note that the lack of atomic accesses to futex words is a recurring
> problem, and not one I intended to solve with this patch.)
> ISTM the cancelhandling change to unsigned int requires more pervasive
> code changes than in the patch you posted (see my reply to that).
> I've kept the casts for now, with the expectation that we'll change to
> unsigned int in the patch that uses atomics for all accesses to
OK. Anything is fine with me as long as by the end we clean things up to
remove all the casts.
> I'm now including sysdeps/nptl/futex-internal.h from the
> platform-specific futex-internal.h files. futex_wait_simple goes there
> as well as futex_fatal_error.
That should be fine. It wouldn't hurt to pass the errno value and perhaps
a string naming the operation to futex_fatal_error. Then we can later
change it to call __libc_message (1, ...) for a more informative error.
(__libc_fatal takes a plain string, but __libc_message takes a minimal
> Can't do that right now because it still needs to cast away the volatile
> that's still present after your recent change that makes it volatile
> unsigned int. I don't want to just remove the volatile either, because
> this way make the synchronization work (it really should use atomics
> though). This can be cleaned up when we transform this to using
Agreed. Perhaps every cast you add should have a formulaic (greppable)
comment so we can be sure we find them all in the later cleanup passes.
> I had posted this last year, and I remember that this is considered the
> "publication" of the code.
> Updated patch is attached. Once this is OK, I'll send an updated patch
> that includes the (updated) sparc changes so that Dave can test (per
> Joseph's request, to allow for bisect-ability, we can't have a separate
> patch for the sparc bits).
I didn't check it closely, nor test it on NaCl. I won't get to that until
next week. (I'm actually on vacation this whole week, and just checking in
today to make sure I wasn't blocking you.) Since we've resolved the issues
I raised, I'm sure it will be fine now. I would appreciate if you could
amend it with greppable comments for everything we agreed we should clean
up later. But beyond that if it has no regressions and nobody else
objects, it's fine with me to commit now. If it turns out NaCl is actually
broken, I'll fix it up next week.
It wouldn't hurt to write up somewhere, probably on the wiki (though in
code comments in this patch would also be fine by me) the known/intended
further steps of transition and cleanup in this area. I imagine that this
patch and maybe a bit more trivia is about all we'll get done before the
coming release freeze. In practice that tends to mean we stop thinking
about the details for a month (and then that bleeds into August travel and
vacations and it always winds up longer than we intend before we get back
to it), so it seems wise to make the short-term future plans clear now
while the current discussion is fresh (and record them someplace easier to
follow than scattered in these review threads).