* [PATCH] Improve performance of IO locks @ 2022-06-08 16:32 Wilco Dijkstra 2022-08-01 11:06 ` Wilco Dijkstra 0 siblings, 1 reply; 16+ messages in thread From: Wilco Dijkstra @ 2022-06-08 16:32 UTC (permalink / raw) To: 'GNU C Library' Improve performance of recursive IO locks by adding a fast path for the single-threaded case. To reduce the number of memory accesses for locking/unlocking, only increment the recursion counter if the lock is already taken. On Neoverse V1, a microbenchmark with many small freads improved by 2.9 times. Multithreaded performance improved by 2%. Passes GLIBC testsuite, OK for commit? --- diff --git a/sysdeps/nptl/stdio-lock.h b/sysdeps/nptl/stdio-lock.h index afa0b779c81d7dd915f8edb6c0974e4f231d4e0a..45823cd1629d3e3efecc64a7d07706a6e6de9af1 100644 --- a/sysdeps/nptl/stdio-lock.h +++ b/sysdeps/nptl/stdio-lock.h @@ -37,12 +37,18 @@ typedef struct { int lock; int cnt; void *owner; } _IO_lock_t; #define _IO_lock_lock(_name) \ do { \ void *__self = THREAD_SELF; \ - if ((_name).owner != __self) \ + if (SINGLE_THREAD_P && (_name).owner == NULL) \ + { \ + (_name).lock = LLL_LOCK_INITIALIZER_LOCKED; \ + (_name).owner = __self; \ + } \ + else if ((_name).owner != __self) \ { \ lll_lock ((_name).lock, LLL_PRIVATE); \ - (_name).owner = __self; \ + (_name).owner = __self; \ } \ - ++(_name).cnt; \ + else \ + ++(_name).cnt; \ } while (0) #define _IO_lock_trylock(_name) \ @@ -52,10 +58,7 @@ typedef struct { int lock; int cnt; void *owner; } _IO_lock_t; if ((_name).owner != __self) \ { \ if (lll_trylock ((_name).lock) == 0) \ - { \ - (_name).owner = __self; \ - (_name).cnt = 1; \ - } \ + (_name).owner = __self; \ else \ __result = EBUSY; \ } \ @@ -66,11 +69,18 @@ typedef struct { int lock; int cnt; void *owner; } _IO_lock_t; #define _IO_lock_unlock(_name) \ do { \ - if (--(_name).cnt == 0) \ + if (SINGLE_THREAD_P && (_name).cnt == 0) \ + { \ + (_name).owner = NULL; \ + (_name).lock = 0; \ + } \ + else if ((_name).cnt == 0) \ { \ - (_name).owner = NULL; \ + (_name).owner = NULL; \ lll_unlock ((_name).lock, LLL_PRIVATE); \ } \ + else \ + --(_name).cnt; \ } while (0) ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Improve performance of IO locks 2022-06-08 16:32 [PATCH] Improve performance of IO locks Wilco Dijkstra @ 2022-08-01 11:06 ` Wilco Dijkstra 2022-08-07 12:51 ` Cristian Rodríguez 2022-08-15 22:27 ` Mark Wielaard 0 siblings, 2 replies; 16+ messages in thread From: Wilco Dijkstra @ 2022-08-01 11:06 UTC (permalink / raw) To: 'GNU C Library' ping Improve performance of recursive IO locks by adding a fast path for the single-threaded case. To reduce the number of memory accesses for locking/unlocking, only increment the recursion counter if the lock is already taken. On Neoverse V1, a microbenchmark with many small freads improved by 2.9 times. Multithreaded performance improved by 2%. Passes GLIBC testsuite, OK for commit? --- diff --git a/sysdeps/nptl/stdio-lock.h b/sysdeps/nptl/stdio-lock.h index afa0b779c81d7dd915f8edb6c0974e4f231d4e0a..45823cd1629d3e3efecc64a7d07706a6e6de9af1 100644 --- a/sysdeps/nptl/stdio-lock.h +++ b/sysdeps/nptl/stdio-lock.h @@ -37,12 +37,18 @@ typedef struct { int lock; int cnt; void *owner; } _IO_lock_t; #define _IO_lock_lock(_name) \ do { \ void *__self = THREAD_SELF; \ - if ((_name).owner != __self) \ + if (SINGLE_THREAD_P && (_name).owner == NULL) \ + { \ + (_name).lock = LLL_LOCK_INITIALIZER_LOCKED; \ + (_name).owner = __self; \ + } \ + else if ((_name).owner != __self) \ { \ lll_lock ((_name).lock, LLL_PRIVATE); \ - (_name).owner = __self; \ + (_name).owner = __self; \ } \ - ++(_name).cnt; \ + else \ + ++(_name).cnt; \ } while (0) #define _IO_lock_trylock(_name) \ @@ -52,10 +58,7 @@ typedef struct { int lock; int cnt; void *owner; } _IO_lock_t; if ((_name).owner != __self) \ { \ if (lll_trylock ((_name).lock) == 0) \ - { \ - (_name).owner = __self; \ - (_name).cnt = 1; \ - } \ + (_name).owner = __self; \ else \ __result = EBUSY; \ } \ @@ -66,11 +69,18 @@ typedef struct { int lock; int cnt; void *owner; } _IO_lock_t; #define _IO_lock_unlock(_name) \ do { \ - if (--(_name).cnt == 0) \ + if (SINGLE_THREAD_P && (_name).cnt == 0) \ + { \ + (_name).owner = NULL; \ + (_name).lock = 0; \ + } \ + else if ((_name).cnt == 0) \ { \ - (_name).owner = NULL; \ + (_name).owner = NULL; \ lll_unlock ((_name).lock, LLL_PRIVATE); \ } \ + else \ + --(_name).cnt; \ } while (0) ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Improve performance of IO locks 2022-08-01 11:06 ` Wilco Dijkstra @ 2022-08-07 12:51 ` Cristian Rodríguez 2022-08-15 22:27 ` Mark Wielaard 1 sibling, 0 replies; 16+ messages in thread From: Cristian Rodríguez @ 2022-08-07 12:51 UTC (permalink / raw) To: Wilco Dijkstra; +Cc: GNU C Library Sounds good, tested on x86_64. On Mon, Aug 1, 2022 at 7:06 AM Wilco Dijkstra via Libc-alpha <libc-alpha@sourceware.org> wrote: > > > ping > > > Improve performance of recursive IO locks by adding a fast path for > the single-threaded case. To reduce the number of memory accesses for > locking/unlocking, only increment the recursion counter if the lock > is already taken. > > On Neoverse V1, a microbenchmark with many small freads improved by > 2.9 times. Multithreaded performance improved by 2%. > > Passes GLIBC testsuite, OK for commit? > > --- > > diff --git a/sysdeps/nptl/stdio-lock.h b/sysdeps/nptl/stdio-lock.h > index afa0b779c81d7dd915f8edb6c0974e4f231d4e0a..45823cd1629d3e3efecc64a7d07706a6e6de9af1 100644 > --- a/sysdeps/nptl/stdio-lock.h > +++ b/sysdeps/nptl/stdio-lock.h > @@ -37,12 +37,18 @@ typedef struct { int lock; int cnt; void *owner; } _IO_lock_t; > #define _IO_lock_lock(_name) \ > do { \ > void *__self = THREAD_SELF; \ > - if ((_name).owner != __self) \ > + if (SINGLE_THREAD_P && (_name).owner == NULL) \ > + { \ > + (_name).lock = LLL_LOCK_INITIALIZER_LOCKED; \ > + (_name).owner = __self; \ > + } \ > + else if ((_name).owner != __self) \ > { \ > lll_lock ((_name).lock, LLL_PRIVATE); \ > - (_name).owner = __self; \ > + (_name).owner = __self; \ > } \ > - ++(_name).cnt; \ > + else \ > + ++(_name).cnt; \ > } while (0) > > #define _IO_lock_trylock(_name) \ > @@ -52,10 +58,7 @@ typedef struct { int lock; int cnt; void *owner; } _IO_lock_t; > if ((_name).owner != __self) \ > { \ > if (lll_trylock ((_name).lock) == 0) \ > - { \ > - (_name).owner = __self; \ > - (_name).cnt = 1; \ > - } \ > + (_name).owner = __self; \ > else \ > __result = EBUSY; \ > } \ > @@ -66,11 +69,18 @@ typedef struct { int lock; int cnt; void *owner; } _IO_lock_t; > > #define _IO_lock_unlock(_name) \ > do { \ > - if (--(_name).cnt == 0) \ > + if (SINGLE_THREAD_P && (_name).cnt == 0) \ > + { \ > + (_name).owner = NULL; \ > + (_name).lock = 0; \ > + } \ > + else if ((_name).cnt == 0) \ > { \ > - (_name).owner = NULL; \ > + (_name).owner = NULL; \ > lll_unlock ((_name).lock, LLL_PRIVATE); \ > } \ > + else \ > + --(_name).cnt; \ > } while (0) > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Improve performance of IO locks 2022-08-01 11:06 ` Wilco Dijkstra 2022-08-07 12:51 ` Cristian Rodríguez @ 2022-08-15 22:27 ` Mark Wielaard 2022-08-16 3:07 ` Thomas Fitzsimmons 1 sibling, 1 reply; 16+ messages in thread From: Mark Wielaard @ 2022-08-15 22:27 UTC (permalink / raw) To: Wilco Dijkstra; +Cc: 'GNU C Library', Thomas Fitzsimmons Hi, On Mon, Aug 01, 2022 at 11:06:07AM +0000, Wilco Dijkstra via Libc-alpha wrote: > Improve performance of recursive IO locks by adding a fast path for > the single-threaded case. To reduce the number of memory accesses for > locking/unlocking, only increment the recursion counter if the lock > is already taken. > > On Neoverse V1, a microbenchmark with many small freads improved by > 2.9 times. Multithreaded performance improved by 2%. Strangely this seems to have broken the glibc-debian-ppc64 buildbot: https://builder.sourceware.org/buildbot/#/builders/glibc-debian-ppc64 I don't see how this commit can cause a failure just on debian-ppc64 (all other distro/arches are fine). And the corresponding bunsen test results don't really show why. https://builder.sourceware.org/testrun/498f51f327afdd7030516455b709a31a0038b432 Most of the .out files are empty, but some indicate an segmentation fault. Comparing to the build before only shows test result diffs, no configuration differences. https://builder.sourceware.org/testrun-diffs?commitish=498f51f327afdd7030516455b709a31a0038b432&commitish=58fd9d63b078b6bbfdba45135c4021038f33534e I don't have access to the buildbot, so cannot easily investigate more. Tom, could you have a look and see if you can find out more? Does just reverting commit c51c483d2b8ae66fe31a12509aedae02a6982ced make things OK again? Or did something else on the buildbot worker change? Note that the buildbot only does a make subdirs=elf check, not a full make check. Thanks, Mark ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Improve performance of IO locks 2022-08-15 22:27 ` Mark Wielaard @ 2022-08-16 3:07 ` Thomas Fitzsimmons 2022-08-16 7:31 ` Florian Weimer 0 siblings, 1 reply; 16+ messages in thread From: Thomas Fitzsimmons @ 2022-08-16 3:07 UTC (permalink / raw) To: Mark Wielaard; +Cc: Wilco Dijkstra, 'GNU C Library' [-- Attachment #1: Type: text/plain, Size: 2005 bytes --] Hi Mark, Mark Wielaard <mark@klomp.org> writes: > On Mon, Aug 01, 2022 at 11:06:07AM +0000, Wilco Dijkstra via Libc-alpha wrote: >> Improve performance of recursive IO locks by adding a fast path for >> the single-threaded case. To reduce the number of memory accesses for >> locking/unlocking, only increment the recursion counter if the lock >> is already taken. >> >> On Neoverse V1, a microbenchmark with many small freads improved by >> 2.9 times. Multithreaded performance improved by 2%. > > Strangely this seems to have broken the glibc-debian-ppc64 buildbot: > https://builder.sourceware.org/buildbot/#/builders/glibc-debian-ppc64 > > I don't see how this commit can cause a failure just on debian-ppc64 > (all other distro/arches are fine). > > And the corresponding bunsen test results don't really show why. > https://builder.sourceware.org/testrun/498f51f327afdd7030516455b709a31a0038b432 > Most of the .out files are empty, but some indicate an segmentation fault. > > Comparing to the build before only shows test result diffs, no > configuration differences. > https://builder.sourceware.org/testrun-diffs?commitish=498f51f327afdd7030516455b709a31a0038b432&commitish=58fd9d63b078b6bbfdba45135c4021038f33534e > > I don't have access to the buildbot, so cannot easily investigate more. > > Tom, could you have a look and see if you can find out more? Does just > reverting commit c51c483d2b8ae66fe31a12509aedae02a6982ced make things > OK again? Yes reverting that commit, the result is: Summary of test results: 317 PASS 10 UNSUPPORTED 2 XPASS Without the reversion: Summary of test results: 256 FAIL 68 PASS 3 UNSUPPORTED 2 XFAIL I looked at elf/unload as an example; it's segfaulting in _dl_relocate_object, backtrace attached; not sure what else to check. > Or did something else on the buildbot worker change? I didn't change anything. > Note that the buildbot only does a make subdirs=elf check, not a full > make check. Thanks, Thomas [-- Attachment #2: glibc-unload-segfault-backtrace-1.txt --] [-- Type: text/plain, Size: 4262 bytes --] buildbot@debian-ppc64-builder:~/workers/wildebeest/glibc-debian-ppc64/build$ env GCONV_PATH=/var/lib/buildbot/workers/wildebeest/glibc-debian-ppc64/build/iconvdata LOCPATH=/var/lib/buildbot/workers/wildebeest/glibc-debian-ppc64/build/localedata LC_ALL=C gdb --args /var/lib/buildbot/workers/wildebeest/glibc-debian-ppc64/build/elf/ld64.so.1 --library-path /var/lib/buildbot/workers/wildebeest/glibc-debian-ppc64/build:/var/lib/buildbot/workers/wildebeest/glibc-debian-ppc64/build/math:/var/lib/buildbot/workers/wildebeest/glibc-debian-ppc64/build/elf:/var/lib/buildbot/workers/wildebeest/glibc-debian-ppc64/build/dlfcn:/var/lib/buildbot/workers/wildebeest/glibc-debian-ppc64/build/nss:/var/lib/buildbot/workers/wildebeest/glibc-debian-ppc64/build/nis:/var/lib/buildbot/workers/wildebeest/glibc-debian-ppc64/build/rt:/var/lib/buildbot/workers/wildebeest/glibc-debian-ppc64/build/resolv:/var/lib/buildbot/workers/wildebeest/glibc-debian-ppc64/build/mathvec:/var/lib/buildbot/workers/wildebeest/glibc-debian-ppc64/build/support:/var/lib/buildbot/workers/wildebeest/glibc-debian-ppc64/build/crypt:/var/lib/buildbot/workers/wildebeest/glibc-debian-ppc64/build/nptl /var/lib/buildbot/workers/wildebeest/glibc-debian-ppc64/build/elf/unload GNU gdb (Debian 10.1-2+b1) 10.1.90.20210103-git Copyright (C) 2021 Free Software Foundation, Inc. License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html> This is free software: you are free to change and redistribute it. There is NO WARRANTY, to the extent permitted by law. Type "show copying" and "show warranty" for details. This GDB was configured as "powerpc64-linux-gnu". Type "show configuration" for configuration details. For bug reporting instructions, please see: <https://www.gnu.org/software/gdb/bugs/>. Find the GDB manual and other documentation resources online at: <http://www.gnu.org/software/gdb/documentation/>. For help, type "help". Type "apropos word" to search for commands related to "word"... Reading symbols from /var/lib/buildbot/workers/wildebeest/glibc-debian-ppc64/build/elf/ld64.so.1... (gdb) r Starting program: /var/lib/buildbot/workers/wildebeest/glibc-debian-ppc64/build/elf/ld64.so.1 --library-path /var/lib/buildbot/workers/wildebeest/glibc-debian-ppc64/build:/var/lib/buildbot/workers/wildebeest/glibc-debian-ppc64/build/math:/var/lib/buildbot/workers/wildebeest/glibc-debian-ppc64/build/elf:/var/lib/buildbot/workers/wildebeest/glibc-debian-ppc64/build/dlfcn:/var/lib/buildbot/workers/wildebeest/glibc-debian-ppc64/build/nss:/var/lib/buildbot/workers/wildebeest/glibc-debian-ppc64/build/nis:/var/lib/buildbot/workers/wildebeest/glibc-debian-ppc64/build/rt:/var/lib/buildbot/workers/wildebeest/glibc-debian-ppc64/build/resolv:/var/lib/buildbot/workers/wildebeest/glibc-debian-ppc64/build/mathvec:/var/lib/buildbot/workers/wildebeest/glibc-debian-ppc64/build/support:/var/lib/buildbot/workers/wildebeest/glibc-debian-ppc64/build/crypt:/var/lib/buildbot/workers/wildebeest/glibc-debian-ppc64/build/nptl /var/lib/buildbot/workers/wildebeest/glibc-debian-ppc64/build/elf/unload Program received signal SIGSEGV, Segmentation fault. 0x00007ffff7fabad8 in _dl_relocate_object (l=0x7ffff7ff2eb0, scope=0x7ffff7ff3250, reloc_mode=<optimized out>, consider_profiling=<optimized out>) at dl-reloc.c:301 301 ELF_DYNAMIC_RELOCATE (l, scope, lazy, consider_profiling, skip_ifunc); (gdb) bt #0 0x00007ffff7fabad8 in _dl_relocate_object (l=0x7ffff7ff2eb0, scope=0x7ffff7ff3250, reloc_mode=<optimized out>, consider_profiling=<optimized out>) at dl-reloc.c:301 #1 0x00007ffff7fc3c1c in dl_main (phdr=<optimized out>, phnum=<optimized out>, user_entry=<optimized out>, auxv=<optimized out>) at rtld.c:2314 #2 0x00007ffff7fbef40 in _dl_sysdep_start (start_argptr=<optimized out>, dl_main=@0x7ffff7fef840: 0x7ffff7fc1ba0 <dl_main>) at ../sysdeps/unix/sysv/linux/dl-sysdep.c:140 #3 0x00007ffff7fc0a30 in _dl_start_final (arg=arg@entry=0x7ffffffff6c0, info=info@entry=0x7ffffffff0a0) at rtld.c:497 #4 0x00007ffff7fc16f4 in _dl_start (arg=0x7ffffffff6c0) at rtld.c:586 #5 0x00007ffff7fbfcb0 in ._start () from /var/lib/buildbot/workers/wildebeest/glibc-debian-ppc64/build/elf/ld64.so.1 (gdb) ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Improve performance of IO locks 2022-08-16 3:07 ` Thomas Fitzsimmons @ 2022-08-16 7:31 ` Florian Weimer 2022-08-16 10:00 ` Mark Wielaard 2022-08-16 10:24 ` Wilco Dijkstra 0 siblings, 2 replies; 16+ messages in thread From: Florian Weimer @ 2022-08-16 7:31 UTC (permalink / raw) To: Thomas Fitzsimmons; +Cc: Mark Wielaard, 'GNU C Library', Wilco Dijkstra * Thomas Fitzsimmons: > Hi Mark, > > Mark Wielaard <mark@klomp.org> writes: > >> On Mon, Aug 01, 2022 at 11:06:07AM +0000, Wilco Dijkstra via Libc-alpha wrote: >>> Improve performance of recursive IO locks by adding a fast path for >>> the single-threaded case. To reduce the number of memory accesses for >>> locking/unlocking, only increment the recursion counter if the lock >>> is already taken. >>> >>> On Neoverse V1, a microbenchmark with many small freads improved by >>> 2.9 times. Multithreaded performance improved by 2%. >> >> Strangely this seems to have broken the glibc-debian-ppc64 buildbot: >> https://builder.sourceware.org/buildbot/#/builders/glibc-debian-ppc64 >> >> I don't see how this commit can cause a failure just on debian-ppc64 >> (all other distro/arches are fine). >> >> And the corresponding bunsen test results don't really show why. >> https://builder.sourceware.org/testrun/498f51f327afdd7030516455b709a31a0038b432 >> Most of the .out files are empty, but some indicate an segmentation fault. >> >> Comparing to the build before only shows test result diffs, no >> configuration differences. >> https://builder.sourceware.org/testrun-diffs?commitish=498f51f327afdd7030516455b709a31a0038b432&commitish=58fd9d63b078b6bbfdba45135c4021038f33534e >> >> I don't have access to the buildbot, so cannot easily investigate more. >> >> Tom, could you have a look and see if you can find out more? Does just >> reverting commit c51c483d2b8ae66fe31a12509aedae02a6982ced make things >> OK again? > > Yes reverting that commit, the result is: > > Summary of test results: > 317 PASS > 10 UNSUPPORTED > 2 XPASS > > Without the reversion: > > Summary of test results: > 256 FAIL > 68 PASS > 3 UNSUPPORTED > 2 XFAIL > > I looked at elf/unload as an example; it's segfaulting in > _dl_relocate_object, backtrace attached; not sure what else to check. I don't see this on powerpc64, with a toolchain based on GCC 8.2 and binutils 2.30. I'm at a loss how these things could be related. Thanks, Florian ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Improve performance of IO locks 2022-08-16 7:31 ` Florian Weimer @ 2022-08-16 10:00 ` Mark Wielaard 2022-08-16 10:08 ` Florian Weimer 2022-08-16 11:19 ` Andreas Schwab 2022-08-16 10:24 ` Wilco Dijkstra 1 sibling, 2 replies; 16+ messages in thread From: Mark Wielaard @ 2022-08-16 10:00 UTC (permalink / raw) To: Florian Weimer Cc: Thomas Fitzsimmons, 'GNU C Library', Wilco Dijkstra Hi, On Tue, Aug 16, 2022 at 09:31:49AM +0200, Florian Weimer wrote: > > Mark Wielaard <mark@klomp.org> writes: > >> On Mon, Aug 01, 2022 at 11:06:07AM +0000, Wilco Dijkstra via Libc-alpha wrote: > >>> Improve performance of recursive IO locks by adding a fast path for > >>> the single-threaded case. To reduce the number of memory accesses for > >>> locking/unlocking, only increment the recursion counter if the lock > >>> is already taken. > >>> > >>> On Neoverse V1, a microbenchmark with many small freads improved by > >>> 2.9 times. Multithreaded performance improved by 2%. > >> > >> Strangely this seems to have broken the glibc-debian-ppc64 buildbot: > >> https://builder.sourceware.org/buildbot/#/builders/glibc-debian-ppc64 > >> > >> I don't see how this commit can cause a failure just on debian-ppc64 > >> (all other distro/arches are fine). > >> > >> And the corresponding bunsen test results don't really show why. > >> https://builder.sourceware.org/testrun/498f51f327afdd7030516455b709a31a0038b432 > >> Most of the .out files are empty, but some indicate an segmentation fault. > >> > >> Comparing to the build before only shows test result diffs, no > >> configuration differences. > >> https://builder.sourceware.org/testrun-diffs?commitish=498f51f327afdd7030516455b709a31a0038b432&commitish=58fd9d63b078b6bbfdba45135c4021038f33534e > >> > >> I don't have access to the buildbot, so cannot easily investigate more. > >> > >> Tom, could you have a look and see if you can find out more? Does just > >> reverting commit c51c483d2b8ae66fe31a12509aedae02a6982ced make things > >> OK again? > > > > Yes reverting that commit, the result is: > > > > Summary of test results: > > 317 PASS > > 10 UNSUPPORTED > > 2 XPASS > > > > Without the reversion: > > > > Summary of test results: > > 256 FAIL > > 68 PASS > > 3 UNSUPPORTED > > 2 XFAIL > > > > I looked at elf/unload as an example; it's segfaulting in > > _dl_relocate_object, backtrace attached; not sure what else to check. > > I don't see this on powerpc64, with a toolchain based on GCC 8.2 and > binutils 2.30. I'm at a loss how these things could be related. Yeah, I am actually surprised just reverting the patch fixed things. But looking at the bunsen data for glibc-debian-ppc64 does seem to show that the only thing changed is this particular patch. Runs before it pass, runs after show lots of fails. https://builder.sourceware.org/testruns/?has_keyvalue_like_k=testrun.git_describe&has_keyvalue_like_v=%25glibc-debian-ppc64%25 The debian-ppc64 worker has GCC 11.2.0 and binutils 2.38 https://builder.sourceware.org/buildbot/#/workers/10 But I don't see how the patch and the crash backtrace are related. Cheers, Mark ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Improve performance of IO locks 2022-08-16 10:00 ` Mark Wielaard @ 2022-08-16 10:08 ` Florian Weimer 2022-08-17 13:45 ` Mark Wielaard 2022-08-16 11:19 ` Andreas Schwab 1 sibling, 1 reply; 16+ messages in thread From: Florian Weimer @ 2022-08-16 10:08 UTC (permalink / raw) To: Mark Wielaard; +Cc: Thomas Fitzsimmons, 'GNU C Library', Wilco Dijkstra * Mark Wielaard: > Yeah, I am actually surprised just reverting the patch fixed things. > But looking at the bunsen data for glibc-debian-ppc64 does seem to > show that the only thing changed is this particular patch. Runs before > it pass, runs after show lots of fails. > > https://builder.sourceware.org/testruns/?has_keyvalue_like_k=testrun.git_describe&has_keyvalue_like_v=%25glibc-debian-ppc64%25 How do I read this table? > The debian-ppc64 worker has GCC 11.2.0 and binutils 2.38 > https://builder.sourceware.org/buildbot/#/workers/10 > > But I don't see how the patch and the crash backtrace are related. It could be a change in relocation types used. Thanks, Florian ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Improve performance of IO locks 2022-08-16 10:08 ` Florian Weimer @ 2022-08-17 13:45 ` Mark Wielaard 2022-08-22 10:25 ` Florian Weimer 0 siblings, 1 reply; 16+ messages in thread From: Mark Wielaard @ 2022-08-17 13:45 UTC (permalink / raw) To: Florian Weimer Cc: Thomas Fitzsimmons, 'GNU C Library', Wilco Dijkstra, Frank Ch. Eigler Hi Florian, On Tue, Aug 16, 2022 at 12:08:03PM +0200, Florian Weimer wrote: > * Mark Wielaard: > > Yeah, I am actually surprised just reverting the patch fixed things. > > But looking at the bunsen data for glibc-debian-ppc64 does seem to > > show that the only thing changed is this particular patch. Runs before > > it pass, runs after show lots of fails. > > > > https://builder.sourceware.org/testruns/?has_keyvalue_like_k=testrun.git_describe&has_keyvalue_like_v=%25glibc-debian-ppc64%25 > > How do I read this table? Glad you ask! Although I think the mystery of the debian-ppc64 build failure has now been solved, the intent of the bunsenweb tool is to help figure out test result differences between builders. I have added Frank to the CC who can explain better than me. We'll also have a BoF at the GNU Tools Cauldron for suggestions on how the improve it and the integration with other sourceware services [*]. So the table above is all the builds that bunsen knows about for glibc-debian-ppc64. Note that currently we only have one ppc64[be] worker that does glibc builds. You can select a couple of builds and compare them. Pick the failing one, which was 498f51, and the last succeeding one. And compare the results, configs, etc. There are a couple of ways to explore the diffs. One is by looking at the buildbot "upload to bunsen" step and select that URL (this really should be improved to make this easier to find). This then gives you different "clusters" of similar (but possible different) other builds to compare to. e.g. look at https://builder.sourceware.org/testrun/498f51f327afdd7030516455b709a31a0038b432 Another is to look at all results for a particular builder (as above) or to look at all the glibc builds using the (sql) search form. e.g. https://builder.sourceware.org/testruns/?has_keyvalue_like_k=testrun.git_describe&has_keyvalue_like_v=%25glibc%25 And then pick various results by hand and hit "diff". The diffs are split up between groups of results, like the glibc test results or autoconf logs, etc. Clicking through will give you the actual config.log, the results.out files, etc. Cheers, Mark [*] https://gcc.gnu.org/wiki/cauldron2022#cauldron2022talks.Sourceware_GNU_Toolchain_Infrastructure_and_beyond ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Improve performance of IO locks 2022-08-17 13:45 ` Mark Wielaard @ 2022-08-22 10:25 ` Florian Weimer 2022-08-22 14:58 ` Frank Ch. Eigler 0 siblings, 1 reply; 16+ messages in thread From: Florian Weimer @ 2022-08-22 10:25 UTC (permalink / raw) To: Mark Wielaard Cc: Thomas Fitzsimmons, 'GNU C Library', Wilco Dijkstra, Frank Ch. Eigler * Mark Wielaard: > Hi Florian, > > On Tue, Aug 16, 2022 at 12:08:03PM +0200, Florian Weimer wrote: >> * Mark Wielaard: >> > Yeah, I am actually surprised just reverting the patch fixed things. >> > But looking at the bunsen data for glibc-debian-ppc64 does seem to >> > show that the only thing changed is this particular patch. Runs before >> > it pass, runs after show lots of fails. >> > >> > https://builder.sourceware.org/testruns/?has_keyvalue_like_k=testrun.git_describe&has_keyvalue_like_v=%25glibc-debian-ppc64%25 >> >> How do I read this table? > > Glad you ask! Although I think the mystery of the debian-ppc64 build > failure has now been solved, the intent of the bunsenweb tool is to > help figure out test result differences between builders. I have added > Frank to the CC who can explain better than me. We'll also have a BoF > at the GNU Tools Cauldron for suggestions on how the improve it and > the integration with other sourceware services [*]. > > So the table above is all the builds that bunsen knows about for > glibc-debian-ppc64. Note that currently we only have one ppc64[be] > worker that does glibc builds. > > You can select a couple of builds and compare them. Pick the failing > one, which was 498f51, and the last succeeding one. And compare the > results, configs, etc. > There are a couple of ways to explore the diffs. One is by looking at > the buildbot "upload to bunsen" step and select that URL (this really > should be improved to make this easier to find). This then gives you > different "clusters" of similar (but possible different) other builds > to compare to. e.g. look at > https://builder.sourceware.org/testrun/498f51f327afdd7030516455b709a31a0038b432 > > Another is to look at all results for a particular builder (as above) > or to look at all the glibc builds using the (sql) search form. e.g. > https://builder.sourceware.org/testruns/?has_keyvalue_like_k=testrun.git_describe&has_keyvalue_like_v=%25glibc%25 Thank you for these links, they surely look useful. I assume the zebra pattern in the table is not actually meaningful (it's too regular for that), it's just to improve readability, right? What confuses me still is that the run lists do not seem to have numbers with failure counts. Thanks, Florian ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Improve performance of IO locks 2022-08-22 10:25 ` Florian Weimer @ 2022-08-22 14:58 ` Frank Ch. Eigler 0 siblings, 0 replies; 16+ messages in thread From: Frank Ch. Eigler @ 2022-08-22 14:58 UTC (permalink / raw) To: Florian Weimer Cc: Mark Wielaard, Thomas Fitzsimmons, 'GNU C Library', Wilco Dijkstra Hi - > > Another is to look at all results for a particular builder (as above) > > or to look at all the glibc builds using the (sql) search form. e.g. > > https://builder.sourceware.org/testruns/?has_keyvalue_like_k=testrun.git_describe&has_keyvalue_like_v=%25glibc%25 > > Thank you for these links, they surely look useful. I assume the zebra > pattern in the table is not actually meaningful (it's too regular for > that), it's just to improve readability, right? Correct. > What confuses me still is that the run lists do not seem to have numbers > with failure counts. Yes, while one can view fail/etc. counts of all the test cases in one testrun [https://tinyurl.com/mry25ekt], there is no single view that contrasts such counts for multiple runs. It's coming. - FChE ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Improve performance of IO locks 2022-08-16 10:00 ` Mark Wielaard 2022-08-16 10:08 ` Florian Weimer @ 2022-08-16 11:19 ` Andreas Schwab 1 sibling, 0 replies; 16+ messages in thread From: Andreas Schwab @ 2022-08-16 11:19 UTC (permalink / raw) To: Mark Wielaard Cc: Florian Weimer, Thomas Fitzsimmons, 'GNU C Library', Wilco Dijkstra It's working fine here on openSUSE Tumbleweed: https://build.opensuse.org/package/live_build_log/home:Andreas_Schwab:glibc/glibc:testsuite/p/ppc64 -- Andreas Schwab, SUSE Labs, schwab@suse.de GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7 "And now for something completely different." ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Improve performance of IO locks 2022-08-16 7:31 ` Florian Weimer 2022-08-16 10:00 ` Mark Wielaard @ 2022-08-16 10:24 ` Wilco Dijkstra 2022-08-16 13:18 ` Thomas Fitzsimmons 1 sibling, 1 reply; 16+ messages in thread From: Wilco Dijkstra @ 2022-08-16 10:24 UTC (permalink / raw) To: Florian Weimer, Thomas Fitzsimmons; +Cc: Mark Wielaard, 'GNU C Library' Hi, >> I looked at elf/unload as an example; it's segfaulting in >> _dl_relocate_object, backtrace attached; not sure what else to check. > > I don't see this on powerpc64, with a toolchain based on GCC 8.2 and > binutils 2.30. I'm at a loss how these things could be related. I would not expect the dynamic linker to have changed at all, so one option is to check the binary is identical before/after my commit. If the dynamic linker somehow got some uses of SINGLE_THREAD_P then that might access TLS before it is setup. The other possibility is that the binary it is trying to link has corrupted relocations. It's hard to imagine how that could happen unless you use the new GLIBC to link an application and fileio fails to write out the data for the relocations. Cheers, Wilco ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Improve performance of IO locks 2022-08-16 10:24 ` Wilco Dijkstra @ 2022-08-16 13:18 ` Thomas Fitzsimmons 2022-08-16 14:31 ` Thomas Fitzsimmons 0 siblings, 1 reply; 16+ messages in thread From: Thomas Fitzsimmons @ 2022-08-16 13:18 UTC (permalink / raw) To: Wilco Dijkstra; +Cc: Florian Weimer, Mark Wielaard, 'GNU C Library' Hi Wilco, Wilco Dijkstra <Wilco.Dijkstra@arm.com> writes: >>> I looked at elf/unload as an example; it's segfaulting in >>> _dl_relocate_object, backtrace attached; not sure what else to check. >> >> I don't see this on powerpc64, with a toolchain based on GCC 8.2 and >> binutils 2.30. I'm at a loss how these things could be related. > > I would not expect the dynamic linker to have changed at all, so one > option is to check the binary is identical before/after my commit. If > the dynamic linker somehow got some uses of SINGLE_THREAD_P then that > might access TLS before it is setup. elf/ld64.so.1 is identical before and after your change. > The other possibility is that the binary it is trying to link has > corrupted relocations. It's hard to imagine how that could happen > unless you use the new GLIBC to link an application and fileio fails > to write out the data for the relocations. I don't know how to check this. I'll contact you off-list to set up temporary remote access for you. Thomas ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Improve performance of IO locks 2022-08-16 13:18 ` Thomas Fitzsimmons @ 2022-08-16 14:31 ` Thomas Fitzsimmons 2022-08-17 11:53 ` Mark Wielaard 0 siblings, 1 reply; 16+ messages in thread From: Thomas Fitzsimmons @ 2022-08-16 14:31 UTC (permalink / raw) To: Wilco Dijkstra; +Cc: Florian Weimer, Mark Wielaard, 'GNU C Library' Hi, Thomas Fitzsimmons <fitzsim@fitzsim.org> writes: > Wilco Dijkstra <Wilco.Dijkstra@arm.com> writes: > >>>> I looked at elf/unload as an example; it's segfaulting in >>>> _dl_relocate_object, backtrace attached; not sure what else to check. >>> >>> I don't see this on powerpc64, with a toolchain based on GCC 8.2 and >>> binutils 2.30. I'm at a loss how these things could be related. >> >> I would not expect the dynamic linker to have changed at all, so one >> option is to check the binary is identical before/after my commit. If >> the dynamic linker somehow got some uses of SINGLE_THREAD_P then that >> might access TLS before it is setup. > > elf/ld64.so.1 is identical before and after your change. > >> The other possibility is that the binary it is trying to link has >> corrupted relocations. It's hard to imagine how that could happen >> unless you use the new GLIBC to link an application and fileio fails >> to write out the data for the relocations. > > I don't know how to check this. I'll contact you off-list to set up > temporary remote access for you. Before setting this up, I checked the system's package list. This builder is running Debian unstable, without unattended upgrades, so some of the toolchain packages were out-of-date. I did an upgrade (388 packages) and the issue seems to have disappeared, so this was likely a toolchain bug that has since been fixed. I have the list of old and new package versions if anyone's interested. Anyway, I think glibc-debian-ppc64 builds will succeed now. Thomas ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Improve performance of IO locks 2022-08-16 14:31 ` Thomas Fitzsimmons @ 2022-08-17 11:53 ` Mark Wielaard 0 siblings, 0 replies; 16+ messages in thread From: Mark Wielaard @ 2022-08-17 11:53 UTC (permalink / raw) To: Thomas Fitzsimmons Cc: Wilco Dijkstra, Florian Weimer, 'GNU C Library' Hi Tom. On Tue, Aug 16, 2022 at 10:31:39AM -0400, Thomas Fitzsimmons wrote: > Before setting this up, I checked the system's package list. This > builder is running Debian unstable, without unattended upgrades, so some > of the toolchain packages were out-of-date. > > I did an upgrade (388 packages) and the issue seems to have disappeared, > so this was likely a toolchain bug that has since been fixed. I have > the list of old and new package versions if anyone's interested. > > Anyway, I think glibc-debian-ppc64 builds will succeed now. It did! https://builder.sourceware.org/buildbot/#builders/128/builds/182 Thanks, Mark ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2022-08-22 14:58 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-06-08 16:32 [PATCH] Improve performance of IO locks Wilco Dijkstra 2022-08-01 11:06 ` Wilco Dijkstra 2022-08-07 12:51 ` Cristian Rodríguez 2022-08-15 22:27 ` Mark Wielaard 2022-08-16 3:07 ` Thomas Fitzsimmons 2022-08-16 7:31 ` Florian Weimer 2022-08-16 10:00 ` Mark Wielaard 2022-08-16 10:08 ` Florian Weimer 2022-08-17 13:45 ` Mark Wielaard 2022-08-22 10:25 ` Florian Weimer 2022-08-22 14:58 ` Frank Ch. Eigler 2022-08-16 11:19 ` Andreas Schwab 2022-08-16 10:24 ` Wilco Dijkstra 2022-08-16 13:18 ` Thomas Fitzsimmons 2022-08-16 14:31 ` Thomas Fitzsimmons 2022-08-17 11:53 ` Mark Wielaard
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).