[PATCH] realloc: Limit chunk reuse to only growing requests [BZ #30579]

[PATCH] realloc: Limit chunk reuse to only growing requests [BZ #30579]

[Siddhesh Poyarekar]

The trim_threshold is too aggressive a heuristic to decide if chunk
reuse is OK for reallocated memory; for repeated small, shrinking
allocations it leads to internal fragmentation and for repeated larger
allocations that fragmentation may blow up even worse due to the dynamic
nature of the threshold.

Limit reuse only when it is within the alignment padding, which is 2 *
size_t for heap allocations and a page size for mmapped allocations.
There's the added wrinkle of THP, but this fix ignores it for now,
pessimizing that case in favor of keeping fragmentation low.

This resolves BZ #30579.

Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
Reported-by: Nicolas Dusart <nicolas@freedelity.be>
Reported-by: Aurelien Jarno <aurelien@aurel32.net>
---

The test case in the bz seems fixed with this, bringing VSZ and RSS back
to ~40M from ~1G.  Aurelien, can you please test with plasma desktop?

Thanks,
Sid


 malloc/malloc.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/malloc/malloc.c b/malloc/malloc.c
index b8c0f4f580..e2f1a615a4 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -3417,16 +3417,23 @@ __libc_realloc (void *oldmem, size_t bytes)
   if (__glibc_unlikely (mtag_enabled))
     *(volatile char*) oldmem;
 
-  /* Return the chunk as is whenever possible, i.e. there's enough usable space
-     but not so much that we end up fragmenting the block.  We use the trim
-     threshold as the heuristic to decide the latter.  */
-  size_t usable = musable (oldmem);
-  if (bytes <= usable
-      && (unsigned long) (usable - bytes) <= mp_.trim_threshold)
-    return oldmem;
-
   /* chunk corresponding to oldmem */
   const mchunkptr oldp = mem2chunk (oldmem);
+
+  /* Return the chunk as is if the request grows within usable bytes, typically
+     into the alignment padding.  We want to avoid reusing the block for
+     shrinkages because it ends up unnecessarily fragmenting the address space.
+     This is also why the heuristic misses alignment padding for THP for
+     now.  */
+  size_t usable = musable (oldmem);
+  if (bytes <= usable)
+    {
+      size_t difference = usable - bytes;
+      if ((unsigned long) difference < 2 * sizeof (INTERNAL_SIZE_T)
+	  || (chunk_is_mmapped (oldp) && difference <= GLRO (dl_pagesize)))
+	return oldmem;
+    }
+
   /* its size */
   const INTERNAL_SIZE_T oldsize = chunksize (oldp);
 
-- 
2.41.0

Re: [PATCH] realloc: Limit chunk reuse to only growing requests [BZ #30579]

[Nicolas Dusart]

Thanks for having reconsidered the issue and for the quick fix.
I can confirm this patch will resolve the issue we have in some of our
processes.

My main concern was not that we hadn't any way to mitigate this but
that this was a transparent change that will very likely impact a lot
of projects and teams in the future years. I was not expecting this to
impact desktop environment and happen this soon. I was expecting that
this would make problems in server environment where it is likely that
some processes cache large datasets and suddenly suffer from a big
raise in memory usage. As these environments tends to stick to stable
distributions, it may take some time before they use latest version of
the libc.
They could use another allocator but still, they would have had to
debug it down to point the libc as the source of that change.

Thanks Aurélien for the report, it confirms the impact it may had :)

Siddhesh, if you happen to find an heuristic that is suitable and can
save reallocations for bigger shrinks, may I suggest to avoid reusing
an option if the new behavior of this option does not fit exactly in
the expectation of how it worked earlier ?
After the addition of trim_threshold in realloc, trim_threshold was now
used as a way to tweak how the top of the heap may grow before
releasing it to the system, and as a way to tweak how big internal
chunks of memory can be left unused and still be acceptable.
This prevent us to enjoy one of this optimization given the other is
not applicable in one of our use case.

I'd argue that if that is appropriate as a change, it is because the
first optimisation (trim for the top of heap only) can be considered as
ineffective if the second optimisation is not applied too.
But I think that you'll agree that this is not the case, the
optimisation had already an appreciable effect.
So why preventing libc users to opt-out from the new change and still
enjoy the older behavior that was available for them ?

That was my main concerns when reporting that issue. Following the
principle of least astonishment, in a way.

Still, the patch is greatly appreciated.
Thanks for that and all your work on the glibc.

Kind Regards,

On Tue, 2023-07-04 at 14:24 -0400, Siddhesh Poyarekar wrote:
> The trim_threshold is too aggressive a heuristic to decide if chunk
> reuse is OK for reallocated memory; for repeated small, shrinking
> allocations it leads to internal fragmentation and for repeated
> larger
> allocations that fragmentation may blow up even worse due to the
> dynamic
> nature of the threshold.
> 
> Limit reuse only when it is within the alignment padding, which is 2
> *
> size_t for heap allocations and a page size for mmapped allocations.
> There's the added wrinkle of THP, but this fix ignores it for now,
> pessimizing that case in favor of keeping fragmentation low.
> 
> This resolves BZ #30579.
> 
> Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
> Reported-by: Nicolas Dusart <nicolas@freedelity.be>
> Reported-by: Aurelien Jarno <aurelien@aurel32.net>
> ---
> 
> The test case in the bz seems fixed with this, bringing VSZ and RSS
> back
> to ~40M from ~1G.  Aurelien, can you please test with plasma desktop?
> 
> Thanks,
> Sid
> 
> 
>  malloc/malloc.c | 23 +++++++++++++++--------
>  1 file changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/malloc/malloc.c b/malloc/malloc.c
> index b8c0f4f580..e2f1a615a4 100644
> --- a/malloc/malloc.c
> +++ b/malloc/malloc.c
> @@ -3417,16 +3417,23 @@ __libc_realloc (void *oldmem, size_t bytes)
>    if (__glibc_unlikely (mtag_enabled))
>      *(volatile char*) oldmem;
>  
> -  /* Return the chunk as is whenever possible, i.e. there's enough
> usable space
> -     but not so much that we end up fragmenting the block.  We use
> the trim
> -     threshold as the heuristic to decide the latter.  */
> -  size_t usable = musable (oldmem);
> -  if (bytes <= usable
> -      && (unsigned long) (usable - bytes) <= mp_.trim_threshold)
> -    return oldmem;
> -
>    /* chunk corresponding to oldmem */
>    const mchunkptr oldp = mem2chunk (oldmem);
> +
> +  /* Return the chunk as is if the request grows within usable
> bytes, typically
> +     into the alignment padding.  We want to avoid reusing the block
> for
> +     shrinkages because it ends up unnecessarily fragmenting the
> address space.
> +     This is also why the heuristic misses alignment padding for THP
> for
> +     now.  */
> +  size_t usable = musable (oldmem);
> +  if (bytes <= usable)
> +    {
> +      size_t difference = usable - bytes;
> +      if ((unsigned long) difference < 2 * sizeof (INTERNAL_SIZE_T)
> +         || (chunk_is_mmapped (oldp) && difference <= GLRO
> (dl_pagesize)))
> +       return oldmem;
> +    }
> +
>    /* its size */
>    const INTERNAL_SIZE_T oldsize = chunksize (oldp);
>  

Re: [PATCH] realloc: Limit chunk reuse to only growing requests [BZ #30579]

[Siddhesh Poyarekar]

On 2023-07-05 03:08, Nicolas Dusart wrote:
> Siddhesh, if you happen to find an heuristic that is suitable and can
> save reallocations for bigger shrinks, may I suggest to avoid reusing
> an option if the new behavior of this option does not fit exactly in
> the expectation of how it worked earlier ?

FWIW, the original optimization was not for shrinks; the shrinks came in 
as a side-effect and I thought it would be clever to allow shrinking up 
to trim threshold and didn't anticipate the broad impact then.  A number 
of distributions tend to rebase early and I had expected applications 
like redis to stumble over if anything was amiss, which didn't happen. 
I reckon the plasma desktop issue didn't get caught early because the 
proactive rebasers (Fedora, Suse Tumbleweed and Ubuntu) are primarily 
Gnome based.

In any case, this patch limits the optimization to growths only, which 
is far more convenient to reason because it grows into existing unused 
padding.  There's no real benefit to keeping parts of a block around in 
case of a shrinking allocation AFAICT.  This growth-into-padding 
optimization is also useful only because it's a relatively lightweight 
check; if it gets any more complex then it's probably not worth the effort.

Sid

Re: [PATCH] realloc: Limit chunk reuse to only growing requests [BZ #30579]

[Aurelien Jarno]

Hi,

On 2023-07-04 14:24, Siddhesh Poyarekar via Libc-alpha wrote:
> The trim_threshold is too aggressive a heuristic to decide if chunk
> reuse is OK for reallocated memory; for repeated small, shrinking
> allocations it leads to internal fragmentation and for repeated larger
> allocations that fragmentation may blow up even worse due to the dynamic
> nature of the threshold.
> 
> Limit reuse only when it is within the alignment padding, which is 2 *
> size_t for heap allocations and a page size for mmapped allocations.
> There's the added wrinkle of THP, but this fix ignores it for now,
> pessimizing that case in favor of keeping fragmentation low.
> 
> This resolves BZ #30579.
> 
> Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
> Reported-by: Nicolas Dusart <nicolas@freedelity.be>
> Reported-by: Aurelien Jarno <aurelien@aurel32.net>
> ---
> 
> The test case in the bz seems fixed with this, bringing VSZ and RSS back
> to ~40M from ~1G.  Aurelien, can you please test with plasma desktop?
 
Thanks for the quick fix. I confirm that this patch fixes the issue with
plasma desktop.

Regards
Aurelien

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                     http://aurel32.net

Re: [PATCH] realloc: Limit chunk reuse to only growing requests [BZ #30579]

[Siddhesh Poyarekar]

On 2023-07-05 07:55, Aurelien Jarno wrote:
> Hi,
> 
> On 2023-07-04 14:24, Siddhesh Poyarekar via Libc-alpha wrote:
>> The trim_threshold is too aggressive a heuristic to decide if chunk
>> reuse is OK for reallocated memory; for repeated small, shrinking
>> allocations it leads to internal fragmentation and for repeated larger
>> allocations that fragmentation may blow up even worse due to the dynamic
>> nature of the threshold.
>>
>> Limit reuse only when it is within the alignment padding, which is 2 *
>> size_t for heap allocations and a page size for mmapped allocations.
>> There's the added wrinkle of THP, but this fix ignores it for now,
>> pessimizing that case in favor of keeping fragmentation low.
>>
>> This resolves BZ #30579.
>>
>> Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
>> Reported-by: Nicolas Dusart <nicolas@freedelity.be>
>> Reported-by: Aurelien Jarno <aurelien@aurel32.net>
>> ---
>>
>> The test case in the bz seems fixed with this, bringing VSZ and RSS back
>> to ~40M from ~1G.  Aurelien, can you please test with plasma desktop?
>   
> Thanks for the quick fix. I confirm that this patch fixes the issue with
> plasma desktop.

Thanks, if you think the fix is OK, can you please give a Reviewed-by? 
I will then backport this to the 2.37 branch as well.

Thanks,
Sid

Re: [PATCH] realloc: Limit chunk reuse to only growing requests [BZ #30579]

[Aurelien Jarno]

On 2023-07-05 10:37, Siddhesh Poyarekar via Libc-alpha wrote:
> On 2023-07-05 07:55, Aurelien Jarno wrote:
> > Hi,
> > 
> > On 2023-07-04 14:24, Siddhesh Poyarekar via Libc-alpha wrote:
> > > The trim_threshold is too aggressive a heuristic to decide if chunk
> > > reuse is OK for reallocated memory; for repeated small, shrinking
> > > allocations it leads to internal fragmentation and for repeated larger
> > > allocations that fragmentation may blow up even worse due to the dynamic
> > > nature of the threshold.
> > > 
> > > Limit reuse only when it is within the alignment padding, which is 2 *
> > > size_t for heap allocations and a page size for mmapped allocations.
> > > There's the added wrinkle of THP, but this fix ignores it for now,
> > > pessimizing that case in favor of keeping fragmentation low.
> > > 
> > > This resolves BZ #30579.
> > > 
> > > Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
> > > Reported-by: Nicolas Dusart <nicolas@freedelity.be>
> > > Reported-by: Aurelien Jarno <aurelien@aurel32.net>
> > > ---
> > > 
> > > The test case in the bz seems fixed with this, bringing VSZ and RSS back
> > > to ~40M from ~1G.  Aurelien, can you please test with plasma desktop?
> > Thanks for the quick fix. I confirm that this patch fixes the issue with
> > plasma desktop.
> 
> Thanks, if you think the fix is OK, can you please give a Reviewed-by? I
> will then backport this to the 2.37 branch as well.

Yep, of course.

Reviewed-by: Aurelien Jarno <aurelien@aurel32.net>
Tested-by: Aurelien Jarno <aurelien@aurel32.net>

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                     http://aurel32.net