Re: [PATCH v2 1/4] mm: support promotion-only NUMA hinting scans

[Gregory Price <gourry@gourry.net>]

On Fri, Sep 18, 2026 at 02:37:52PM +0200, David Hildenbrand (Arm) wrote:
> > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > index 969594074fd2..2d1b59a27629 100644
> > --- a/include/linux/mm.h
> > +++ b/include/linux/mm.h
> > @@ -3408,6 +3408,8 @@ int get_cmdline(struct task_struct *task, char *buffer, int buflen);
> >  #define  MM_CP_UFFD_RWP_RESOLVE            (1UL << 5) /* resolve rwp */
> >  #define  MM_CP_UFFD_RWP_ALL                (MM_CP_UFFD_RWP | \
> >  					    MM_CP_UFFD_RWP_RESOLVE)
> > +/* Whether a MM_CP_PROT_NUMA change is for promotion only */
> > +#define  MM_CP_PROT_NUMA_PROMO_ONLY        (1UL << 6)
> 
> BTW, shouldn't we just be using BIT()?
>

I originally had a 5th patch to convert it, but i dropped it while
making multiple attempts to avoid a CP bit at all.

I can add it back to the end of the series if you like.

> > +		promo_only = !(numab_mode & NUMA_BALANCING_NORMAL);
> 
> bool promo_only = !(numab_mode & NUMA_BALANCING_NORMAL);
> 
> 
> and in the later patch
> 
> if (vma_is_ro_file(vma))
> 	promo_only = true;
> 
> ?

Yeah this is confusing, but it is correct.

1) If we're in the code at all, balancing was on at some point.

2) If !NORMAL - then TIERING must have been set - so always true
   (promo_only says: only PROT_NONE low-tier folios)

3) In (NORMAL | TIERING) mode.  promo_only = !NORMAL = false
   (so in numab=3 - we PROT_NONE top-tier folios)

4) But this causes socket-to-socket bouncing when (NORMAL) is set
   so we retain the "no R/O file" filter by checking it and setting
   the promo_only filter back it.

It is, decidedly, quite awful.

But the problem isn't the fix - the introduction of the R/O filter broke
TIERING first.  The problem is that these filters never took both modes
(NORMAL, TIERING) into account in the first place.  They optimized for
NORMAL and broke TIERING.

> 
> Stupid question: why can't/shouldn't change_prot_numa() query
> sysctl_numa_balancing_mode? Why do we have to query this outside of the function
> and forward it?
> 

We need to calculate it anyway for patch #4 to track when the last full
vma scan occurred in numab=3 mode.

We certainly can, but then we calculate it twice in the stack and it can
change out from under us.  I didn't want to have to think about that
split-state problem, so I err'd on the side of calculate-once and do the
whole operation based on that state.

I'll need to pull up some investigation notes, but I also remember there
being a situation where checking it underneath this caused more scanning
work - didn't want to regress anyone. This might be resolved by patch #4.

> I mean, change_prot_numa() gets the vma and can query
> sysctl_numa_balancing_mode. Why not move that into the function and avoid the
> boolean parameter?
> 
> We do have a single change_prot_numa() caller in the tree ...
> 
> >  
> >  			/*
> >  			 * Try to scan sysctl_numa_balancing_size worth of
> > diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> > index 30b7c63b0e35..2f9ada1bbcfc 100644
> > --- a/mm/huge_memory.c
> > +++ b/mm/huge_memory.c
> > @@ -2784,7 +2784,8 @@ int change_huge_pmd(struct mmu_gather *tlb, struct vm_area_struct *vma,
> >  			goto unlock;
> >  
> >  		if (!folio_can_map_prot_numa(pmd_folio(*pmd), vma,
> > -					     vma_is_single_threaded_private(vma)))
> > +					     vma_is_single_threaded_private(vma),
> > +					     cp_flags & MM_CP_PROT_NUMA_PROMO_ONLY))
> 
> As raised, maybe just forward cp_flags
> 

Yeah fair, i'll do that and just add (or eliminate) the
single_threaded_private argument if possible.

~Gregory