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

["David Hildenbrand (Arm)" <david@kernel.org>]

On 9/18/26 15:46, Gregory Price wrote:
> 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.

Should be a nice and simple cleanup.

> 
>>> +		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.

Given that that code is rather messy, I wonder if it can be cleaned up in a way
that would require us to avoid this.

> 
> 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.

Maybe forwarding whether the VMA was accessed could end up cleaner. But yeah, it
depends on how patch #4 will end up.

> 
> 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?
-- 
Cheers,

David