Re: [PATCH 1/2] mm: page_alloc: do not give all non-blocking requests reserve access

[Vlastimil Babka (SUSE)]

On 9/22/26 15:56, Johannes Weiner wrote:
> On Tue, Sep 22, 2026 at 01:52:41PM +0200, Vlastimil Babka (SUSE) wrote:
>> 
>> 
>> On 9/21/26 5:58 PM, Johannes Weiner wrote:
>> > On Mon, Sep 21, 2026 at 03:54:32PM +0100, Matthew Wilcox wrote:
>> >> On Mon, Sep 21, 2026 at 10:38:18AM -0400, Johannes Weiner wrote:
>> >>> +++ b/mm/page_alloc.c
>> >>> @@ -3246,7 +3246,9 @@ struct page *rmqueue_buddy(struct zone *preferred_zone, struct zone *zone,
>> >>>  			 * reserves as failing now is worse than failing a
>> >>>  			 * high-order atomic allocation in the future.
>> >>>  			 */
>> >>> -			if (!page && (alloc_flags & (ALLOC_OOM|ALLOC_NON_BLOCK)))
>> >>> +			if (!page &&
>> >>> +			    ((alloc_flags & ALLOC_OOM) ||
>> >>> +			     (alloc_flags & ALLOC_MASK_ATOMIC) == ALLOC_MASK_ATOMIC))
>> >>>  				page = __rmqueue_smallest(zone, order, MIGRATE_HIGHATOMIC);
>> >>
>> >> Would this be slightly neater?
>> >>
>> >> static inline bool may_access_reserves(unsigned int alloc_flags)
>> >> {
>> >> 	if (alloc_flags & ALLOC_OOM)
>> >> 		return true;
>> >> 	if (alloc_flags & (ALLOC_NON_BLOCK | ALLOC_MIN_RESERVE)) ==
>> >> 	    (ALLOC_NON_BLOCK | ALLOC_MIN_RESERVE)
>> >> 		return true;
>> >> 	return false;
>> >> }
>> 
>> I think it should be named e.g. may_access_highatomic_reserves() as
>> may_access_reserves() is rather generic and would seem to imply an
>> ALLOC_RESERVES match (see 2/2).
> 
> Note that it doesn't actually check ALLOC_HIGHATOMIC itself. It's just
> the hail-mary AFTER trying the primary migratetype. So the name still
> doesn't look right, and rmqueue_buddy() reads kind of awkardly:
> 
> if (alloc_flags & ALLOC_HIGHATOMIC)
> 	page = __rmqueue_smallest(..., MIGRATE_HIGHATOMIC);
> if (!page) {
> 	page = __rmqueue(..., migratetype, ...);
> 
> 	/* Allow OOM and order-0 atomic */
> 	if (!page && may_access_highatomic_reserve())
> 		page = __rmqueue_smallest(..., MIGRATE_HIGHATOMIC);
> }
> 
> It would have to be may_access_highatomic_reserve_as_last_resort() or
> something?
> 
> Hm, this is a mess. Looking closer, I think there is more breakage,
> name aside.
> 
> You suggest in 2/2 to use the same helper in unusable_free. But I
> think that's broken. My 2/2 does not look like the full fix either:
> 
> The fundamental problem is including the highatomics reserves in the
> watermark check for any allocations that first prefer a different
> migratetype - "regular memory". Any time we do this, we allow those
> allocations to draw down regular memory to 0 based on the presence of
> the highatomic reserves. And when reclaim, swap etc. come along there
> is nothing left for them. ALLOC_NO_WATERMARKS e.g. permits ignoring
> the wmarks but doesn't grant access to highatomic *freelists*.

Hmm yes, we'd have to be nuanced about which freelists we allow allocating
from depending on their particular watermarks. I.e. if we passed the
watermark checks only thanks to zone->nr_free_highatomic, we would have to
try those first. I assume it would be complicated and perhaps with perf
impact. But maybe if we managed to keep this complexity outside of fastpath
attempts?

The CMA handling had a similar issue and now we have that balancing
heuristic since 16867664936e ("mm,page_alloc,cma: conditionally prefer cma
pageblocks for movable allocations")

> So I think there are two choices:
> 
> (1) Let *everything* with some sort of reserve access fall back to
> highatomic, or
> 
> (2) Only allow ALLOC_HIGHATOMIC to include highatomic reserves in
> their watermarks check. With limited opportunistic fallback to the
> highatomics *freelists*, like the order-0 atomics above.
> 
> My intuition is that (1) might weaken the highatomic reserves to the
> point of uselessness, and we should probably go with (2):

I think the goal of commit 281dd25c1a01 ("mm/page_alloc: let GFP_ATOMIC
order-0 allocs access highatomic reserves") would be defeated with (2). The
order-0 atomic fallback to MIGRATE_HIGHATOMIC freelists would be there, but
the watermark check would never let such allocation through anymore, if
there wasn't a free page on the regular freelists? So it would become (some
races aside maybe) a dead code?

I guess your current patches are a compromise that can work, as patch 2/2
improves the current worse-than-(1) state to exclude the plain non-block
allocations.

> watermarks:
> if (alloc_flags & ALLOC_HIGHATOMIC)
> 	unusable_free += zone->nr_free_highatomic
> 
> freelists:
> if (alloc_flags & ALLOC_HIGHATOMIC)
> 	page = __rmqueue_smallest(..., MIGRATE_HIGHATOMIC);
> if (!page) {
> 	page = __rmqueue(..., migratetype, ...);
> 	/* Opportunistic fallback for order-0 atomics */
> 	if (!page && opportunistic_highatomic_fallback())
> 		page = __rmqueue_smallest(..., MIGRATE_HIGHATOMIC);
> }
> 
>> > I tend to be hesitant with single-use abstractions, but no objection
>> > if people think this is better.
>> 
>> True but single-use ALLOC_MASK_ATOMIC is also not that great, and the
>> usage makes the code hard to decipher. And see my reply to 2/2.
> 
> There is one small upside, which is that it pairs with the
> ALLOC_HIGHATOMIC check that precedes it. The comment says "order-0
> atomics" get a hail mary, but there is no order check. That order-0
> comes out of the sequence of events here: we first check highatomic,
> which is order > 0 && atomic. If that, and the native type, fail, we
> do the hail mary for atomic, which must be by definition order-0.
> 
> If you abstract that privilege into a generic "can access highatomic
> reserves" without an order check, it tempts refactors that cause bugs
> like the above.

Ack.