Re: [PATCH] mm/compaction: cap compact_gap() at COMPACT_CLUSTER_MAX

[Vlastimil Babka (SUSE)]

On 5/27/26 02:10, JP Kobryn wrote:
> On 5/25/26 3:02 AM, Vlastimil Babka (SUSE) wrote:
>> On 5/19/26 22:08, JP Kobryn (Meta) wrote:
>>> compact_gap() returns 2 << order, which is used as watermark headroom in
>>> __compaction_suitable() and as a reclaim target in kswapd. The computed
>>> value scales exponentially by order. For order-9 THP allocations this
>>> evaluates to 1024 pages, but the compaction free scanner's working set is
>>> bounded by COMPACT_CLUSTER_MAX (32 pages). The scanner stops 
>>> isolating free
>>> pages once it matches the migration batch. The current gap 
>>> over-reserves by
>>> 32x.
>>>
>>> On fragmented production hosts, kswapd will try and reclaim up to the 
>>> gap,
>>> but it only reaches that threshold 18% of the time, causing reclaim to
>>> continue a majority of the time.
>> But doesn't that mean there's genuine memory pressure? We're effectively
>> raising the high watermark by 4 MB, but if processes are continuously
>> allocating, we'd be reclaiming without the gap as well? Unless the 
>> workload
>> is sized to fit without the gap.
> 
> It wasn't actual pressure, but the repetitive order-9 THP failures that were
> waking up kswapd. I should make this more clear in the changelog. After
> looking into why so much reclaim was occurring though, the compact gap stood
> out since it dictates the target amount to reclaim.

But the "amount to reclaim" is still defined as "reach high watermark +
compact_gap()" and not "reclaim at least compact_gap() pages" right? Or did
I miss something non-obvious.

So if kswapd did any work, it means the memory was consumed (i.e. there was
some memory pressure) and amount of free memory was below high watermark +
compact_gap()?

BTW, are you using mglru here? (probably not)
As that might be different and I'm not so familiar with it.

>>> The over-sized gap also causes 46% of
>>> order-9 compaction suitability checks to fail unnecessarily - the 
>>> zone has
>>> sufficient free pages for the scanner to operate, but not enough to clear
>>> the inflated threshold.
>>>
>>> Cap compact_gap() at COMPACT_CLUSTER_MAX to align the watermark headroom
>>> with the scanner's actual capacity. Orders 0-4 are unaffected since their
>>> gap is <= 32.
>>>
>>> A/B test on ~100 instagram production hosts (64GB, 60s measurement):
>> What was the base kernel version?
> 
> 6.13. Additional benchmarks were done using a recent mm-new build as well,
> and they showed similar reductions in reclaim.

If it's a NUMA machine, we recently found an over-reclaim issue there fixed
by 9c9828d3ead6 ("mm, page_alloc, thp: prevent reclaim for __GFP_THISNODE
THP allocations")

>>> Unpatched (43 hosts)
>>> pgscan_kswapd (mean/host): ~1.6M
>>> reclaim efficiency (steal/scan): 83.8%
>>> compaction success (success/stall): 2.1%
>>> THP success (alloc/alloc+fallback): 4.9%
>>> forced lru_add_drain (mean/host): ~107K
>>>
>>> Patched (59 hosts)
>>> pgscan_kswapd (mean/host): ~449K
>> Did the extra reclaim just disappear because we allow the allocations 
>> to use
>> 4MB more memory? Or it shifted to direct reclaim?
> 
> Specifically in the order-9 case, the reclaim target goes from 1024 to 32.
> What the data shows is that capping the gap allows compaction to take over
> sooner and start working to produce large size pages needed for THP. Whereas
> in the pre-patch state, trying to reclaim the full 2x THP delays compaction.

So do I understand correctly we might have an issue due to lack of
hysteresis? We require reaching high watermark + compact_gap() to terminate
reclaim, but then compaction can find out we meanwhile dropped below that
(due to concurrent allocations) and it's not suitable again?

However the suitability checks e.g. compaction_zonelist_suitable() are using
min watermark, so that should provide the difference already.
Actually it's low watermark because of __compaction_suitable() adding an
extra low-min gap for costly orders. But still.

I did just notice compaction_ready() might be too strict. It wants
effectivly high wmark plus the gap plus the low-min difference. Is it
perhaps the underlying issue here?

>>> reclaim efficiency (steal/scan): 91.0%
>>> compaction success (success/stall): 28.3%
>> Is this compaction success per compaction stall or per alloc stall?
> 
> That's per compaction.
> 
>>> THP success (alloc/alloc+fallback): 17.2%
>> Weird that things would improve that much. I would expect the free memory
>> just to stabilize around the lower gap but then behave similarly. Are we
>> missing something here?
> 
> This patch was tested in isolation, but also occurring was the case where
> bursty net allocations reserve many pageblocks as high atomic. So as
> THP-size pages become eligible, their blocks are reserved before being
> allocated as THP.
> 
>>> forced lru_add_drain (mean/host): ~64K
>>>
>>> Signed-off-by: JP Kobryn (Meta) <jp.kobryn@xxxxxxxxx>
>>> ---
>>> include/linux/compaction.h | 8 ++++----
>>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/include/linux/compaction.h b/include/linux/compaction.h
>>> index 173d9c07a8952..09aea63b8a89d 100644
>>> --- a/include/linux/compaction.h
>>> +++ b/include/linux/compaction.h
>>> @@ -2,6 +2,8 @@
>>> #ifndef _LINUX_COMPACTION_H
>>> #define _LINUX_COMPACTION_H
>>> +#include <linux/swap.h>
>>> +
>>> /*
>>> * Determines how hard direct compaction should try to succeed.
>>> * Lower value means higher priority, analogically to reclaim priority.
>>> @@ -73,11 +75,9 @@ static inline unsigned long compact_gap(unsigned 
>>> int order)
>>> * effectively limited by COMPACT_CLUSTER_MAX, as that's the maximum
>>> * that the migrate scanner can have isolated on migrate list, and free
>>> * scanner is only invoked when the number of isolated free pages is
>>> - * lower than that. But it's not worth to complicate the formula here
>>> - * as a bigger gap for higher orders than strictly necessary can also
>>> - * improve chances of compaction success.
>>> + * lower than that.
>>> */
>>> - return 2UL << order;
>>> + return min(2UL << order, COMPACT_CLUSTER_MAX);
>> Shouldn't it at least be 2x COMPACT_CLUSTER_MAX?
> 
> I'm thinking I could reframe this patch as reclaim-focused and use
> min(2UL << order, COMPACT_CLUSTER_MAX) as a reclaim-only target, while
> either leaving the other non-reclaim users of this function alone or
> using the 2x form you suggest above. i.e. I can split this function
> into a separate reclaim_compact_gap() and use the originally proposed cap.
> Thoughts?

Do I understand correctly you want to cap the reclaim target by
COMPACT_CLUSTER_MAX but leave e.g. the compaction_suitable() usage as it is?
But wouldn't that mean we'll actually make changes of passing
compaction_suitable() worse?