Re: [PATCH] Improve performance of IO locks

[Mark Wielaard <mark@klomp.org>]

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