| Message ID | 20230704182402.1040962-1-siddhesh@sourceware.org |
|---|---|
| State | New |
| Headers | show |
| Series | realloc: Limit chunk reuse to only growing requests [BZ #30579] | expand |
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); >
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
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
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
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>
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);
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(-)