mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
From: "Vlastimil Babka (SUSE)" <vbabka@kernel.org>
To: Matthew Wilcox <willy@infradead.org>,
	Andrew Morton <akpm@linux-foundation.org>
Cc: Johannes Weiner <hannes@cmpxchg.org>,
	Salvatore Dipietro <dipiets@amazon.it>,
	abuehaze@amazon.com, alisaidi@amazon.com, blakgeof@amazon.com,
	brauner@kernel.org, brendan.jackman@linux.dev, david@redhat.com,
	dgc@kernel.org, dipietro.salvatore@gmail.com, djwong@kernel.org,
	hch@infradead.org, hch@lst.de, linux-fsdevel@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-mm@kvack.org,
	linux-xfs@vger.kernel.org, mhocko@suse.com,
	ritesh.list@gmail.com, rvvandan@amazon.com,
	stable@vger.kernel.org, surenb@google.com, ziy@nvidia.com
Subject: Re: [PATCH v4] mm/page_alloc: avoid direct compaction for costly __GFP_NORETRY allocations
Date: Mon, 21 Sep 2026 11:35:42 +0200	[thread overview]
Message-ID: <6c94405a-48e4-4842-a096-e2fbbbaa862f@kernel.org> (raw)
In-Reply-To: <aq4L29SBCYwvKrw2@casper.infradead.org>

On 9/19/26 06:13, Matthew Wilcox wrote:
> On Wed, Sep 16, 2026 at 03:35:40PM -0700, Andrew Morton wrote:
>> From: Salvatore Dipietro <dipiets@amazon.it>
>> Subject: mm/page_alloc: avoid direct compaction for costly __GFP_NORETRY allocations
>> Date: Fri, 4 Sep 2026 11:56:28 +0000
>> 
>> Commit 5d8edfb900d5 ("iomap: Copy larger chunks from userspace")
>> introduced high-order folio allocations in the iomap buffered write path. 
>> When memory is fragmented, each failed costly-order allocation enters
>> __alloc_pages_slowpath() which runs direct compaction and
>> drain_all_pages(), causing a 0.38x throughput drop on PostgreSQL pgbench
>> (simple-update) with 1024 clients on a 96-vCPU arm64 system.
>> 
>> The root issue is that direct compaction is too expensive for hot
>> allocation paths that have fallbacks to smaller allocations. 
>> __filemap_get_folio_mpol() already marks higher-order allocations with
>> __GFP_NORETRY | __GFP_NOWARN, signalling that the caller can handle
>> failure.  However, the page allocator still attempts full direct
>> compaction for costly orders with __GFP_NORETRY, which is unnecessarily
>> aggressive when the caller will simply retry at a lower order.
>> 
>> For costly-order allocations with __GFP_NORETRY, clear
>> __GFP_DIRECT_RECLAIM at the very start of the slowpath, before
>> can_direct_reclaim, can_compact and the nofail checks are evaluated.  This
>> makes the entire slowpath treat the request as non-blocking: no direct
>> reclaim, no direct compaction and no drain_all_pages() IPI across every
>> CPU.  kswapd (and in turn kcompactd) is still woken further down for
>> background defragmentation, so compaction keeps working for long-term
>> system health while being removed from the latency-critical direct
>> allocation path.
>> 
>> Allocations that also request __GFP_THISNODE are exempted.  That flag
>> pairing identifies the local-node-first THP attempt issued by
>> alloc_pages_mpol() (mempolicy.c), which relies on direct compaction to
>> form transparent huge pages.
> 
> This patch is still piling hack on hack.  We haven't made a serious
> effort to understand what's going on, we're just adjusting flags until
> things stop sucking.
> 
> What we know:
>  - Doing compaction every time (which is what we currently do)
>    performs badly
>  - Doing compaction once at the requested large size and then not
>    again until the smallest size also performs badly.  So it's not
>    that we're doing too much compaction, it's that we're doing
>    compaction at all.

The very first version [1] mentions the associated drain_all_pages(),
interestingly.

> But we don't know why compaction is performing badly.  For example,
> we could specify MIGRATE_ASYNC or MIGRATE_SYNC_LIGHT if __GFP_NORETRY
> is set.  Just as a reminder, here's how __GFP_NORETRY is documented:

That already happens (for costly orders) in __alloc_pages_slowpath() where
it sets INIT_COMPACT_PRIORITY.

>  * %__GFP_NORETRY: The VM implementation will try only very lightweight
>  * memory direct reclaim to get some memory under memory pressure (thus
>  * it can sleep). It will avoid disruptive actions like OOM killer. The
>  * caller must handle the failure which is quite likely to happen under
>  * heavy memory pressure. The flag is suitable when failure can easily be
>  * handled at small cost, such as reduced throughput.
> 
> So the callers aren't doing anything unreasonable when they say
> __GFP_NORETRY.  It's the page allocator (and apparently the compaction
> side of it) that's not living up to the documented contract.

Note that all these different versions of this patch mention compaction, but
implement a removal of __GFP_DIRECT_RECLAIM, thus suppressing both reclaim
and compaction, in response to __GFP_NORETRY. Thus turning this effectively
to a GFP_NOWAIT allocation. So that's also a violation of the contract, no?

In [1] thread and also later on v2 thread by you [2] it was proposed to make
the caller itself do this downgrade to GFP_NOWAIT. But Christoph and Dave
didn't like that approach (that was conditional to costly order, I don't
think yours had a response), which is understandable for the costly-order
case, but for your case it makes sense that if you don't want any
reclaim+compaction, you tell the allocator GFP_NOWAIT and not __GFP_NORETRY.

> So that's one approach which has not, as far as I can tell, been
> investigated.
> 
> The other thing that icks me about this patch is how complex the
> condition is:
> 
>> +	/*
>> +	 * Costly __GFP_NORETRY callers have a cheap fallback, so don't stall
>> +	 * them in reclaim or compaction. __GFP_THISNODE callers are exempt.
>> +	 */
>> +	if (costly_order && (gfp_mask & __GFP_NORETRY) &&
>> +	    !(gfp_mask & __GFP_THISNODE))
>> +		gfp_mask &= ~__GFP_DIRECT_RECLAIM;
> 
> So basically we're saying that __GFP_NORETRY means don't do compaction
> unless __GFP_THISNODE is set, which is just special pleading.  Surely
> the right answer is to remove __GFP_NORETRY from the one caller which
> needs __GFP_THISNODE?
> 
> 
> A slightly unrelated critique of this patch is that it's far too
> complicated for what it does.  We could achieve the same thing by doing:
> 
>  static inline bool gfp_compaction_allowed(gfp_t gfp_mask)
>  {
> -       return IS_ENABLED(CONFIG_COMPACTION) && (gfp_mask & __GFP_IO);
> +       return IS_ENABLED(CONFIG_COMPACTION) && (gfp_mask & __GFP_IO) &&
> +               (!(gfp_mask & __GFP_NORETRY) || (gfp_mask & __GFP_THISNODE));
>  }
> 
> 
> Something I've been noodling on the past day or so is cleaning up the
> GFP flags for "how hard to reclaim".  We currently have five
> possibilities encoded in four bits:
> 
> 1. No direct reclaim (__GFP_DIRECT_RECLAIM clear)
> 2. Light reclaim (__GFP_DIRECT_RECLAIM | __GFP_NORETRY)
> 3. Normal reclaim (__GFP_DIRECT_RECLAIM)
> 4. Extra reclaim (__GFP_DIRECT_RECLAIM | ___GFP_RETRY_MAYFAIL)
> 5. Reclaim forever (__GFP_DIRECT_RECLAIM | __GFP_NOFAIL)
> 
> Clearly we can save ourselves a GFP flag bit by encoding those five
> options into three bits.  Some of the code that manipulates "how hard to
> reclaim" will need to be adjusted, but it shouldn't be that many places
> to change.
> 
> If we do that, we can insert more options into the mix if they're really
> needed.  Like we could have:
> 
> 0 - No direct reclaim
> 1 - Light reclaim, no compaction
> 2 - Light reclaim with compaction
> 3 - Normal reclaim
> 4 - Extra reclaim
> 5 - Reclaim forever

There's also still (for better or worse) the THP special case of light
compaction with no reclaim.
And the long-standing principle (maybe cargo cult? who knows) that for
non-costly-order allocations (with fallback) the tradeoff of e.g. 2 is more
favorable than 0. But we don't want the callers to have to change flags
depending on the order. So it would have to be a distinct option?

> and we'd still have two extra states in case we need to add more
> flavours.

[1] https://lore.kernel.org/all/20260403193535.9970-1-dipiets@amazon.it/
[2] https://lore.kernel.org/all/alEz4Chf7Ibyg-ZG@casper.infradead.org/

  reply	other threads:[~2026-09-21  9:35 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-09-04 11:56 Salvatore Dipietro
2026-09-04 14:11 ` Vlastimil Babka (SUSE)
2026-09-04 15:08   ` Zi Yan
2026-09-07  7:30     ` Vlastimil Babka (SUSE)
2026-09-09  2:33       ` Zi Yan
2026-09-09  8:51         ` Vlastimil Babka (SUSE)
2026-09-04 16:10 ` Johannes Weiner
2026-09-06  0:42 ` Andrew Morton
2026-09-06 23:05   ` Dave Chinner
2026-09-10 11:46   ` Salvatore Dipietro
2026-09-10 22:00     ` Andrew Morton
2026-09-11 14:30       ` Salvatore Dipietro
2026-09-11 15:59     ` Johannes Weiner
2026-09-16 11:24       ` Vlastimil Babka (SUSE)
2026-09-16 15:58         ` Johannes Weiner
2026-09-16 22:34           ` Andrew Morton
2026-09-16 22:35             ` Andrew Morton
2026-09-19  4:13               ` Matthew Wilcox
2026-09-21  9:35                 ` Vlastimil Babka (SUSE) [this message]
2026-09-18  7:05           ` Vlastimil Babka (SUSE)
2026-09-18 21:28             ` Andrew Morton
2026-09-22  8:44               ` Vlastimil Babka (SUSE)
2026-09-21 14:37             ` Johannes Weiner
2026-09-21 14:38               ` [PATCH 1/2] mm: page_alloc: do not give all non-blocking requests reserve access Johannes Weiner
2026-09-21 14:54                 ` Matthew Wilcox
2026-09-21 15:58                   ` Johannes Weiner
2026-09-22 11:52                     ` Vlastimil Babka (SUSE)
2026-09-21 14:39               ` [PATCH 2/2] mm: page_alloc: remove ALLOC_NON_BLOCK from ALLOC_RESERVES Johannes Weiner
2026-09-22 12:06                 ` Vlastimil Babka (SUSE)
2026-09-07  5:54 ` [PATCH v4] mm/page_alloc: avoid direct compaction for costly __GFP_NORETRY allocations Christoph Hellwig

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=6c94405a-48e4-4842-a096-e2fbbbaa862f@kernel.org \
    --to=vbabka@kernel.org \
    --cc=abuehaze@amazon.com \
    --cc=akpm@linux-foundation.org \
    --cc=alisaidi@amazon.com \
    --cc=blakgeof@amazon.com \
    --cc=brauner@kernel.org \
    --cc=brendan.jackman@linux.dev \
    --cc=david@redhat.com \
    --cc=dgc@kernel.org \
    --cc=dipietro.salvatore@gmail.com \
    --cc=dipiets@amazon.it \
    --cc=djwong@kernel.org \
    --cc=hannes@cmpxchg.org \
    --cc=hch@infradead.org \
    --cc=hch@lst.de \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=linux-xfs@vger.kernel.org \
    --cc=mhocko@suse.com \
    --cc=ritesh.list@gmail.com \
    --cc=rvvandan@amazon.com \
    --cc=stable@vger.kernel.org \
    --cc=surenb@google.com \
    --cc=willy@infradead.org \
    --cc=ziy@nvidia.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®