Message ID | 87pm3uajev.fsf@oldenburg.str.redhat.com |
---|---|
State | New |
Headers | show |
Series | malloc: Remove bin scanning from memalign (bug 30723) | expand |
Hi Florian, This fails testing on aarch64-linux-gnu: FAIL: malloc/tst-memalign-2-malloc-hugetlb1 original exit status 1 error: tst-memalign-2.c:155: not true: count > LN / 2 error: 1 test failures FAIL: malloc/tst-memalign-2-malloc-hugetlb2 original exit status 1 error: tst-memalign-2.c:155: not true: count > LN / 2 error: 1 test failures FAIL: malloc/tst-memalign-2 original exit status 1 error: tst-memalign-2.c:155: not true: count > LN / 2 error: 1 test failures Let me know if you need any assistance in reproducing this. Thanks! -- Maxim Kuvyrkov https://www.linaro.org > On Aug 10, 2023, at 21:36, Florian Weimer via Libc-alpha <libc-alpha@sourceware.org> wrote: > > On the test workload (mpv --cache=yes with VP9 video decoding), the > bin scanning has a very poor success rate (less than 2%). The tcache > scanning has about 50% success rate, so keep that. > > --- > DJ, this is on top of my other patch. Given that the chunk scanning > code has such a small hit rate on the workload, I have just removed it. > > I think we need better data structures before we can bring this back, > otherwise most workloads with a high number of memalign calls suffer too > much. Typical heaps have hundreds, if not thousands, of free list > entries. I had not considered the impact of that on repeated memalign > calls. > > What do you think? > > Tested x86_64-linux-gnu. > > Thanks, > Florian > > malloc/malloc.c | 127 +++----------------------------------------------------- > 1 file changed, 5 insertions(+), 122 deletions(-) > > diff --git a/malloc/malloc.c b/malloc/malloc.c > index 948f9759af..9c2cab7a59 100644 > --- a/malloc/malloc.c > +++ b/malloc/malloc.c > @@ -5082,7 +5082,6 @@ _int_memalign (mstate av, size_t alignment, size_t bytes) > mchunkptr remainder; /* spare room at end to split off */ > unsigned long remainder_size; /* its size */ > INTERNAL_SIZE_T size; > - mchunkptr victim; > > nb = checked_request2size (bytes); > if (nb == 0) > @@ -5101,129 +5100,13 @@ _int_memalign (mstate av, size_t alignment, size_t bytes) > we don't find anything in those bins, the common malloc code will > scan starting at 2x. */ > > - /* This will be set if we found a candidate chunk. */ > - victim = NULL; > + /* Call malloc with worst case padding to hit alignment. */ > + m = (char *) (_int_malloc (av, nb + alignment + MINSIZE)); > > - /* Fast bins are singly-linked, hard to remove a chunk from the middle > - and unlikely to meet our alignment requirements. We have not done > - any experimentation with searching for aligned fastbins. */ > + if (m == 0) > + return 0; /* propagate failure */ > > - if (av != NULL) > - { > - int first_bin_index; > - int first_largebin_index; > - int last_bin_index; > - > - if (in_smallbin_range (nb)) > - first_bin_index = smallbin_index (nb); > - else > - first_bin_index = largebin_index (nb); > - > - if (in_smallbin_range (nb * 2)) > - last_bin_index = smallbin_index (nb * 2); > - else > - last_bin_index = largebin_index (nb * 2); > - > - first_largebin_index = largebin_index (MIN_LARGE_SIZE); > - > - int victim_index; /* its bin index */ > - > - for (victim_index = first_bin_index; > - victim_index < last_bin_index; > - victim_index ++) > - { > - victim = NULL; > - > - if (victim_index < first_largebin_index) > - { > - /* Check small bins. Small bin chunks are doubly-linked despite > - being the same size. */ > - > - mchunkptr fwd; /* misc temp for linking */ > - mchunkptr bck; /* misc temp for linking */ > - > - bck = bin_at (av, victim_index); > - fwd = bck->fd; > - while (fwd != bck) > - { > - if (chunk_ok_for_memalign (fwd, alignment, nb) > 0) > - { > - victim = fwd; > - > - /* Unlink it */ > - victim->fd->bk = victim->bk; > - victim->bk->fd = victim->fd; > - break; > - } > - > - fwd = fwd->fd; > - } > - } > - else > - { > - /* Check large bins. */ > - mchunkptr fwd; /* misc temp for linking */ > - mchunkptr bck; /* misc temp for linking */ > - mchunkptr best = NULL; > - size_t best_size = 0; > - > - bck = bin_at (av, victim_index); > - fwd = bck->fd; > - > - while (fwd != bck) > - { > - int extra; > - > - if (chunksize (fwd) < nb) > - break; > - extra = chunk_ok_for_memalign (fwd, alignment, nb); > - if (extra > 0 > - && (extra <= best_size || best == NULL)) > - { > - best = fwd; > - best_size = extra; > - } > - > - fwd = fwd->fd; > - } > - victim = best; > - > - if (victim != NULL) > - { > - unlink_chunk (av, victim); > - break; > - } > - } > - > - if (victim != NULL) > - break; > - } > - } > - > - /* Strategy: find a spot within that chunk that meets the alignment > - request, and then possibly free the leading and trailing space. > - This strategy is incredibly costly and can lead to external > - fragmentation if header and footer chunks are unused. */ > - > - if (victim != NULL) > - { > - p = victim; > - m = chunk2mem (p); > - set_inuse (p); > - if (av != &main_arena) > - set_non_main_arena (p); > - } > - else > - { > - /* Call malloc with worst case padding to hit alignment. */ > - > - m = (char *) (_int_malloc (av, nb + alignment + MINSIZE)); > - > - if (m == 0) > - return 0; /* propagate failure */ > - > - p = mem2chunk (m); > - } > + p = mem2chunk (m); > > if ((((unsigned long) (m)) % alignment) != 0) /* misaligned */ > { > > base-commit: e02ce52fd890d3b5197f78cf25096faa9446fa3d >
* Maxim Kuvyrkov: > Hi Florian, > > This fails testing on aarch64-linux-gnu: > > FAIL: malloc/tst-memalign-2-malloc-hugetlb1 > original exit status 1 > error: tst-memalign-2.c:155: not true: count > LN / 2 > error: 1 test failures > > FAIL: malloc/tst-memalign-2-malloc-hugetlb2 > original exit status 1 > error: tst-memalign-2.c:155: not true: count > LN / 2 > error: 1 test failures > > FAIL: malloc/tst-memalign-2 > original exit status 1 > error: tst-memalign-2.c:155: not true: count > LN / 2 > error: 1 test failures > > Let me know if you need any assistance in reproducing this. I can't reproduce this on x86-64, but looking at this particular subtest, I agree that it should fail after the removal of the bin scanning code. 8-/ I'll send a v2. Thanks, Florian
* Florian Weimer: > * Maxim Kuvyrkov: > >> Hi Florian, >> >> This fails testing on aarch64-linux-gnu: >> >> FAIL: malloc/tst-memalign-2-malloc-hugetlb1 >> original exit status 1 >> error: tst-memalign-2.c:155: not true: count > LN / 2 >> error: 1 test failures >> >> FAIL: malloc/tst-memalign-2-malloc-hugetlb2 >> original exit status 1 >> error: tst-memalign-2.c:155: not true: count > LN / 2 >> error: 1 test failures >> >> FAIL: malloc/tst-memalign-2 >> original exit status 1 >> error: tst-memalign-2.c:155: not true: count > LN / 2 >> error: 1 test failures >> >> Let me know if you need any assistance in reproducing this. > > I can't reproduce this on x86-64, but looking at this particular > subtest, I agree that it should fail after the removal of the bin > scanning code. 8-/ > > I'll send a v2. And I'll need a v3. The test is still valid with the return-to-lowest-allocator patches in memalign, and passes. But this commit here applies separately without the conflict, and then the test fails (as expected). Thanks, Florian
malloc.c: In function '_int_free': malloc.c:4496:13: error: unused variable 'fwd' [-Werror=unused-variable] 4496 | mchunkptr fwd; /* misc temp for linking */ | ^~~ malloc.c:4495:13: error: unused variable 'bck' [-Werror=unused-variable] 4495 | mchunkptr bck; /* misc temp for linking */ | ^~~ malloc.c:4494:19: error: unused variable 'prevsize' [-Werror=unused-variable] 4494 | INTERNAL_SIZE_T prevsize; /* size of previous contiguous chunk */ | ^~~~~~~~ malloc.c:4493:7: error: unused variable 'nextinuse' [-Werror=unused-variable] 4493 | int nextinuse; /* true if nextchunk is used */ | ^~~~~~~~~ malloc.c:4492:19: error: unused variable 'nextsize' [-Werror=unused-variable] 4492 | INTERNAL_SIZE_T nextsize; /* its size */ | ^~~~~~~~ malloc.c:4491:13: error: unused variable 'nextchunk' [-Werror=unused-variable] 4491 | mchunkptr nextchunk; /* next contiguous chunk */ | ^~~~~~~~~ cc1: all warnings being treated as errors make[2]: *** [../o-iterator.mk:9: /home/abuild/rpmbuild/BUILD/glibc-2.38.9000.31.g084fb31bc2/cc-base/malloc/malloc.o] Error 1
* Andreas Schwab: > malloc.c: In function '_int_free': > malloc.c:4496:13: error: unused variable 'fwd' [-Werror=unused-variable] > 4496 | mchunkptr fwd; /* misc temp for linking */ > | ^~~ > malloc.c:4495:13: error: unused variable 'bck' [-Werror=unused-variable] > 4495 | mchunkptr bck; /* misc temp for linking */ > | ^~~ > malloc.c:4494:19: error: unused variable 'prevsize' [-Werror=unused-variable] > 4494 | INTERNAL_SIZE_T prevsize; /* size of previous contiguous chunk */ > | ^~~~~~~~ > malloc.c:4493:7: error: unused variable 'nextinuse' [-Werror=unused-variable] > 4493 | int nextinuse; /* true if nextchunk is used */ > | ^~~~~~~~~ > malloc.c:4492:19: error: unused variable 'nextsize' [-Werror=unused-variable] > 4492 | INTERNAL_SIZE_T nextsize; /* its size */ > | ^~~~~~~~ > malloc.c:4491:13: error: unused variable 'nextchunk' [-Werror=unused-variable] > 4491 | mchunkptr nextchunk; /* next contiguous chunk */ > | ^~~~~~~~~ > cc1: all warnings being treated as errors > make[2]: *** [../o-iterator.mk:9: /home/abuild/rpmbuild/BUILD/glibc-2.38.9000.31.g084fb31bc2/cc-base/malloc/malloc.o] Error 1 This is a disable-tcache configuration, right? Will fix.
* Florian Weimer: > * Andreas Schwab: > >> malloc.c: In function '_int_free': >> malloc.c:4496:13: error: unused variable 'fwd' [-Werror=unused-variable] >> 4496 | mchunkptr fwd; /* misc temp for linking */ >> | ^~~ >> malloc.c:4495:13: error: unused variable 'bck' [-Werror=unused-variable] >> 4495 | mchunkptr bck; /* misc temp for linking */ >> | ^~~ >> malloc.c:4494:19: error: unused variable 'prevsize' [-Werror=unused-variable] >> 4494 | INTERNAL_SIZE_T prevsize; /* size of previous contiguous chunk */ >> | ^~~~~~~~ >> malloc.c:4493:7: error: unused variable 'nextinuse' [-Werror=unused-variable] >> 4493 | int nextinuse; /* true if nextchunk is used */ >> | ^~~~~~~~~ >> malloc.c:4492:19: error: unused variable 'nextsize' [-Werror=unused-variable] >> 4492 | INTERNAL_SIZE_T nextsize; /* its size */ >> | ^~~~~~~~ >> malloc.c:4491:13: error: unused variable 'nextchunk' [-Werror=unused-variable] >> 4491 | mchunkptr nextchunk; /* next contiguous chunk */ >> | ^~~~~~~~~ >> cc1: all warnings being treated as errors >> make[2]: *** [../o-iterator.mk:9: /home/abuild/rpmbuild/BUILD/glibc-2.38.9000.31.g084fb31bc2/cc-base/malloc/malloc.o] Error 1 > > This is a disable-tcache configuration, right? Will fix. No, the variables are always unused. This looks like a GCC bug on my side. It's strange that it appears with GCC 12 and GCC 13. Which version do you use?
This is openSUSE Tumbleweed, which uses gcc 13. https://build.opensuse.org/package/show/home:Andreas_Schwab:glibc/glibc
On Sat, 2023-08-12 at 15:33 +0200, Florian Weimer wrote: > * Florian Weimer: > > > * Andreas Schwab: > > > > > malloc.c: In function '_int_free': > > > malloc.c:4496:13: error: unused variable 'fwd' [-Werror=unused-variable] > > > 4496 | mchunkptr fwd; /* misc temp for linking */ > > > | ^~~ > > > malloc.c:4495:13: error: unused variable 'bck' [-Werror=unused-variable] > > > 4495 | mchunkptr bck; /* misc temp for linking */ > > > | ^~~ > > > malloc.c:4494:19: error: unused variable 'prevsize' [-Werror=unused-variable] > > > 4494 | INTERNAL_SIZE_T prevsize; /* size of previous contiguous chunk */ > > > | ^~~~~~~~ > > > malloc.c:4493:7: error: unused variable 'nextinuse' [-Werror=unused-variable] > > > 4493 | int nextinuse; /* true if nextchunk is used */ > > > | ^~~~~~~~~ > > > malloc.c:4492:19: error: unused variable 'nextsize' [-Werror=unused-variable] > > > 4492 | INTERNAL_SIZE_T nextsize; /* its size */ > > > | ^~~~~~~~ > > > malloc.c:4491:13: error: unused variable 'nextchunk' [-Werror=unused-variable] > > > 4491 | mchunkptr nextchunk; /* next contiguous chunk */ > > > | ^~~~~~~~~ > > > cc1: all warnings being treated as errors > > > make[2]: *** [../o-iterator.mk:9: /home/abuild/rpmbuild/BUILD/glibc-2.38.9000.31.g084fb31bc2/cc-base/malloc/malloc.o] Error 1 > > > > This is a disable-tcache configuration, right? Will fix. > > No, the variables are always unused. This looks like a GCC bug on my > side. It's strange that it appears with GCC 12 and GCC 13. Which > version do you use? But I just removed these variables in a patched (applying 542b1105852568c3ebc712225ae78b8c8ba31a78 and v3 of this) Glibc-2.38 and it builds OK. So I can bet they are really unused...
* Andreas Schwab: > malloc.c: In function '_int_free': > malloc.c:4496:13: error: unused variable 'fwd' [-Werror=unused-variable] > 4496 | mchunkptr fwd; /* misc temp for linking */ > | ^~~ > malloc.c:4495:13: error: unused variable 'bck' [-Werror=unused-variable] > 4495 | mchunkptr bck; /* misc temp for linking */ > | ^~~ > malloc.c:4494:19: error: unused variable 'prevsize' [-Werror=unused-variable] > 4494 | INTERNAL_SIZE_T prevsize; /* size of previous contiguous chunk */ > | ^~~~~~~~ > malloc.c:4493:7: error: unused variable 'nextinuse' [-Werror=unused-variable] > 4493 | int nextinuse; /* true if nextchunk is used */ > | ^~~~~~~~~ > malloc.c:4492:19: error: unused variable 'nextsize' [-Werror=unused-variable] > 4492 | INTERNAL_SIZE_T nextsize; /* its size */ > | ^~~~~~~~ > malloc.c:4491:13: error: unused variable 'nextchunk' [-Werror=unused-variable] > 4491 | mchunkptr nextchunk; /* next contiguous chunk */ > | ^~~~~~~~~ > cc1: all warnings being treated as errors > make[2]: *** [../o-iterator.mk:9: /home/abuild/rpmbuild/BUILD/glibc-2.38.9000.31.g084fb31bc2/cc-base/malloc/malloc.o] Error 1 Found it, I believe I'm not seeing this because of: commit 78ceef25d64efeeb6067d1cb282a00466e637e2a Author: Adhemerval Zanella Netto <adhemerval.zanella@linaro.org> Date: Mon Jul 24 14:22:27 2023 -0300 configure: Remove --enable-all-warnings option The option is not activelly tested and has bitrotten, to fix it would require a lot of work and multiple fixes. A better option would to evaluate each option and enable the warning if it makes sense. Reviewed-by: Siddhesh Poyarekar <siddhesh@sourceware.org> diff --git a/Makeconfig b/Makeconfig index 77d7fd14df..c4dd9ea8f2 100644 --- a/Makeconfig +++ b/Makeconfig @@ -857,12 +857,7 @@ host-test-program-cmd = $(host-built-program-cmd) endif # Extra flags to pass to GCC. -ifeq ($(all-warnings),yes) -+gccwarn := -Wall -Wwrite-strings -Wcast-qual -Wbad-function-cast -Wmissing-noreturn -Wmissing-prototypes -Wmissing-declarations -Wcomment -Wcomments -Wtrigraphs -Wsign-compare -Wfloat-equal -Wmultichar -else -+gccwarn := -Wall -Wwrite-strings -endif -+gccwarn += -Wundef +gccwarn := -Wall -Wwrite-strings -Wundef ifeq ($(enable-werror),yes) +gccwarn += -Werror endif It dropped -Wall from glibc's default CFLAGS due to a typo (gccwarn instead of +gccwarn). Will test a fix with build-many-glibcs.py. Thanks, Florian
On 14/08/23 06:16, Florian Weimer wrote: > * Andreas Schwab: > >> malloc.c: In function '_int_free': >> malloc.c:4496:13: error: unused variable 'fwd' [-Werror=unused-variable] >> 4496 | mchunkptr fwd; /* misc temp for linking */ >> | ^~~ >> malloc.c:4495:13: error: unused variable 'bck' [-Werror=unused-variable] >> 4495 | mchunkptr bck; /* misc temp for linking */ >> | ^~~ >> malloc.c:4494:19: error: unused variable 'prevsize' [-Werror=unused-variable] >> 4494 | INTERNAL_SIZE_T prevsize; /* size of previous contiguous chunk */ >> | ^~~~~~~~ >> malloc.c:4493:7: error: unused variable 'nextinuse' [-Werror=unused-variable] >> 4493 | int nextinuse; /* true if nextchunk is used */ >> | ^~~~~~~~~ >> malloc.c:4492:19: error: unused variable 'nextsize' [-Werror=unused-variable] >> 4492 | INTERNAL_SIZE_T nextsize; /* its size */ >> | ^~~~~~~~ >> malloc.c:4491:13: error: unused variable 'nextchunk' [-Werror=unused-variable] >> 4491 | mchunkptr nextchunk; /* next contiguous chunk */ >> | ^~~~~~~~~ >> cc1: all warnings being treated as errors >> make[2]: *** [../o-iterator.mk:9: /home/abuild/rpmbuild/BUILD/glibc-2.38.9000.31.g084fb31bc2/cc-base/malloc/malloc.o] Error 1 > > Found it, I believe I'm not seeing this because of: > > commit 78ceef25d64efeeb6067d1cb282a00466e637e2a > Author: Adhemerval Zanella Netto <adhemerval.zanella@linaro.org> > Date: Mon Jul 24 14:22:27 2023 -0300 > > configure: Remove --enable-all-warnings option > > The option is not activelly tested and has bitrotten, to fix it > would require a lot of work and multiple fixes. A better option > would to evaluate each option and enable the warning if it makes > sense. > Reviewed-by: Siddhesh Poyarekar <siddhesh@sourceware.org> > > diff --git a/Makeconfig b/Makeconfig > index 77d7fd14df..c4dd9ea8f2 100644 > --- a/Makeconfig > +++ b/Makeconfig > @@ -857,12 +857,7 @@ host-test-program-cmd = $(host-built-program-cmd) > endif > > # Extra flags to pass to GCC. > -ifeq ($(all-warnings),yes) > -+gccwarn := -Wall -Wwrite-strings -Wcast-qual -Wbad-function-cast -Wmissing-noreturn -Wmissing-prototypes -Wmissing-declarations -Wcomment -Wcomments -Wtrigraphs -Wsign-compare -Wfloat-equal -Wmultichar > -else > -+gccwarn := -Wall -Wwrite-strings > -endif > -+gccwarn += -Wundef > +gccwarn := -Wall -Wwrite-strings -Wundef > ifeq ($(enable-werror),yes) > +gccwarn += -Werror > endif > > It dropped -Wall from glibc's default CFLAGS due to a typo (gccwarn > instead of +gccwarn). > > Will test a fix with build-many-glibcs.py. Oops, sorry about that and thanks for fixing this up.
diff --git a/malloc/malloc.c b/malloc/malloc.c index 948f9759af..9c2cab7a59 100644 --- a/malloc/malloc.c +++ b/malloc/malloc.c @@ -5082,7 +5082,6 @@ _int_memalign (mstate av, size_t alignment, size_t bytes) mchunkptr remainder; /* spare room at end to split off */ unsigned long remainder_size; /* its size */ INTERNAL_SIZE_T size; - mchunkptr victim; nb = checked_request2size (bytes); if (nb == 0) @@ -5101,129 +5100,13 @@ _int_memalign (mstate av, size_t alignment, size_t bytes) we don't find anything in those bins, the common malloc code will scan starting at 2x. */ - /* This will be set if we found a candidate chunk. */ - victim = NULL; + /* Call malloc with worst case padding to hit alignment. */ + m = (char *) (_int_malloc (av, nb + alignment + MINSIZE)); - /* Fast bins are singly-linked, hard to remove a chunk from the middle - and unlikely to meet our alignment requirements. We have not done - any experimentation with searching for aligned fastbins. */ + if (m == 0) + return 0; /* propagate failure */ - if (av != NULL) - { - int first_bin_index; - int first_largebin_index; - int last_bin_index; - - if (in_smallbin_range (nb)) - first_bin_index = smallbin_index (nb); - else - first_bin_index = largebin_index (nb); - - if (in_smallbin_range (nb * 2)) - last_bin_index = smallbin_index (nb * 2); - else - last_bin_index = largebin_index (nb * 2); - - first_largebin_index = largebin_index (MIN_LARGE_SIZE); - - int victim_index; /* its bin index */ - - for (victim_index = first_bin_index; - victim_index < last_bin_index; - victim_index ++) - { - victim = NULL; - - if (victim_index < first_largebin_index) - { - /* Check small bins. Small bin chunks are doubly-linked despite - being the same size. */ - - mchunkptr fwd; /* misc temp for linking */ - mchunkptr bck; /* misc temp for linking */ - - bck = bin_at (av, victim_index); - fwd = bck->fd; - while (fwd != bck) - { - if (chunk_ok_for_memalign (fwd, alignment, nb) > 0) - { - victim = fwd; - - /* Unlink it */ - victim->fd->bk = victim->bk; - victim->bk->fd = victim->fd; - break; - } - - fwd = fwd->fd; - } - } - else - { - /* Check large bins. */ - mchunkptr fwd; /* misc temp for linking */ - mchunkptr bck; /* misc temp for linking */ - mchunkptr best = NULL; - size_t best_size = 0; - - bck = bin_at (av, victim_index); - fwd = bck->fd; - - while (fwd != bck) - { - int extra; - - if (chunksize (fwd) < nb) - break; - extra = chunk_ok_for_memalign (fwd, alignment, nb); - if (extra > 0 - && (extra <= best_size || best == NULL)) - { - best = fwd; - best_size = extra; - } - - fwd = fwd->fd; - } - victim = best; - - if (victim != NULL) - { - unlink_chunk (av, victim); - break; - } - } - - if (victim != NULL) - break; - } - } - - /* Strategy: find a spot within that chunk that meets the alignment - request, and then possibly free the leading and trailing space. - This strategy is incredibly costly and can lead to external - fragmentation if header and footer chunks are unused. */ - - if (victim != NULL) - { - p = victim; - m = chunk2mem (p); - set_inuse (p); - if (av != &main_arena) - set_non_main_arena (p); - } - else - { - /* Call malloc with worst case padding to hit alignment. */ - - m = (char *) (_int_malloc (av, nb + alignment + MINSIZE)); - - if (m == 0) - return 0; /* propagate failure */ - - p = mem2chunk (m); - } + p = mem2chunk (m); if ((((unsigned long) (m)) % alignment) != 0) /* misaligned */ {