Re: [PATCH v4] mm/page_alloc: avoid direct compaction for costly __GFP_NORETRY allocations

[Vlastimil Babka (SUSE)]

On 9/11/26 17:59, Johannes Weiner wrote:
> On Thu, Sep 10, 2026 at 11:46:02AM +0000, Salvatore Dipietro wrote:
>> 
>> On Sat, 05 Sep 2026 17:42:39 -0700 Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
>> 
>> > Is there anything particularly unusual about this test case?
>> 
>> It is a stock pgbench simple-update PostgreSQL workload on a large
>> instance (96 vCPUs), using standard PostgreSQL settings and with no huge
>> pages assigned to the database. We deliberately overprovision the
>> pgbench clients: 1024 clients over 96 threads. That keeps enough writers
>> in the buffered write path concurrently to hit the costly-order
>> allocation failure path continuously. The memory fragmentation comes from 
>> page tables: PostgreSQL spawns a new process per client, and those page
>> tables consume ~40% of memory, which significantly limits the page cache
>> and the free memory available.
>> 
>> 
>> > > Results (average of 3 runs, TPS):
>> > >
>> > >   Config                   Avg TPS      % vs Baseline
>> > >   baseline (no patch)       59,408       -
>> > >   With this patch          155,409      +161.6%
>> >
>> > Is this back to pre-5d8edfb900d5 performance?
>> 
>> Yes - fully recovered. Here is the summary, same host and workload
>> throughout, average of 3 runs:
>> 
>>   Config                                        Avg TPS   % vs baseline
>>   AL2023 stock 6.1 kernel                       136,942            n/a
>>   v7.3-rc1 baseline (no patch)                   59,408             -
>>   v7.3-rc1, 5d8edfb900d5 behaviour reverted     151,184        +154.5%
>>   v7.3-rc1 + v4                                 155,409        +161.6%
>> 
>> The 6.1 row predates 5d8edfb900d5 entirely. A different kernel version,
>> so not directly comparable, but it shows the performance this workload
>> used to get on this host.
>> 
>> A literal "git revert 5d8edfb900d5" does not apply to v7.3-rc1 -
>> iomap_write_iter() has been rewritten since - so the reverted row is a
>> one-line behavioural revert, forcing the write loop back to copying at
>> most PAGE_SIZE per iteration:
>> 
>>   -	size_t chunk = mapping_max_folio_size(mapping);
>>   +	size_t chunk = PAGE_SIZE;
>> 
>> which is what the pre-5d8edfb900d5 loop computed. That makes
>> iomap_get_folio() pass no order hint, so the path issues only order-0
>> allocations.
>> 
>> 
>> > AI review asked a few serious-looking questions:
>> > 	https://sashiko.dev/#/patchset/20260904115629.3993331-1-dipiets@xxxxxxxxx
>> 
>> Thanks for pointing that out. To address them, we can have something
>> like the patch below. Performance results are still similar to v4. Happy
>> to submit a formal v5 patch with it if you would like.
> 
> The feedback looks misleading to me.

Agreed.

> ALLOC_NON_BLOCKING itself is fine, it usually only means something in
> conjunction with other flags, like __GFP_HIGH.

+1

> The access to
> MIGRATE_HIGHATOMIC that it points out is in itself too generous. This
> seems like a real but separate bug. It will allow GFP_TRANSHUGE_LIGHT
> into the highatomic reserves as well, for example.

I don't follow this part. For ALLOC_HIGHATOMIC you need __GFP_HIGH in
alloc_flags_nonblocking(). So GFP_TRANSHUGE_LIGHT won't get the access, no?

> The rt issue seems made up too. We're talking about costly_order &&
> __GFP_NORETRY allocations. Why would they need access to reserves?

Yep.

And the concern about warning is also moot as __GFP_NORETRY is still needed
and its users virtually always add __GFP_NOWARN as well.

>>   Config                                        Avg TPS   % vs baseline
>>   v7.3-rc1 baseline (no patch)                   59,408             -
>>   v7.3-rc1 + v4                                 155,409        +161.6%
>>   v7.3-rc1 + proposed patch                     161,994        +172.7%
>> 
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index 12fac9084c48..be8b0d72a2db 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -4784,10 +4784,22 @@ static inline struct page *
>>  __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>>  						struct alloc_context *ac)
>>  {
>> -	bool can_direct_reclaim = gfp_mask & __GFP_DIRECT_RECLAIM;
>> +	const bool costly_order = order > PAGE_ALLOC_COSTLY_ORDER;
>> +	/*
>> +	 * Costly __GFP_NORETRY callers have a cheap fallback to a lower order,
>> +	 * so don't stall them in direct reclaim or direct compaction.  Exempt
>> +	 * __GFP_THISNODE (the THP attempt from alloc_pages_mpol() needs direct
>> +	 * compaction) and __GFP_NOFAIL (must not be made to fail).  Don't
>> +	 * clear __GFP_DIRECT_RECLAIM from gfp_mask instead: that would also
>> +	 * change the alloc_flags derived by alloc_flags_slowpath().
>> +	 */
>> +	const bool costly_noretry = costly_order &&
>> +		(gfp_mask & __GFP_NORETRY) &&
>> +		!(gfp_mask & (__GFP_THISNODE | __GFP_NOFAIL));
>> +	bool can_direct_reclaim = !costly_noretry &&
>> +		(gfp_mask & __GFP_DIRECT_RECLAIM);
> 
> IMO this is worse because now there is a disconnect between these
> flags and the gfp mask again. This sets us up for subtle bugs.
> 
> I much preferred v4.

Unless I'm mistaken about that MIGRATE_HIGHATOMIC part, it seems all sashiko
concerns can be dismissed and then indeed v4 is the better version.