public inbox for libc-ports@sourceware.org
 help / color / mirror / Atom feed
* [PATCH] Remove arm lowlevellock.c
@ 2014-04-28 14:50 Bernie Ogden
  2014-04-28 14:56 ` Will Newton
  0 siblings, 1 reply; 7+ messages in thread
From: Bernie Ogden @ 2014-04-28 14:50 UTC (permalink / raw)
  To: libc-ports

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] Remove arm lowlevellock.c
  2014-04-28 14:50 [PATCH] Remove arm lowlevellock.c Bernie Ogden
@ 2014-04-28 14:56 ` Will Newton
  2014-04-29 15:26   ` Joseph S. Myers
  0 siblings, 1 reply; 7+ messages in thread
From: Will Newton @ 2014-04-28 14:56 UTC (permalink / raw)
  To: Bernie Ogden; +Cc: libc-ports, libc-alpha

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



-- 
Will Newton
Toolchain Working Group, Linaro

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] Remove arm lowlevellock.c
  2014-04-28 14:56 ` Will Newton
@ 2014-04-29 15:26   ` Joseph S. Myers
  2014-04-30 12:58     ` Bernie Ogden
  0 siblings, 1 reply; 7+ messages in thread
From: Joseph S. Myers @ 2014-04-29 15:26 UTC (permalink / raw)
  To: Will Newton; +Cc: Bernie Ogden, libc-ports, libc-alpha

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] Remove arm lowlevellock.c
  2014-04-29 15:26   ` Joseph S. Myers
@ 2014-04-30 12:58     ` Bernie Ogden
  2014-04-30 15:50       ` Joseph S. Myers
  0 siblings, 1 reply; 7+ messages in thread
From: Bernie Ogden @ 2014-04-30 12:58 UTC (permalink / raw)
  To: Joseph S. Myers; +Cc: Will Newton, libc-ports, libc-alpha

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] Remove arm lowlevellock.c
  2014-04-30 12:58     ` Bernie Ogden
@ 2014-04-30 15:50       ` Joseph S. Myers
  2014-05-01 13:03         ` Bernie Ogden
  2014-05-01 13:29         ` Will Newton
  0 siblings, 2 replies; 7+ messages in thread
From: Joseph S. Myers @ 2014-04-30 15:50 UTC (permalink / raw)
  To: Bernie Ogden; +Cc: Will Newton, libc-ports, libc-alpha

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] Remove arm lowlevellock.c
  2014-04-30 15:50       ` Joseph S. Myers
@ 2014-05-01 13:03         ` Bernie Ogden
  2014-05-01 13:29         ` Will Newton
  1 sibling, 0 replies; 7+ messages in thread
From: Bernie Ogden @ 2014-05-01 13:03 UTC (permalink / raw)
  To: Joseph S. Myers; +Cc: Will Newton, libc-ports, libc-alpha

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] Remove arm lowlevellock.c
  2014-04-30 15:50       ` Joseph S. Myers
  2014-05-01 13:03         ` Bernie Ogden
@ 2014-05-01 13:29         ` Will Newton
  1 sibling, 0 replies; 7+ messages in thread
From: Will Newton @ 2014-05-01 13:29 UTC (permalink / raw)
  To: Joseph S. Myers; +Cc: Bernie Ogden, libc-ports, libc-alpha

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.

-- 
Will Newton
Toolchain Working Group, Linaro

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2014-05-01 13:29 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-04-28 14:50 [PATCH] Remove arm lowlevellock.c Bernie Ogden
2014-04-28 14:56 ` Will Newton
2014-04-29 15:26   ` Joseph S. Myers
2014-04-30 12:58     ` Bernie Ogden
2014-04-30 15:50       ` Joseph S. Myers
2014-05-01 13:03         ` Bernie Ogden
2014-05-01 13:29         ` Will Newton

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).