[PATCH] malloc: Remove bin scanning from memalign (bug 30723)

[PATCH] malloc: Remove bin scanning from memalign (bug 30723)

[Florian Weimer]

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

Re: [PATCH] malloc: Remove bin scanning from memalign (bug 30723)

[DJ Delorie]

My alternative idea for this is a tunable to limit the depth of the
search, but I can't say how well that would work.  We would really need
more metadata to search effectively.

So yeah, I guess reverting this part of it makes sense.

Re: [PATCH] malloc: Remove bin scanning from memalign (bug 30723)

[Florian Weimer]

* DJ Delorie:

> My alternative idea for this is a tunable to limit the depth of the
> search, but I can't say how well that would work.  We would really need
> more metadata to search effectively.
>
> So yeah, I guess reverting this part of it makes sense.

Should I commit both patches then?

Thanks,
Florian

Re: [PATCH] malloc: Remove bin scanning from memalign (bug 30723)

[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.

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
> 

Re: [PATCH] malloc: Remove bin scanning from memalign (bug 30723)

[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.

Thanks,
Florian

Re: [PATCH] malloc: Remove bin scanning from memalign (bug 30723)

[Florian Weimer]

* 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

Re: [PATCH] malloc: Remove bin scanning from memalign (bug 30723)

[DJ Delorie]

Florian Weimer <fweimer@redhat.com> writes:
>> So yeah, I guess reverting this part of it makes sense.
>
> Should I commit both patches then?

Yes please.

Re: [PATCH] malloc: Remove bin scanning from memalign (bug 30723)

[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

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH] malloc: Remove bin scanning from memalign (bug 30723)

[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.

Re: [PATCH] malloc: Remove bin scanning from memalign (bug 30723)

[Florian Weimer]

* 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?

Re: [PATCH] malloc: Remove bin scanning from memalign (bug 30723)

[Andreas Schwab]

This is openSUSE Tumbleweed, which uses gcc 13.

https://build.opensuse.org/package/show/home:Andreas_Schwab:glibc/glibc

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH] malloc: Remove bin scanning from memalign (bug 30723)

[Xi Ruoyao]

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...

-- 
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University

Re: [PATCH] malloc: Remove bin scanning from memalign (bug 30723)

[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

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

Re: [PATCH] malloc: Remove bin scanning from memalign (bug 30723)

[Adhemerval Zanella Netto]

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.

Re: [PATCH] malloc: Remove bin scanning from memalign (bug 30723)

[Sam James]

Florian Weimer via Libc-alpha <libc-alpha@sourceware.org> writes:

> * DJ Delorie:
>
>> My alternative idea for this is a tunable to limit the depth of the
>> search, but I can't say how well that would work.  We would really need
>> more metadata to search effectively.
>>
>> So yeah, I guess reverting this part of it makes sense.
>
> Should I commit both patches then?

[sorry, I couldn't find the email where you first mentioned
backporting.]

Could you push to 2.38? I've not had any issues myself yet
on my personal machines with the two patches and for good or
ill, Arch already pushed these to the masses on the day you
committed them, so I suspect they're ready enough.

Re: [PATCH] malloc: Remove bin scanning from memalign (bug 30723)

[Florian Weimer]

* Sam James:

> Florian Weimer via Libc-alpha <libc-alpha@sourceware.org> writes:
>
>> * DJ Delorie:
>>
>>> My alternative idea for this is a tunable to limit the depth of the
>>> search, but I can't say how well that would work.  We would really need
>>> more metadata to search effectively.
>>>
>>> So yeah, I guess reverting this part of it makes sense.
>>
>> Should I commit both patches then?
>
> [sorry, I couldn't find the email where you first mentioned
> backporting.]
>
> Could you push to 2.38? I've not had any issues myself yet
> on my personal machines with the two patches and for good or
> ill, Arch already pushed these to the masses on the day you
> committed them, so I suspect they're ready enough.

Testing our build failed, and someone said that it was a genuine failure
and not an infrastructure issue.  I'd like to investigate first if this
was caused by the malloc patches.

Thanks,
Florian

Re: [PATCH] malloc: Remove bin scanning from memalign (bug 30723)

[DJ Delorie]

Florian Weimer <fweimer@redhat.com> writes:
> 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.
>
> Update comments in malloc/tst-memalign-2 to indicate the purpose
> of the tests.  Even with the scanning removed, the additional
> merging opportunities since commit 542b1105852568c3ebc712225ae78b
> ("malloc: Enable merging of remainders in memalign (bug 30723)")
> are sufficient to pass the existing large bins test.
>
> Remove leftover variables from _int_free from refactoring in the
> same commit.

LGTM
Reviewed-by: DJ Delorie <dj@redhat.com>

> -  mchunkptr nextchunk;         /* next contiguous chunk */
> -  INTERNAL_SIZE_T nextsize;    /* its size */
> -  int nextinuse;               /* true if nextchunk is used */
> -  INTERNAL_SIZE_T prevsize;    /* size of previous contiguous chunk */
> -  mchunkptr bck;               /* misc temp for linking */
> -  mchunkptr fwd;               /* misc temp for linking */

No longer needed in this scope, ok.

> @@ -5032,42 +5026,6 @@ _int_realloc (mstate av, mchunkptr oldp, INTERNAL_SIZE_T oldsize,
>     ------------------------------ memalign ------------------------------
>   */
>  
> -/* Returns 0 if the chunk is not and does not contain the requested
> -   aligned sub-chunk, else returns the amount of "waste" from
> -   trimming.  NB is the *chunk* byte size, not the user byte
> -   size.  */
> -static size_t
> -chunk_ok_for_memalign (mchunkptr p, size_t alignment, size_t nb)
> -{
>   . . .
> -}

Only used for bin scanning, so no longer needed.  Ok.

> -  mchunkptr victim;

No longer needed, ok.

> -  /* 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));

Consistent with what the remaining code expects, ok.

> +  if (m == 0)
> +    return 0;           /* propagate failure */

Ok.

> -  /* 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 (av != NULL)
> -    {
>   . . .
> -    }

Ok.

> -  /* 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);
> -    }

No longer needed, ok.

> +  p = mem2chunk (m);

Ok.

> diff --git a/malloc/tst-memalign-2.c b/malloc/tst-memalign-2.c
> index f229283dbf..ecd6fa249e 100644
> --- a/malloc/tst-memalign-2.c
> +++ b/malloc/tst-memalign-2.c
> @@ -86,7 +86,8 @@ do_test (void)
>        TEST_VERIFY (tcache_allocs[i].ptr1 == tcache_allocs[i].ptr2);
>      }
>  
> -  /* Test for non-head tcache hits.  */
> +  /* Test for non-head tcache hits.  This exercises the memalign
> +     scanning code to find matching allocations.  */
>    for (i = 0; i < array_length (ptr); ++ i)
>      {
>        if (i == 4)
> @@ -113,7 +114,9 @@ do_test (void)
>    free (p);
>    TEST_VERIFY (count > 0);
>  
> -  /* Large bins test.  */
> +  /* Large bins test.  This verifies that the over-allocated parts
> +     that memalign releases for future allocations can be reused by
> +     memalign itself at least in some cases.  */
>  
>    for (i = 0; i < LN; ++ i)
>      {

Ok.

[PATCH] malloc: Remove bin scanning from memalign (bug 30723)

[Florian Weimer]

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.

Update comments in malloc/tst-memalign-2 to indicate the purpose
of the tests.  Even with the scanning removed, the additional
merging opportunities since commit 542b1105852568c3ebc712225ae78b
("malloc: Enable merging of remainders in memalign (bug 30723)")
are sufficient to pass the existing large bins test.

Remove leftover variables from _int_free from refactoring in the
same commit.

---
v4: Fix -Wall issues.
 malloc/malloc.c         | 169 ++----------------------------------------------
 malloc/tst-memalign-2.c |   7 +-
 2 files changed, 10 insertions(+), 166 deletions(-)

diff --git a/malloc/malloc.c b/malloc/malloc.c
index 948f9759af..d0bbbf3710 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -4488,12 +4488,6 @@ _int_free (mstate av, mchunkptr p, int have_lock)
 {
   INTERNAL_SIZE_T size;        /* its size */
   mfastbinptr *fb;             /* associated fastbin */
-  mchunkptr nextchunk;         /* next contiguous chunk */
-  INTERNAL_SIZE_T nextsize;    /* its size */
-  int nextinuse;               /* true if nextchunk is used */
-  INTERNAL_SIZE_T prevsize;    /* size of previous contiguous chunk */
-  mchunkptr bck;               /* misc temp for linking */
-  mchunkptr fwd;               /* misc temp for linking */
 
   size = chunksize (p);
 
@@ -5032,42 +5026,6 @@ _int_realloc (mstate av, mchunkptr oldp, INTERNAL_SIZE_T oldsize,
    ------------------------------ memalign ------------------------------
  */
 
-/* Returns 0 if the chunk is not and does not contain the requested
-   aligned sub-chunk, else returns the amount of "waste" from
-   trimming.  NB is the *chunk* byte size, not the user byte
-   size.  */
-static size_t
-chunk_ok_for_memalign (mchunkptr p, size_t alignment, size_t nb)
-{
-  void *m = chunk2mem (p);
-  INTERNAL_SIZE_T size = chunksize (p);
-  void *aligned_m = m;
-
-  if (__glibc_unlikely (misaligned_chunk (p)))
-    malloc_printerr ("_int_memalign(): unaligned chunk detected");
-
-  aligned_m = PTR_ALIGN_UP (m, alignment);
-
-  INTERNAL_SIZE_T front_extra = (intptr_t) aligned_m - (intptr_t) m;
-
-  /* We can't trim off the front as it's too small.  */
-  if (front_extra > 0 && front_extra < MINSIZE)
-    return 0;
-
-  /* If it's a perfect fit, it's an exception to the return value rule
-     (we would return zero waste, which looks like "not usable"), so
-     handle it here by returning a small non-zero value instead.  */
-  if (size == nb && front_extra == 0)
-    return 1;
-
-  /* If the block we need fits in the chunk, calculate total waste.  */
-  if (size > nb + front_extra)
-    return size - nb;
-
-  /* Can't use this chunk.  */
-  return 0;
-}
-
 /* BYTES is user requested bytes, not requested chunksize bytes.  */
 static void *
 _int_memalign (mstate av, size_t alignment, size_t bytes)
@@ -5082,7 +5040,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 +5058,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 */
     {
diff --git a/malloc/tst-memalign-2.c b/malloc/tst-memalign-2.c
index f229283dbf..ecd6fa249e 100644
--- a/malloc/tst-memalign-2.c
+++ b/malloc/tst-memalign-2.c
@@ -86,7 +86,8 @@ do_test (void)
       TEST_VERIFY (tcache_allocs[i].ptr1 == tcache_allocs[i].ptr2);
     }
 
-  /* Test for non-head tcache hits.  */
+  /* Test for non-head tcache hits.  This exercises the memalign
+     scanning code to find matching allocations.  */
   for (i = 0; i < array_length (ptr); ++ i)
     {
       if (i == 4)
@@ -113,7 +114,9 @@ do_test (void)
   free (p);
   TEST_VERIFY (count > 0);
 
-  /* Large bins test.  */
+  /* Large bins test.  This verifies that the over-allocated parts
+     that memalign releases for future allocations can be reused by
+     memalign itself at least in some cases.  */
 
   for (i = 0; i < LN; ++ i)
     {

base-commit: 542b1105852568c3ebc712225ae78b8c8ba31a78