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

[Nicolas Dusart <nicolas@freedelity.be>]

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