Remove arm lowlevellock.c

Message ID CALE0ps2nxAqHeotsxVcBEOV+nRsFGLBLD8+kP2ZY-PdnELkueA@mail.gmail.com
State New
Headers show

Commit Message

Bernie Ogden April 28, 2014, 2:50 p.m.
lowlevellock.c for arm differs from the generic lowlevellock.c only in
insignificant ways, so can be removed. Happily, this fixes BZ 15119
(unnecessary busy loop in __lll_timedlock_wait on arm).

The notable differences between the arm and generic implementations are:

1) arm __lll_timedlock_wait has a fast path out if futex has been set
to 0 between since the function was called. This seems unlikely to
happen very often, so it seems at worst harmless to lose this fast
path.

2) Some function in arm's lowlevellock.c set futex to 2 if it was 1.
The generic version always sets the futex to 2. As futex can only be
0, 1 or 2 on entry into these functions, the behaviour is equivalent.
(If the futex manages to be 0 on entry then we've just lost another
unlikely fast path out.)

There are no test suite regressions.

Note that hppa and sparc also have their own lowlevellock.c. I believe
hppa can also be removed, so I'll send a separate patch for that
shortly. sparc's seems to be genuinely needed as it uses a different
locking structure.

Also note that the analysis at
https://sourceware.org/ml/libc-ports/2013-02/msg00021.html indicates a
further locking performance bug to fix - I've got a partial patch for
that which I can submit once I've finished testing.

2014-04-24  Bernard Ogden <bernie.ogden@linaro.org>

[BZ #15119]
* sysdeps/unix/sysv/linux/arm/nptl/lowlevellock.c: Remove file.

Comments

Will Newton April 28, 2014, 2:56 p.m. | #1
Hi Bernie,

ARM patches can now be sent to libc-alpha as ARM has moved from ports
into the main tree.

I'm not sure if we still use libc-ports for HPPA patches...

On 28 April 2014 15:50, Bernie Ogden <bernie.ogden@linaro.org> wrote:
> lowlevellock.c for arm differs from the generic lowlevellock.c only in
> insignificant ways, so can be removed. Happily, this fixes BZ 15119
> (unnecessary busy loop in __lll_timedlock_wait on arm).
>
> The notable differences between the arm and generic implementations are:
>
> 1) arm __lll_timedlock_wait has a fast path out if futex has been set
> to 0 between since the function was called. This seems unlikely to
> happen very often, so it seems at worst harmless to lose this fast
> path.
>
> 2) Some function in arm's lowlevellock.c set futex to 2 if it was 1.
> The generic version always sets the futex to 2. As futex can only be
> 0, 1 or 2 on entry into these functions, the behaviour is equivalent.
> (If the futex manages to be 0 on entry then we've just lost another
> unlikely fast path out.)
>
> There are no test suite regressions.
>
> Note that hppa and sparc also have their own lowlevellock.c. I believe
> hppa can also be removed, so I'll send a separate patch for that
> shortly. sparc's seems to be genuinely needed as it uses a different
> locking structure.
>
> Also note that the analysis at
> https://sourceware.org/ml/libc-ports/2013-02/msg00021.html indicates a
> further locking performance bug to fix - I've got a partial patch for
> that which I can submit once I've finished testing.
>
> 2014-04-24  Bernard Ogden <bernie.ogden@linaro.org>
>
> [BZ #15119]
> * sysdeps/unix/sysv/linux/arm/nptl/lowlevellock.c: Remove file.
>
>
> diff --git a/sysdeps/unix/sysv/linux/arm/nptl/lowlevellock.c
> b/sysdeps/unix/sysv/linux/arm/nptl/lowlevellock.c
> deleted file mode 100644
> index 9603d7b..0000000
> --- a/sysdeps/unix/sysv/linux/arm/nptl/lowlevellock.c
> +++ /dev/null
> @@ -1,132 +0,0 @@
> -/* low level locking for pthread library.  Generic futex-using version.
> -   Copyright (C) 2003-2014 Free Software Foundation, Inc.
> -   This file is part of the GNU C Library.
> -
> -   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 <errno.h>
> -#include <sysdep.h>
> -#include <lowlevellock.h>
> -#include <sys/time.h>
> -
> -void
> -__lll_lock_wait_private (int *futex)
> -{
> -  do
> -    {
> -      int oldval = atomic_compare_and_exchange_val_acq (futex, 2, 1);
> -      if (oldval != 0)
> - lll_futex_wait (futex, 2, LLL_PRIVATE);
> -    }
> -  while (atomic_compare_and_exchange_bool_acq (futex, 2, 0) != 0);
> -}
> -
> -
> -/* These functions don't get included in libc.so  */
> -#ifdef IS_IN_libpthread
> -void
> -__lll_lock_wait (int *futex, int private)
> -{
> -  do
> -    {
> -      int oldval = atomic_compare_and_exchange_val_acq (futex, 2, 1);
> -      if (oldval != 0)
> - lll_futex_wait (futex, 2, private);
> -    }
> -  while (atomic_compare_and_exchange_bool_acq (futex, 2, 0) != 0);
> -}
> -
> -
> -int
> -__lll_timedlock_wait (int *futex, const struct timespec *abstime, int private)
> -{
> -  struct timespec rt;
> -
> -  /* Reject invalid timeouts.  */
> -  if (abstime->tv_nsec < 0 || abstime->tv_nsec >= 1000000000)
> -    return EINVAL;
> -
> -  /* Upgrade the lock.  */
> -  if (atomic_exchange_acq (futex, 2) == 0)
> -    return 0;
> -
> -  do
> -    {
> -      struct timeval tv;
> -
> -      /* 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;
> -
> -      // XYZ: Lost the lock to check whether it was private.
> -      lll_futex_timed_wait (futex, 2, &rt, private);
> -    }
> -  while (atomic_compare_and_exchange_bool_acq (futex, 2, 0) != 0);
> -
> -  return 0;
> -}
> -
> -
> -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;
> -
> -      /* Wait until thread terminates.  */
> -      // XYZ: Lost the lock to check whether it was private.
> -      if (lll_futex_timed_wait (tidp, tid, &rt, LLL_SHARED) == -ETIMEDOUT)
> - return ETIMEDOUT;
> -    }
> -
> -  return 0;
> -}
> -#endif
Joseph Myers April 29, 2014, 3:26 p.m. | #2
On Mon, 28 Apr 2014, Will Newton wrote:

> Hi Bernie,
> 
> ARM patches can now be sent to libc-alpha as ARM has moved from ports
> into the main tree.
> 
> I'm not sure if we still use libc-ports for HPPA patches...
> 
> On 28 April 2014 15:50, Bernie Ogden <bernie.ogden@linaro.org> wrote:
> > lowlevellock.c for arm differs from the generic lowlevellock.c only in
> > insignificant ways, so can be removed. Happily, this fixes BZ 15119
> > (unnecessary busy loop in __lll_timedlock_wait on arm).

 ...

> > Also note that the analysis at
> > https://sourceware.org/ml/libc-ports/2013-02/msg00021.html indicates a
> > further locking performance bug to fix - I've got a partial patch for
> > that which I can submit once I've finished testing.

That analysis asserts that ARM's lowlevellock.c is trying to work around 
a bug in lowlevellock.h.  Are you asserting in this patch that in fact the 
workaround is not needed - that there is no regression caused by removing 
the lowlevellock.c file before fixing the lowlevellock.h bug?

(Actually I'd like to see unification of the lowlevellock.h files as far 
as possible, not just lowlevellock.c.)
Bernie Ogden April 30, 2014, 12:58 p.m. | #3
The workaround is that some of the arm lowlevellock.c functions
promote futex to 2 if it is 1. Generic lowlevellock.c always promotes
futex to 2. Hence, removing arm's lowlevellock.c doesn't cause a
regression in this sense.

It does mean that arm stops being affected by BZ 15119 and instead is
affected by the second bug. So we go from having BZ 15119 on arm, and
a second bug on aarch64, m68k and sh/sh4, to having the second bug
across all of these platforms. That feels like progress to me, but you
could reasonably differ.

I agree with you on unifying lowlevellock.h - so it'll take a little
longer for me to submit the fix for the second bug as I'll stop to
unify the files as part of the work. (Quite a few of them do look
unifiable.)

I guess I should create something in bugzilla for 'the second bug' -
I'll go do that soon.

On 29 April 2014 16:26, Joseph S. Myers <joseph@codesourcery.com> wrote:
> On Mon, 28 Apr 2014, Will Newton wrote:
>
>> Hi Bernie,
>>
>> ARM patches can now be sent to libc-alpha as ARM has moved from ports
>> into the main tree.
>>
>> I'm not sure if we still use libc-ports for HPPA patches...
>>
>> On 28 April 2014 15:50, Bernie Ogden <bernie.ogden@linaro.org> wrote:
>> > lowlevellock.c for arm differs from the generic lowlevellock.c only in
>> > insignificant ways, so can be removed. Happily, this fixes BZ 15119
>> > (unnecessary busy loop in __lll_timedlock_wait on arm).
>
>  ...
>
>> > Also note that the analysis at
>> > https://sourceware.org/ml/libc-ports/2013-02/msg00021.html indicates a
>> > further locking performance bug to fix - I've got a partial patch for
>> > that which I can submit once I've finished testing.
>
> That analysis asserts that ARM's lowlevellock.c is trying to work around
> a bug in lowlevellock.h.  Are you asserting in this patch that in fact the
> workaround is not needed - that there is no regression caused by removing
> the lowlevellock.c file before fixing the lowlevellock.h bug?
>
> (Actually I'd like to see unification of the lowlevellock.h files as far
> as possible, not just lowlevellock.c.)
>
> --
> Joseph S. Myers
> joseph@codesourcery.com
Joseph Myers April 30, 2014, 3:49 p.m. | #4
On Wed, 30 Apr 2014, Bernie Ogden wrote:

> The workaround is that some of the arm lowlevellock.c functions
> promote futex to 2 if it is 1. Generic lowlevellock.c always promotes
> futex to 2. Hence, removing arm's lowlevellock.c doesn't cause a
> regression in this sense.

Thanks.  The original patch is OK.

> I agree with you on unifying lowlevellock.h - so it'll take a little
> longer for me to submit the fix for the second bug as I'll stop to
> unify the files as part of the work. (Quite a few of them do look
> unifiable.)

FWIW there are two main different styles of syscall error handling in the 
files, but I don't know if that's in any way a necessary difference; at 
least it shouldn't require duplicating the whole file.  (Compare the ARM 
and MIPS versions of lll_futex_timed_wait, for example.)
Bernie Ogden May 1, 2014, 1:02 p.m. | #5
Raised BZ 16892 for the lowlevellock.h issues.

Thanks for the pointer.

On 30 April 2014 16:49, Joseph S. Myers <joseph@codesourcery.com> wrote:
> On Wed, 30 Apr 2014, Bernie Ogden wrote:
>
>> The workaround is that some of the arm lowlevellock.c functions
>> promote futex to 2 if it is 1. Generic lowlevellock.c always promotes
>> futex to 2. Hence, removing arm's lowlevellock.c doesn't cause a
>> regression in this sense.
>
> Thanks.  The original patch is OK.
>
>> I agree with you on unifying lowlevellock.h - so it'll take a little
>> longer for me to submit the fix for the second bug as I'll stop to
>> unify the files as part of the work. (Quite a few of them do look
>> unifiable.)
>
> FWIW there are two main different styles of syscall error handling in the
> files, but I don't know if that's in any way a necessary difference; at
> least it shouldn't require duplicating the whole file.  (Compare the ARM
> and MIPS versions of lll_futex_timed_wait, for example.)
>
> --
> Joseph S. Myers
> joseph@codesourcery.com
Will Newton May 1, 2014, 1:29 p.m. | #6
On 30 April 2014 16:49, Joseph S. Myers <joseph@codesourcery.com> wrote:
> On Wed, 30 Apr 2014, Bernie Ogden wrote:
>
>> The workaround is that some of the arm lowlevellock.c functions
>> promote futex to 2 if it is 1. Generic lowlevellock.c always promotes
>> futex to 2. Hence, removing arm's lowlevellock.c doesn't cause a
>> regression in this sense.
>
> Thanks.  The original patch is OK.

i applied this patch on Bernie's behalf.

Patch hide | download patch | download mbox

diff --git a/sysdeps/unix/sysv/linux/arm/nptl/lowlevellock.c
b/sysdeps/unix/sysv/linux/arm/nptl/lowlevellock.c
deleted file mode 100644
index 9603d7b..0000000
--- a/sysdeps/unix/sysv/linux/arm/nptl/lowlevellock.c
+++ /dev/null
@@ -1,132 +0,0 @@ 
-/* low level locking for pthread library.  Generic futex-using version.
-   Copyright (C) 2003-2014 Free Software Foundation, Inc.
-   This file is part of the GNU C Library.
-
-   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 <errno.h>
-#include <sysdep.h>
-#include <lowlevellock.h>
-#include <sys/time.h>
-
-void
-__lll_lock_wait_private (int *futex)
-{
-  do
-    {
-      int oldval = atomic_compare_and_exchange_val_acq (futex, 2, 1);
-      if (oldval != 0)
- lll_futex_wait (futex, 2, LLL_PRIVATE);
-    }
-  while (atomic_compare_and_exchange_bool_acq (futex, 2, 0) != 0);
-}
-
-
-/* These functions don't get included in libc.so  */
-#ifdef IS_IN_libpthread
-void
-__lll_lock_wait (int *futex, int private)
-{
-  do
-    {
-      int oldval = atomic_compare_and_exchange_val_acq (futex, 2, 1);
-      if (oldval != 0)
- lll_futex_wait (futex, 2, private);
-    }
-  while (atomic_compare_and_exchange_bool_acq (futex, 2, 0) != 0);
-}
-
-
-int
-__lll_timedlock_wait (int *futex, const struct timespec *abstime, int private)
-{
-  struct timespec rt;
-
-  /* Reject invalid timeouts.  */
-  if (abstime->tv_nsec < 0 || abstime->tv_nsec >= 1000000000)
-    return EINVAL;
-
-  /* Upgrade the lock.  */
-  if (atomic_exchange_acq (futex, 2) == 0)
-    return 0;
-
-  do
-    {
-      struct timeval tv;
-
-      /* 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;
-
-      // XYZ: Lost the lock to check whether it was private.
-      lll_futex_timed_wait (futex, 2, &rt, private);
-    }
-  while (atomic_compare_and_exchange_bool_acq (futex, 2, 0) != 0);
-
-  return 0;
-}
-
-
-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;
-
-      /* Wait until thread terminates.  */
-      // XYZ: Lost the lock to check whether it was private.
-      if (lll_futex_timed_wait (tidp, tid, &rt, LLL_SHARED) == -ETIMEDOUT)
- return ETIMEDOUT;
-    }
-
-  return 0;
-}
-#endif