Re: [PATCH 1/5] mm: compaction: push watermark into compaction_suitable() callers

[Vlastimil Babka <vbabka@suse.cz>]

On 4/10/25 22:17, Johannes Weiner wrote:
> On Thu, Apr 10, 2025 at 05:19:06PM +0200, Vlastimil Babka wrote:
>> On 3/13/25 22:05, Johannes Weiner wrote:
> 
> Ah yes, it would have made sense to point out.
> 
> I was wondering about this check. It was introduced to bail on
> compaction if there are not enough free non-CMA pages. But if there
> are, we still fall through and check the superset of regular + CMA
> pages against the watermarks as well. We know this will succeed, so
> this seems moot.

I guess we didn't want to avoid the fragindex part of compaction_suitable(),
which in theory may not succeed?

> It's also a little odd that compaction_suitable() hardcodes ALLOC_CMA
> with the explanation that "CMA are migration targets", but then this
> check says "actually, it doesn't help us if blocks are formed in CMA".

Hm yes.

> Does it make more sense to plumb alloc_flags to compaction_suitable()?

Possibly.

> There is more head-scratching, though. The check is meant to test
> whether compaction has a chance of forming non-CMA blocks. But free
> pages are targets. You could have plenty of non-contiguous, free
> non-CMA memory - compaction will then form blocks in CMA by moving CMA
> pages into those non-CMA targets.
> 
> The longer I look at this, the more I feel like this just hard-coded
> the very specific scenario the patch author had a problem with: CMA is
> massive. The page allocator fills up regular memory first. Once
> regular memory is full, non-CMA requests stall on compaction making
> CMA blocks. So just bail on compaction then.

Right.

> It's a valid problem, but I don't see how this code makes any general
> sense outside of this exact sequence of events. Especially once
> compaction has moved stuff around between regular and CMA memory, the
> issue will be back, and the check does nothing to prevent it.

Yeah, it seemed to fix a real problem and we both acked it :) but it's not
ideal.

Maybe the true solution (or a step towards it) would be for compaction for
!ALLOC_CMA only use non-CMA pageblocks as migration sources.
IMHO it's just another symptom of the general problem that CMA pageblocks
exist as part of a zone that's not otherwise ZONE_MOVABLE and suddenly the
watermarks have to depend on the allocation type. And of course for the
high-order allocations it doesn't just matter the amount of memory in cma vs
non-cma parts of the zone, but also its contiguity.