| Message ID | 20150526221729.7B7762C39FC@topped-with-meat.com |
|---|---|
| State | New |
| Headers | show |
On Tue, 2015-05-26 at 15:17 -0700, Roland McGrath wrote: > This lets a configuration replace __lll_timedlock_wait and/or > __lll_timedwait_tid with different implementations while still > using the generic code for __lll_lock_wait{,_private}. It seems you made the split just to support the absolute timeout path that NaCl futexes provide out of the box. IMO, this should have been addressed by fixing the glibc-internal futex API, not by adding a NaCl-specific variant and lots of new dummy files just to get back to the common implementation for all other archs. Didn't we agree to have as much commonality in our synchronization code as possible? I'd appreciate if we could have discussion on such patches in the future before they are committed. (And I wouldn't mind at all if you'd revert this patch.)
> It seems you made the split just to support the absolute timeout path > that NaCl futexes provide out of the box. IMO, this should have been > addressed by fixing the glibc-internal futex API, not by adding a > NaCl-specific variant and lots of new dummy files just to get back to > the common implementation for all other archs. Didn't we agree to have > as much commonality in our synchronization code as possible? > > I'd appreciate if we could have discussion on such patches in the future > before they are committed. (And I wouldn't mind at all if you'd revert > this patch.) In fact, the NaCl-specific __lll_timedwait_tid change was the real motivation and the NaCl-specific __lll_timedlock_wait (which shouldn't need to be, as you note) was just nearby low-hanging fruit. The actual change in this thread was purely splitting one file of multiple functions into more files of fewer functions each. That is simply moving things in the direction that they traditionally always were and always should be in libc: separate functions in separate files. One of the reasons that's the good way to be is that it makes more, finer grains that can be overridden in sysdeps/ variants without duplicating the source code that doesn't need to vary. But that's not the only reason to do it. (Avoiding dead code in static linking is the other concrete one, though that might well not apply usefully here.) It's a proper cleanup regardless of whether one needs or wants to actually implement new sysdeps/ variants of any of the functions in question. Such pure cleanup changes never need nontrivial review, and I won't apologize for committing them without waiting for feedback (unless I break a build or whatever, which was not the case here). Once the file was split up, it was very easy to add sysdeps/nacl/lll_timedlock_wait.c and save some suboptimal stupidity that had become hard for me to pretend I didn't see. I have no general expectation that anybody will review changes I make purely in sysdeps/nacl/ code, though I'm certainly grateful for any feedback anybody wants to give on that code. So there was no reason to wait before committing sysdeps/nacl/lll_timedlock_wait.c. Just because it's not the ideal solution doesn't mean it wasn't worth doing when I did it. It certainly will be simple enough to remove it again when it is no longer adding any benefit, and I fully intend to do that. Thanks, Roland
On Tue, 2015-06-09 at 17:13 -0700, Roland McGrath wrote: > > It seems you made the split just to support the absolute timeout path > > that NaCl futexes provide out of the box. IMO, this should have been > > addressed by fixing the glibc-internal futex API, not by adding a > > NaCl-specific variant and lots of new dummy files just to get back to > > the common implementation for all other archs. Didn't we agree to have > > as much commonality in our synchronization code as possible? > > > > I'd appreciate if we could have discussion on such patches in the future > > before they are committed. (And I wouldn't mind at all if you'd revert > > this patch.) > > In fact, the NaCl-specific __lll_timedwait_tid change was the real > motivation and the NaCl-specific __lll_timedlock_wait (which shouldn't > need to be, as you note) was just nearby low-hanging fruit. > > The actual change in this thread was purely splitting one file of > multiple functions into more files of fewer functions each. That is > simply moving things in the direction that they traditionally always > were and always should be in libc: separate functions in separate files. > One of the reasons that's the good way to be is that it makes more, > finer grains that can be overridden in sysdeps/ variants without > duplicating the source code that doesn't need to vary. In general, I agree. Nonetheless, in the particular case of synchronization code, I'd argue that we're likely doing something wrong if we need to specialize, say, Linux pthread_cond_signal, while keeping all other Linux pthread_cond_* functions unchanged. Functions that synchronize with each other are often much more dependent on each other than in sequential code, so the more variations there are the more complex it gets. Your patch added arch-specific variants on Linux, which isn't a bug as you correctly say, but it goes in the opposite direction of where we want to be at, I believe. Splitting into a Linux and a NaCl variant is a different story, of course. Another thing I have observed is that having variants of files available seems to invite variant-specific changes that could actually have been applied or fixed generically. Maybe that's something we can counter by paying more attention to this during review -- but not having separate files in the first place is a bigger hurdle :) (This is a general comment, of course, not specifically on your patch.)
> In general, I agree. Nonetheless, in the particular case of > synchronization code, I'd argue that we're likely doing something wrong > if we need to specialize, say, Linux pthread_cond_signal, while keeping > all other Linux pthread_cond_* functions unchanged. Functions that > synchronize with each other are often much more dependent on each other > than in sequential code, so the more variations there are the more > complex it gets. I understand the concern. > Your patch added arch-specific variants on Linux, which isn't a bug as > you correctly say, but it goes in the opposite direction of where we > want to be at, I believe. Not really. I did not actually change the status quo for x86 and sparc32 (the machines that have their own lowlevellock.[cS]). I had to add dummy files to let them stay as they are. I didn't touch them further than that since I wanted to minimize perturbation in that one change. But the change makes it possible to remove some existing duplication, such as __lll_timedwait_tid in sparc32/lowlevellock.c, which is a verbatim copy of the generic code that was made in the first place only because too many functions were put into this one file. I didn't do that sparc32 cleanup myself because I don't want to deal with testing or possibly breaking the sparc build myself. > Another thing I have observed is that having variants of files available > seems to invite variant-specific changes that could actually have been > applied or fixed generically. Maybe that's something we can counter by > paying more attention to this during review -- but not having separate > files in the first place is a bigger hurdle :) Certainly review is the best time to catch these.
diff --git a/nptl/Makefile b/nptl/Makefile index 8e99452..3dd2944 100644 --- a/nptl/Makefile +++ b/nptl/Makefile @@ -106,6 +106,7 @@ libpthread-routines = nptl-init vars events version \ pt-longjmp pt-cleanup\ cancellation \ lowlevellock lowlevelrobustlock \ + lll_timedlock_wait lll_timedwait_tid \ pt-fork pt-vfork \ ptw-write ptw-read ptw-close ptw-fcntl ptw-accept \ ptw-connect ptw-recv ptw-recvfrom ptw-recvmsg ptw-send \ diff --git a/nptl/lll_timedlock_wait.c b/nptl/lll_timedlock_wait.c new file mode 100644 index 0000000..37cf083 --- /dev/null +++ b/nptl/lll_timedlock_wait.c @@ -0,0 +1,59 @@ +/* Timed low level locking for pthread library. Generic futex-using version. + Copyright (C) 2003-2015 Free Software Foundation, Inc. + This file is part of the GNU C Library. + Contributed by Paul Mackerras <paulus@au.ibm.com>, 2003. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <http://www.gnu.org/licenses/>. */ + +#include <atomic.h> +#include <errno.h> +#include <lowlevellock.h> +#include <sys/time.h> + + +int +__lll_timedlock_wait (int *futex, const struct timespec *abstime, int private) +{ + /* Reject invalid timeouts. */ + if (abstime->tv_nsec < 0 || abstime->tv_nsec >= 1000000000) + return EINVAL; + + /* Try locking. */ + while (atomic_exchange_acq (futex, 2) != 0) + { + struct timeval tv; + + /* Get the current time. */ + (void) __gettimeofday (&tv, NULL); + + /* Compute relative timeout. */ + struct timespec rt; + rt.tv_sec = abstime->tv_sec - tv.tv_sec; + rt.tv_nsec = abstime->tv_nsec - tv.tv_usec * 1000; + if (rt.tv_nsec < 0) + { + rt.tv_nsec += 1000000000; + --rt.tv_sec; + } + + if (rt.tv_sec < 0) + return ETIMEDOUT; + + /* If *futex == 2, wait until woken or timeout. */ + lll_futex_timed_wait (futex, 2, &rt, private); + } + + return 0; +} diff --git a/nptl/lll_timedwait_tid.c b/nptl/lll_timedwait_tid.c new file mode 100644 index 0000000..68b4857 --- /dev/null +++ b/nptl/lll_timedwait_tid.c @@ -0,0 +1,70 @@ +/* Timed waiting for thread death. Generic futex-using version. + Copyright (C) 2003-2015 Free Software Foundation, Inc. + This file is part of the GNU C Library. + Contributed by Paul Mackerras <paulus@au.ibm.com>, 2003. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <http://www.gnu.org/licenses/>. */ + +#include <atomic.h> +#include <errno.h> +#include <lowlevellock.h> +#include <sys/time.h> + + +/* The kernel notifies a process which uses CLONE_CHILD_CLEARTID via futex + wake-up when the clone terminates. The memory location contains the + thread ID while the clone is running and is reset to zero by the kernel + afterwards. The kernel up to version 3.16.3 does not use the private futex + operations for futex wake-up when the clone terminates. */ +int +__lll_timedwait_tid (int *tidp, const struct timespec *abstime) +{ + int tid; + + if (abstime->tv_nsec < 0 || abstime->tv_nsec >= 1000000000) + return EINVAL; + + /* Repeat until thread terminated. */ + while ((tid = *tidp) != 0) + { + struct timeval tv; + struct timespec rt; + + /* Get the current time. */ + (void) __gettimeofday (&tv, NULL); + + /* Compute relative timeout. */ + rt.tv_sec = abstime->tv_sec - tv.tv_sec; + rt.tv_nsec = abstime->tv_nsec - tv.tv_usec * 1000; + if (rt.tv_nsec < 0) + { + rt.tv_nsec += 1000000000; + --rt.tv_sec; + } + + /* Already timed out? */ + if (rt.tv_sec < 0) + return ETIMEDOUT; + + /* If *tidp == tid, wait until thread terminates or the wait times out. + The kernel up to version 3.16.3 does not use the private futex + operations for futex wake-up when the clone terminates. + */ + if (lll_futex_timed_wait (tidp, tid, &rt, LLL_SHARED) == -ETIMEDOUT) + return ETIMEDOUT; + } + + return 0; +} diff --git a/nptl/lowlevellock.c b/nptl/lowlevellock.c index 98c859f..7025b9e 100644 --- a/nptl/lowlevellock.c +++ b/nptl/lowlevellock.c @@ -34,7 +34,7 @@ __lll_lock_wait_private (int *futex) } -/* These functions don't get included in libc.so */ +/* This function doesn't get included in libc. */ #if IS_IN (libpthread) void __lll_lock_wait (int *futex, int private) @@ -45,87 +45,4 @@ __lll_lock_wait (int *futex, int private) while (atomic_exchange_acq (futex, 2) != 0) lll_futex_wait (futex, 2, private); /* Wait if *futex == 2. */ } - - -int -__lll_timedlock_wait (int *futex, const struct timespec *abstime, int private) -{ - /* Reject invalid timeouts. */ - if (abstime->tv_nsec < 0 || abstime->tv_nsec >= 1000000000) - return EINVAL; - - /* Try locking. */ - while (atomic_exchange_acq (futex, 2) != 0) - { - struct timeval tv; - - /* Get the current time. */ - (void) __gettimeofday (&tv, NULL); - - /* Compute relative timeout. */ - struct timespec rt; - rt.tv_sec = abstime->tv_sec - tv.tv_sec; - rt.tv_nsec = abstime->tv_nsec - tv.tv_usec * 1000; - if (rt.tv_nsec < 0) - { - rt.tv_nsec += 1000000000; - --rt.tv_sec; - } - - if (rt.tv_sec < 0) - return ETIMEDOUT; - - /* If *futex == 2, wait until woken or timeout. */ - lll_futex_timed_wait (futex, 2, &rt, private); - } - - return 0; -} - - -/* The kernel notifies a process which uses CLONE_CHILD_CLEARTID via futex - wake-up when the clone terminates. The memory location contains the - thread ID while the clone is running and is reset to zero by the kernel - afterwards. The kernel up to version 3.16.3 does not use the private futex - operations for futex wake-up when the clone terminates. */ -int -__lll_timedwait_tid (int *tidp, const struct timespec *abstime) -{ - int tid; - - if (abstime->tv_nsec < 0 || abstime->tv_nsec >= 1000000000) - return EINVAL; - - /* Repeat until thread terminated. */ - while ((tid = *tidp) != 0) - { - struct timeval tv; - struct timespec rt; - - /* Get the current time. */ - (void) __gettimeofday (&tv, NULL); - - /* Compute relative timeout. */ - rt.tv_sec = abstime->tv_sec - tv.tv_sec; - rt.tv_nsec = abstime->tv_nsec - tv.tv_usec * 1000; - if (rt.tv_nsec < 0) - { - rt.tv_nsec += 1000000000; - --rt.tv_sec; - } - - /* Already timed out? */ - if (rt.tv_sec < 0) - return ETIMEDOUT; - - /* If *tidp == tid, wait until thread terminates or the wait times out. - The kernel up to version 3.16.3 does not use the private futex - operations for futex wake-up when the clone terminates. - */ - if (lll_futex_timed_wait (tidp, tid, &rt, LLL_SHARED) == -ETIMEDOUT) - return ETIMEDOUT; - } - - return 0; -} #endif diff --git a/sysdeps/sparc/sparc32/lll_timedlock_wait.c b/sysdeps/sparc/sparc32/lll_timedlock_wait.c new file mode 100644 index 0000000..c2c93fa --- /dev/null +++ b/sysdeps/sparc/sparc32/lll_timedlock_wait.c @@ -0,0 +1 @@ +/* __lll_timedlock_wait is in lowlevellock.c. */ diff --git a/sysdeps/sparc/sparc32/lll_timedwait_tid.c b/sysdeps/sparc/sparc32/lll_timedwait_tid.c new file mode 100644 index 0000000..511608e --- /dev/null +++ b/sysdeps/sparc/sparc32/lll_timedwait_tid.c @@ -0,0 +1 @@ +/* __lll_timedwait_tid is in lowlevellock.c. */ diff --git a/sysdeps/unix/sysv/linux/i386/i486/lll_timedlock_wait.c b/sysdeps/unix/sysv/linux/i386/i486/lll_timedlock_wait.c new file mode 100644 index 0000000..f6875b8 --- /dev/null +++ b/sysdeps/unix/sysv/linux/i386/i486/lll_timedlock_wait.c @@ -0,0 +1 @@ +/* __lll_timedlock_wait is in lowlevellock.S. */ diff --git a/sysdeps/unix/sysv/linux/i386/i486/lll_timedwait_tid.c b/sysdeps/unix/sysv/linux/i386/i486/lll_timedwait_tid.c new file mode 100644 index 0000000..43900c6 --- /dev/null +++ b/sysdeps/unix/sysv/linux/i386/i486/lll_timedwait_tid.c @@ -0,0 +1 @@ +/* __lll_timedwait_tid is in lowlevellock.S. */ diff --git a/sysdeps/unix/sysv/linux/i386/i586/lll_timedlock_wait.c b/sysdeps/unix/sysv/linux/i386/i586/lll_timedlock_wait.c new file mode 100644 index 0000000..fa4357b --- /dev/null +++ b/sysdeps/unix/sysv/linux/i386/i586/lll_timedlock_wait.c @@ -0,0 +1 @@ +#include "../i486/lll_timedlock_wait.c" diff --git a/sysdeps/unix/sysv/linux/i386/i586/lll_timedwait_tid.c b/sysdeps/unix/sysv/linux/i386/i586/lll_timedwait_tid.c new file mode 100644 index 0000000..1586104 --- /dev/null +++ b/sysdeps/unix/sysv/linux/i386/i586/lll_timedwait_tid.c @@ -0,0 +1 @@ +#include "../i486/lll_timedwait_tid.c" diff --git a/sysdeps/unix/sysv/linux/i386/i686/lll_timedlock_wait.c b/sysdeps/unix/sysv/linux/i386/i686/lll_timedlock_wait.c new file mode 100644 index 0000000..fa4357b --- /dev/null +++ b/sysdeps/unix/sysv/linux/i386/i686/lll_timedlock_wait.c @@ -0,0 +1 @@ +#include "../i486/lll_timedlock_wait.c" diff --git a/sysdeps/unix/sysv/linux/i386/i686/lll_timedwait_tid.c b/sysdeps/unix/sysv/linux/i386/i686/lll_timedwait_tid.c new file mode 100644 index 0000000..1586104 --- /dev/null +++ b/sysdeps/unix/sysv/linux/i386/i686/lll_timedwait_tid.c @@ -0,0 +1 @@ +#include "../i486/lll_timedwait_tid.c" diff --git a/sysdeps/unix/sysv/linux/x86_64/lll_timedlock_wait.c b/sysdeps/unix/sysv/linux/x86_64/lll_timedlock_wait.c new file mode 100644 index 0000000..f6875b8 --- /dev/null +++ b/sysdeps/unix/sysv/linux/x86_64/lll_timedlock_wait.c @@ -0,0 +1 @@ +/* __lll_timedlock_wait is in lowlevellock.S. */ diff --git a/sysdeps/unix/sysv/linux/x86_64/lll_timedwait_tid.c b/sysdeps/unix/sysv/linux/x86_64/lll_timedwait_tid.c new file mode 100644 index 0000000..43900c6 --- /dev/null +++ b/sysdeps/unix/sysv/linux/x86_64/lll_timedwait_tid.c @@ -0,0 +1 @@ +/* __lll_timedwait_tid is in lowlevellock.S. */
This lets a configuration replace __lll_timedlock_wait and/or __lll_timedwait_tid with different implementations while still using the generic code for __lll_lock_wait{,_private}. Build-tested x86_64-linux-gnu, i686-linux-gnu, and arm-linux-gnueabihf. This does nothing but move code around (and not even that on i?86, x86_64, and sparc32), so it should change nothing except possibly the order of these functions in the libpthread.so text segment (and statically-linked -lpthread applications' text segments). Thanks, Roland 2015-05-26 Roland McGrath <roland@hack.frob.com> * nptl/lowlevellock.c (__lll_timedlock_wait): Moved ... * nptl/lll_timedlock_wait.c: ... to this new file. * nptl/Makefile (libpthread-routines): Add it. * nptl/lowlevellock.c (__lll_timedwait_tid): Moved ... * nptl/lll_timedwait_tid.c: ... to this new file. * nptl/Makefile (libpthread-routines): Add it. * sysdeps/sparc/sparc32/lll_timedlock_wait.c: New file. * sysdeps/sparc/sparc32/lll_timedwait_tid.c: New file. * sysdeps/unix/sysv/linux/i386/i486/lll_timedlock_wait.c: New file. * sysdeps/unix/sysv/linux/i386/i586/lll_timedlock_wait.c: New file. * sysdeps/unix/sysv/linux/i386/i686/lll_timedlock_wait.c: New file. * sysdeps/unix/sysv/linux/i386/i486/lll_timedwait_tid.c: New file. * sysdeps/unix/sysv/linux/i386/i586/lll_timedwait_tid.c: New file. * sysdeps/unix/sysv/linux/i386/i686/lll_timedwait_tid.c: New file. * sysdeps/unix/sysv/linux/x86_64/lll_timedlock_wait.c: New file. * sysdeps/unix/sysv/linux/x86_64/lll_timedwait_tid.c: New file.