[COMMITTED] Split timed-wait functions out of nptl/lowlevellock.c.
diff mbox

Message ID 20150526221729.7B7762C39FC@topped-with-meat.com
State New
Headers show

Commit Message

Roland McGrath May 26, 2015, 10:17 p.m. UTC
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.

Comments

Torvald Riegel May 27, 2015, 8:51 a.m. UTC | #1
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.)
Roland McGrath June 10, 2015, 12:13 a.m. UTC | #2
> 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
Torvald Riegel June 10, 2015, 12:46 p.m. UTC | #3
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.)
Roland McGrath June 10, 2015, 8:18 p.m. UTC | #4
> 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.

Patch
diff mbox

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