| Message ID | CALE0ps2nxAqHeotsxVcBEOV+nRsFGLBLD8+kP2ZY-PdnELkueA@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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
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.)
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
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.)
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
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.
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