Re: [RFC PATCH v2 0/4] mm: compaction: mTHP-friendly memory compaction

[Bo Zhang]

On 10/20/26 16:52, Vlastimil Babka (SUSE) wrote:
> Isn't using  struct compact_control clean enough? There's already many
> fields like
>     bool direct_compaction;         /* False from kcompactd or /proc/... */
>     bool proactive_compaction;      /* kcompactd proactive compaction */
>
> Who says is_via_compact_memory() cannot be refactored into something using
> these kinds of fields and not 'order' being -1, which could be then used as
> the target order.

Thanks, that's a much cleaner direction than reading huge_anon_orders_always
and would also address the race I raised in the second open question.

I tried to work out what it takes, and ran into one thing I'd like your
opinion on before committing to an approach.

Today order == -1 (is_via_compact_memory()) covers two callers that both go
through compact_node():

  - proactive compaction         (proactive_compaction = true,  order = -1)
  - /proc/sys/vm/compact_memory  (proactive_compaction = false, order = -1)

and the real order-carrying callers are:

  - direct compaction            (direct_compaction = true,     order > 0)
  - kcompactd normal work        (direct_compaction = false,    order > 0)

The issue is that /proc/sys/vm/compact_memory and kcompactd normal work
differ only in the order value (-1 vs > 0), and both have direct_compaction =
= false and proactive_compaction == false. So with the existing bool fields
aloneI can't reproduce is_via_compact_memory() exactly without adding a new
field, which I'd rather avoid.

If instead I give proactive compaction a real target order (and keep
compact_memory on order == -1), the ambiguity goes away, but giving proactive
a real order is not behaviour-neutral: several paths that proactive currently
skips because order <= -1 would start running. I went through them:

1) skip_isolation_on_order() - currently proactive takes the pageblock_order
   branch, because target_order == -1 short-circuits the first test:

       if (!is_via_compact_memory(target_order) && order >= target_order)
               return true;

   With a real target order the test starts applying and would skip isolating
   folios with order >= target_order. That is close to what patch 3 does, but
   not identical: patch 3 tests order >= compact_hpage_order() and
   additionally gates on mthp_always_enabled(), whereas the native test keys
   off target_order directly. Whether patch 3 can be folded into this depends
   on exactly how the proactive target order is set, so I'd treat that as a
   follow-up rather than a given.

2) fast_isolate_freepages() - currently returns early for proactive:

       /* Full compaction passes in a negative order */
       if (cc->order <= 0)
               return;

   Here the order == -1 is doing something deliberate: it keeps full
   compaction out of the fast freelist search and on the linear free scanner.
   The fast search is meant to grab enough free pages of a specific order
   for direct compaction and stops early, which is at odds with proactive's
   goal of scanning broadly. With a real target order proactive would start
   taking the fast path, so my plan is to preserve the current behaviour with
   an explicit guard:

       if (cc->order <= 0 || cc->proactive_compaction)
               return;

3) suitable_migration_target() - the free-block test changes:

       int order = cc->order > 0 ? cc->order : pageblock_order;

   A free buddy page is rejected as a migration target when its order is
   >= this value. For proactive today the threshold is pageblock_order, so
   only very large free blocks are skipped; with a real target order it drops
   to the mTHP order. To keep proactive's behaviour unchanged here too, I'd
   keep the threshold at pageblock_order for proactive, i.e. gate on
   proactive_compaction rather than let the real order take effect.

4) compact_zone() end-of-loop drain - this becomes active:

       if (cc->order > 0 && last_migrated_pfn) { ... lru drain ... }

   (capture is not a concern here, proactive passes capc == NULL.) This one
   is harmless, but for consistency I'd keep it gated off for proactive along
   with (2) and (3), so the order change does not alter the proactive path.

5) compact_zone() start - this watermark check starts running for proactive:

       if (!is_via_compact_memory(cc->order)) {
               ret = compaction_suit_allocation_order(cc->zone, cc->order, ...);
               if (ret != COMPACT_CONTINUE)
                       return ret;
       }

   This one is different from (2)-(4): it may actually be worth having for
   proactive, so I've left it for discussion below rather than gating it off.

A couple of paths are not affected, since proactive already diverges there
via existing flags: __compact_finished() handles proactive earlier through
cc->proactive_compaction, and fast_find_migrateblock() returns early because
proactive sets ignore_skip_hint.

So my plan would be to let cc->order carry the real target order, but gate
(2), (3) and (4) on cc->proactive_compaction so the proactive path keeps its
current broad-scan behaviour - i.e. the order change is purely about the
target, not about switching on direct-compaction paths.

The one I'd genuinely like your opinion on is (5): the
compaction_suit_allocation_order() watermark check. Unlike the others, this
one might actually be desirable for proactive - it would stop proactive from
compacting when the order is already satisfied by the watermark, which is
close to the "don't compact when memory is already sufficient" point Dev
raised on the sibling thread. But it layers a watermark gate on top of the
fragmentation-score logic that drives proactive today, and I'm not sure the
two should be mixed. Would you let this one take effect for proactive, or
keep it gated off like the rest?

The other open point is the one you raised: compact_memory and
kcompactd-normal work can only be told apart by the order today (both have
direct_compaction == false, proactive_compaction == false). So a fully
behaviour-neutral "bool instead of order == -1" cleanup isn't quite possible
without a new field. Do you see adding a field as off the table, or is it
acceptable here?

Thanks,
Bo