Hi,
I'm propose to take a glibc's commit 6d96f5e4c0 to solve the unnecessary busy loop in __lll_timedlock_wait on ARM, described in details at:
https://sourceware.org/bugzilla/show_bug.cgi?id=15119 https://patchwork.ozlabs.org/patch/332656/
what is your opinion?
Regards, Leonid
Hi Leonid, Leonid Lisovskiy wrote,
Hi,
I'm propose to take a glibc's commit 6d96f5e4c0 to solve the unnecessary busy loop in __lll_timedlock_wait on ARM, described in details at:
https://sourceware.org/bugzilla/show_bug.cgi?id=15119 https://patchwork.ozlabs.org/patch/332656/
what is your opinion?
So you remove the specific lowlevellock.c from ARM and take that the generic works fine?
best regards Waldemar
On Mon, May 23, 2016 at 2:18 PM, Waldemar Brodkorb wbx@uclibc-ng.org wrote:
So you remove the specific lowlevellock.c from ARM and take that the generic works fine?
With appropriate Makefile.arch changes, of course.
I haven't real hardware, but basic tests runs under QEMU fine. Moreover, it tested by glibc users since generic lowlevellock.c exactly the same as ours.
Leonid
Hi Leonid, Leonid Lisovskiy wrote,
On Mon, May 23, 2016 at 2:18 PM, Waldemar Brodkorb wbx@uclibc-ng.org wrote:
So you remove the specific lowlevellock.c from ARM and take that the generic works fine?
With appropriate Makefile.arch changes, of course.
I haven't real hardware, but basic tests runs under QEMU fine. Moreover, it tested by glibc users since generic lowlevellock.c exactly the same as ours.
I am fine with this change, removing unneccessary architecture specific files is a good thing.
best regards Waldemar
On Tue, May 24, 2016 at 10:32 AM, Waldemar Brodkorb wbx@uclibc-ng.org wrote:
I am fine with this change, removing unneccessary architecture specific files is a good thing.
Should I prepare the patch against the trunk? If yes, should I keep the original author of glibc commit ("Will Newton")?
Leonid
Hi, Leonid Lisovskiy wrote,
On Tue, May 24, 2016 at 10:32 AM, Waldemar Brodkorb wbx@uclibc-ng.org wrote:
I am fine with this change, removing unneccessary architecture specific files is a good thing.
Should I prepare the patch against the trunk? If yes, should I keep the original author of glibc commit ("Will Newton")?
Trunk is good. As you like, you can mention the commit from Glibc, should be enough.
best regards Waldemar
lowlevellock.c for arm differs from the generic lowlevellock.c only in insignificant ways, so can be removed. Follow glibc commit 6d96f5e4c0
Solves __lll_timedlock_wait busy-wait issues described at http://sourceware.org/bugzilla/show_bug.cgi?id=15119
Signed-off-by: Leonid Lisovskiy lly.dev@gmail.com --- .../nptl/sysdeps/unix/sysv/linux/arm/Makefile.arch | 5 +- .../unix/sysv/linux/arm/libc-lowlevellock.c | 20 --- .../sysdeps/unix/sysv/linux/arm/lowlevellock.c | 136 --------------------- 3 files changed, 2 insertions(+), 159 deletions(-) delete mode 100644 libpthread/nptl/sysdeps/unix/sysv/linux/arm/libc-lowlevellock.c delete mode 100644 libpthread/nptl/sysdeps/unix/sysv/linux/arm/lowlevellock.c
diff --git a/libpthread/nptl/sysdeps/unix/sysv/linux/arm/Makefile.arch b/libpthread/nptl/sysdeps/unix/sysv/linux/arm/Makefile.arch index 5522ce3..80a0306 100644 --- a/libpthread/nptl/sysdeps/unix/sysv/linux/arm/Makefile.arch +++ b/libpthread/nptl/sysdeps/unix/sysv/linux/arm/Makefile.arch @@ -7,10 +7,9 @@
libpthread_linux_arch_SSRC = libpthread_linux_arch_CSRC = pthread_once.c \ - pt-__syscall_rt_sigaction.c pt-__syscall_error.c \ - lowlevellock.c + pt-__syscall_rt_sigaction.c pt-__syscall_error.c
-libc_linux_arch_CSRC = fork.c libc-lowlevellock.c +libc_linux_arch_CSRC = fork.c libc_linux_arch_SSRC = clone.S vfork.S libc_linux_arch_SSRC-OMIT = waitpid.S
diff --git a/libpthread/nptl/sysdeps/unix/sysv/linux/arm/libc-lowlevellock.c b/libpthread/nptl/sysdeps/unix/sysv/linux/arm/libc-lowlevellock.c deleted file mode 100644 index 28672a6..0000000 --- a/libpthread/nptl/sysdeps/unix/sysv/linux/arm/libc-lowlevellock.c +++ /dev/null @@ -1,20 +0,0 @@ -/* Copyright (C) 2003 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/. */ - -/* No difference to lowlevellock.c, except we lose a couple of functions. */ -#include "lowlevellock.c" diff --git a/libpthread/nptl/sysdeps/unix/sysv/linux/arm/lowlevellock.c b/libpthread/nptl/sysdeps/unix/sysv/linux/arm/lowlevellock.c deleted file mode 100644 index cd42135..0000000 --- a/libpthread/nptl/sysdeps/unix/sysv/linux/arm/lowlevellock.c +++ /dev/null @@ -1,136 +0,0 @@ -/* low level locking for pthread library. Generic futex-using version. - Copyright (C) 2003, 2005, 2007 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> -#include <tls.h> - -void -#ifndef IS_IN_libpthread -weak_function -#endif -__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
Hi Leonid, Leonid Lisovskiy wrote,
lowlevellock.c for arm differs from the generic lowlevellock.c only in insignificant ways, so can be removed. Follow glibc commit 6d96f5e4c0
Solves __lll_timedlock_wait busy-wait issues described at http://sourceware.org/bugzilla/show_bug.cgi?id=15119
Signed-off-by: Leonid Lisovskiy lly.dev@gmail.com
Thanks, applied and pushed, Waldemar
On Thu, Jun 2, 2016 at 9:58 PM, Waldemar Brodkorb wbx@uclibc-ng.org wrote:
lowlevellock.c for arm differs from the generic lowlevellock.c only in insignificant ways, so can be removed. Follow glibc commit 6d96f5e4c0
Thanks, applied and pushed, Waldemar
I suggest to try to repeat the same action for other platforms using local lowlevellock.c copy. Starting from xtensa, since it don't use it at all(no reference to lowlevellock.c in Makefile.in), then - arc, metag, sparc.
But, unfortunately, I can't make the tests on these platforms myself. Waldemar, could you help with testing?
Regards, Leonid
Hi Leonid, Leonid Lisovskiy wrote,
On Thu, Jun 2, 2016 at 9:58 PM, Waldemar Brodkorb wbx@uclibc-ng.org wrote:
lowlevellock.c for arm differs from the generic lowlevellock.c only in insignificant ways, so can be removed. Follow glibc commit 6d96f5e4c0
Thanks, applied and pushed, Waldemar
I suggest to try to repeat the same action for other platforms using local lowlevellock.c copy. Starting from xtensa, since it don't use it at all(no reference to lowlevellock.c in Makefile.in), then - arc, metag, sparc.
But, unfortunately, I can't make the tests on these platforms myself. Waldemar, could you help with testing?
If you provide one patch for each architecture I can test them and if no regression happens I can commit them. Do you think our testsuite covers the included functionality in lowlevellock.c enough, so that no failure count changes, mean the generic code is fine?
best regards Waldemar
On Fri, Jun 3, 2016 at 6:28 PM, Waldemar Brodkorb wbx@uclibc-ng.org wrote:
If you provide one patch for each architecture I can test them and if no regression happens I can commit them.
Excellent! Patches attached. The single platform that keep local lowlevellock.c is sparc, like glibc (sysdeps/sparc/sparc32/lowlevellock.c).
Do you think our testsuite covers the included functionality in lowlevellock.c enough, so that no failure count changes, mean the generic code is fine?
Testsuite cover standard, predictable, situations. But I'm unsure about races & other unusual problems tests. Yes, this adds some potential risk, but I haven't any good idea how to eliminate it at all.
regards, Leonid
Hi Leonid, Leonid Lisovskiy wrote,
On Fri, Jun 3, 2016 at 6:28 PM, Waldemar Brodkorb wbx@uclibc-ng.org wrote:
If you provide one patch for each architecture I can test them and if no regression happens I can commit them.
Excellent! Patches attached. The single platform that keep local lowlevellock.c is sparc, like glibc (sysdeps/sparc/sparc32/lowlevellock.c).
Do you think our testsuite covers the included functionality in lowlevellock.c enough, so that no failure count changes, mean the generic code is fine?
Testsuite cover standard, predictable, situations. But I'm unsure about races & other unusual problems tests. Yes, this adds some potential risk, but I haven't any good idea how to eliminate it at all.
Thanks, I can live with this risk.
Tested and pushed, Waldemar
-
Leonid Lisovskiy -
Waldemar Brodkorb