Re: [PATCH] memory tiering: Do not allow promotion if NUMA_BALANCING_MEMORY_TIERING is disabled

[Donet Tom <donettom@linux.ibm.com>]

On 3/20/26 9:50 PM, Andrew Morton wrote:
> On Fri, 20 Mar 2026 14:52:51 +0530 Donet Tom <donettom@linux.ibm.com> wrote:
>
>> In the current implementation, if NUMA_BALANCING_MEMORY_TIERING is
>> disabled and the pages are on the lower tier, the pages may still be
>> promoted.
>>
>> This happens because task_numa_work() updates the last_cpupid field to
>> record the last access time only when NUMA_BALANCING_MEMORY_TIERING is
>> enabled and the folio is on the lower tier. If
>> NUMA_BALANCING_MEMORY_TIERING is disabled, the last_cpupid field
>> retains a valid last CPU id.
>>
>> In should_numa_migrate_memory(), the decision checks whether
>> NUMA_BALANCING_MEMORY_TIERING is disabled, the folio is on the lower
>> tier, and last_cpupid is invalid. However, since last_cpupid remains
>> valid when NUMA_BALANCING_MEMORY_TIERING is disabled, the condition
>> evaluates to false and migration is allowed.
>>
>> This patch prevents promotion when NUMA_BALANCING_MEMORY_TIERING is
>> disabled and the folio is on the lower tier.
> Thanks.  The AI reviewbot asked some questions:
> 	https://sashiko.dev/#/patchset/20260320092251.1290207-1-donettom@linux.ibm.com
>
AI review questions

> commit b0847daa3e0064d33d2232cba1d7607919cae7db
> Author: Donet Tom <donettom@linux.ibm.com>
> memory tiering: Do not allow promotion if NUMA_BALANCING_MEMORY_TIERING is disabled
> In the current implementation, if NUMA_BALANCING_MEMORY_TIERING is
> disabled and the pages are on the lower tier, the pages may still be
> promoted.
> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -2060,6 +2060,13 @@ bool should_numa_migrate_memory(struct task_struct *p, struct folio *folio,
> >  	 */
> >  	if (!node_state(dst_nid, N_MEMORY))
> >  		return false;
> > +	/*
> > +	 * Do not allow promotion if NUMA_BALANCING_MEMORY_TIERING is disabled
> > +	 * and the pages are on the lower tier.
> > +	 */
> > +	if (!(sysctl_numa_balancing_mode & NUMA_BALANCING_MEMORY_TIERING) &&
> > +	    !node_is_toptier(src_nid))
> > +		return false;
> Does this block all migrations originating from a lower-tier node when tiering
>
> is disabled, including valid same-tier migration between two 
> lower-tier nodes?
>
This blocks migration (promotion) from a lower tier to a higher tier 
when tiering is disabled. However, migration within the same tier will 
still be performed if NUMA_BALANCING_NORMAL is enabled.

> Conversely, since this only checks !node_is_toptier(src_nid), does it still
> allow cross-tier migration from top-tier to lower-tier nodes when tiering is
> disabled?

Migration (demotion) from a higher tier to a lower tier is not handled 
by NUMA balancing, so it will work irrespective of this setting.

> Also, because this early return is placed before folio_xchg_last_cpupid() is
> called, does it prevent lower-tier folios from overwriting their stale
> _last_cpupid timestamp with a valid CPU ID? This could permanently freeze NUMA
> access tracking for these folios and inflate private fault statistics.


No, the timestamp is only stored when NUMA_BALANCING_MEMORY_TIERING is 
enabled.


> [ ... ]
> > @@ -2094,10 +2101,6 @@ bool should_numa_migrate_memory(struct task_struct *p, struct folio *folio,
> >  	this_cpupid = cpu_pid_to_cpupid(dst_cpu, current->pid);
> >  	last_cpupid = folio_xchg_last_cpupid(folio, this_cpupid);
> >  
> > -	if (!(sysctl_numa_balancing_mode & NUMA_BALANCING_MEMORY_TIERING) &&
> > -	    !node_is_toptier(src_nid) && !cpupid_valid(last_cpupid))
> > -		return false;
> > -
> This isn't a bug, but since the only usages of cpupid_valid() were removed
> here and in task_numa_fault(), should the definition of cpupid_valid() and its
> accompanying comments also be removed to avoid leaving dead code behind?


Yes, We can remove that. I will send a V2 with that change.


> [ ... ]
> > @@ -3312,8 +3315,7 @@ void task_numa_fault(int last_cpupid, int mem_node, int pages, int flags)
> >  	 * node for memory tiering mode.
> >  	 */
> >  	if (!node_is_toptier(mem_node) &&
> > -	    (sysctl_numa_balancing_mode & NUMA_BALANCING_MEMORY_TIERING ||
> > -	     !cpupid_valid(last_cpupid)))
> > +	    (sysctl_numa_balancing_mode & NUMA_BALANCING_MEMORY_TIERING))
> >  		return;
> If memory tiering is disabled at runtime, existing folios on slow nodes may
> retain an access time stored in _last_cpupid instead of a valid CPU ID.
> By removing the !cpupid_valid(last_cpupid) check, could this timestamp be
> treated as a valid CPU ID and passed into task_numa_group()?
> Inside task_numa_group(), cpupid_to_cpu(cpupid) could then extract an
> out-of-bounds CPU index, which might lead to an out-of-bounds read when
> accessing cpu_rq(cpu)->curr and cause a kernel panic.

Yes. This is also a valid point. Let me fix this and send a v2.