RE: [RFC PATCH v2 11/23] sched/cache: Introduce helpers for task migration decisions

[Jianyong Wu <wujianyong@hygon.cn>]

Hi Peter,

> -----Original Message-----
> From: Peter Zijlstra <peterz@infradead.org>
> Sent: Tuesday, September 1, 2026 5:09 PM
> To: Jianyong Wu <wujianyong@hygon.cn>
> Cc: Ingo Molnar <mingo@redhat.com>; Juri Lelli <juri.lelli@redhat.com>;
> Vincent Guittot <vincent.guittot@linaro.org>; Chen Yu
> <yu.c.chen@intel.com>; Tim Chen <tim.c.chen@linux.intel.com>; Dietmar
> Eggemann <dietmar.eggemann@arm.com>; Steven Rostedt
> <rostedt@goodmis.org>; Ben Segall <bsegall@google.com>; Mel Gorman
> <mgorman@suse.de>; Valentin Schneider <vschneid@redhat.com>; K
> Prateek Nayak <kprateek.nayak@amd.com>; Shrikanth Hegde
> <sshegde@linux.ibm.com>; Phil Auld <pauld@redhat.com>; Andrew
> Morton <akpm@linux-foundation.org>; David Hildenbrand
> <david@kernel.org>; linux-kernel@vger.kernel.org; linux-mm@kvack.org;
> jianyong.wu@outlook.com; Yuan Zhong <zhongyuan@hygon.cn>; Huangsj
> <huangsj@hygon.cn>; Fengyu Wang <wangfengyu@hygon.cn>; Zhiwei Ying
> <yingzhiwei@hygon.cn>; justin.he@arm.com
> Subject: Re: [RFC PATCH v2 11/23] sched/cache: Introduce helpers for task
> migration decisions
> 
> On Thu, Aug 27, 2026 at 08:28:04PM +0800, Jianyong Wu wrote:
> 
> > +/*
> > + * Decide if migration should happen on a specific node.
> > + * The node here is an LLC or a NUMA.
> > + */
> > +static enum llc_mig __maybe_unused can_migrate_node(int src_cpu, int
> dst_cpu,
> > +			struct task_struct *p, bool to_pref)
> > +{
> > +	const struct cpumask *span;
> > +	struct mm_struct *mm;
> > +	unsigned long dst_util, dst_cap, tsk_util = 0;
> > +	unsigned long src_util = 0, src_cap = 0;
> > +	unsigned long acc_util = 0, acc_cap = 0;
> > +	int node, target_cpu = src_cpu;
> > +	int get_src = 0;
> > +
> > +	if (!get_llc_stats(dst_cpu, &dst_util, &dst_cap))
> > +		return mig_unrestricted;
> > +
> > +	if (!get_llc_stats(src_cpu, &src_util, &src_cap))
> > +		src_cap = 0;
> 
> Comparing against can_migrate_llc(), that bails with mig_unrestricted
> when !get_llc_stats(src_cpu).
> 
Agreed.

> > +
> > +	if (p) {
> > +		mm = p->mm;
> > +		if (mm) {
> > +			if (mm->sc_stat.cpu >= 0)
> > +				target_cpu = mm->sc_stat.cpu;
> > +		}
> > +		tsk_util = task_util(p);
> > +	}
> 
> It isn't clear when !p would be valid. migration is always about a task,
> no?
>
This helper is also called by llc_balance which will pass NULL task. So, !p
is valid here, same as can_migrate_llc(). In that case tsk_util is 0 and the
decision is made via the to_pref path.
 
> > +
> > +	dst_util = dst_util + tsk_util;
> 
> can_migrate_llc() also adjusts src_util by subtracting tsk_util (and
> flooring at 0).

Both the pre- and post-migration utilization are needed here. The current imbalance
check uses the utilization before the move, while the capacity and anti-bounce checks
need the utilization after the move.

I'll introduce explicit src_pre/src_post and dst_pre/dst_post values with the source
subtraction floored to zero.

> 
> > +	if (to_pref) {
> > +		unsigned long dst_pre = dst_util - tsk_util;
> > +
> > +		if (fits_llc_capacity(dst_util, dst_cap))
> > +			return mig_llc;
> > +
> > +		/*
> > +		 * The destination is over the margin. That is a reason to
> > +		 * refuse a task while the margin can still be met, but not
> > +		 * while every LLC of the node is over it: no placement
> > +		 * satisfies the margin then, and refusing every migration
> > +		 * leaves the imbalance in place.
> > +		 *
> > +		 * Let the task through when the move still lowers the peak,
> > +		 * that is when the source is noticeably heavier than the
> > +		 * destination and carries at least two more tasks worth of
> > +		 * utilization. The second condition keeps the destination
> > +		 * from becoming the heavier side, which would bounce the
> > +		 * task straight back.
> > +		 */
> > +		if (src_cap && util_greater(src_util, dst_pre) &&
> > +		    src_util >= dst_pre + 2 * tsk_util)
> > +			return mig_llc;
> > +
> > +		return mig_forbid;
> > +	}
> > +
> > +	for_each_sched_node(target_cpu, node) {
> 
> So this iterates the nodes in the order specific to the node that
> contains target_cpu.

Yes, the node including target_cpu decides the affinity node sequence.

> 
> > +		unsigned long u = 0, c = 0, nu, nc;
> > +
> > +		/*
> > +		 * The walk starts at the anchor, so the nodes it crosses before
> > +		 * reaching the source say nothing about this migration: the task
> > +		 * does not live there and is not going there. Judging them only
> > +		 * lets an unrelated node with room refuse the move. Start at
> the
> > +		 * node the task actually sits on.
> > +		 */
> > +		if (!get_src) {
> > +			if (!cpumask_test_cpu(src_cpu, cpumask_of_node(node)))
> > +				continue;
> > +			else
> 
> Strictly speaking this else is superfluous.
>
Yeah, will remove it.
 
> > +				get_src = 1;
> > +		}
> 
> And then you skip the nodes between target and src, which are the nodes
> with best locality, confusing, but lets read on..
> 
> (sometimes target == src, and you thus don't skip anything, but other
> times this is the preferred cpu)
>

As this is the "!to-prefer" path, dest cpu is not in the sub-sequence from the target_llc to src_llc in the affinity node sequence. That's why we skip them here. 
 
> > +		if (cpumask_test_cpu(dst_cpu, cpumask_of_node(node))) {
> 
> For the node that contains dst_cpu..
> 
> > +			nu = 0;
> > +			nc = 0;
> > +			for_each_llc_node_span(node, span) {
> 
> Iterate the LLCs in this node..
> 
> > +				get_span_stats(span, &u, &c);
> 
> Now I'm confused again, @span is the span of the llc, but
> get_span_stats() is a wrapper around get_llc_stats(), and since span is
> just a single llc, why use this rather than get_llc_stats() directly?
>
Oh, it's simpler to use get_llc_stats here. Will change it.
 
> > +				nu += u;
> > +				nc += c;
> > +				if (cpumask_test_cpu(dst_cpu, span)) {
> 
> If llc includes dst_cpu
> 
> (indent is getting a little out of hand here)
>
OK, the nesting got too deep here. I'll refactor these code.

> > +					if (fits_llc_capacity(u + tsk_util, c))
> > +						return mig_llc;
> > +
> > +					/*
> > +					 * The destination is over the margin,
> > +					 * but so may be the source. Refusing
> > +					 * then leaves the peak where it is:
> > +					 * a LLC at seven tasks stays at seven
> > +					 * while a neighbour in the same node
> > +					 * sits at four, because taking one
> > +					 * more would put that neighbour over
> > +					 * the margin as well.
> > +					 *
> > +					 * Let the task through when the move
> > +					 * still lowers the peak, guarded the
> > +					 * same way as the aggregation path:
> > +					 * the source must be noticeably
> > +					 * heavier and carry at least two more
> > +					 * tasks worth of utilization, so the
> > +					 * destination cannot end up the
> > +					 * heavier side and bounce it back.
> > +					 */
> > +					if (src_cap &&
> > +					    util_greater(src_util, u + tsk_util) &&
> > +					    src_util >= u + 2 * tsk_util)
> > +						return mig_llc;
> > +
> > +					return mig_forbid;
> 
> This is an unconditional return..

This unconditional return is intentional. Once the walk reaches the dst llc,
we have enough information to make the final decision about whether to
migrate or not. LLCs after the destination llc don't affect this decision.
> 
> > +				/*
> > +				 * A nearer LLC only justifies vetoing this
> > +				 * migration if the task would actually fit
> 
> But you just skipped the nodes between target and src, those are nearer,
> no?

Yes, they are nearer. But they don't contain the destination LLC, so they are
irrelevant to this migration.

> 
> > +				 * there, so account for its utilization the
> > +				 * same way the destination branch above does.
> > +				 * Without it a LLC already holding one task
> > +				 * per core still reads as having room and
> > +				 * vetoes every migration towards a farther,
> > +				 * genuinely idle LLC.
> > +				 */
> > +				} else if (fits_llc_capacity(acc_util + nu + tsk_util,
> 
> .. which renders this else superfluous, but that won't help with the
> indent because you still have the chained if :-(

Right, this else is unnecessary. It needs some refactoring here to make it readable.

> 
> > +						acc_cap + nc)
> > +						&& fits_llc_capacity(u + tsk_util, c)
> > +						&& !util_greater(u, dst_pre))
> 
> (logical operators go at the end of the previous line, your patch is
> inconsistent on this point, since that is what you do elsewhere)

OK, I will move the logical operators to the ends of the preceding line.

> 
> > +					return mig_forbid;
> > +			}
> > +		}
> > +
> > +		/* Don't migrate if this is a good place to live. */
> > +		for_each_llc_node_span(node, span) {
> > +			get_span_stats(span, &u, &c);
> 
> This is shared with the above loop, meaning you're now duplicating this
> work in case you fell through.

The destination node walk is intended to be terminal: it should find the llc
containing dst_cpu and return a decision.

I will make that explicit by returning mig_unrestricted if the destination llc is
unexpectedly not found, so that the generic llc walk is only used for
non-destination nodes. Thus, duplicate scan is avoided.
 
> 
> > +			if (cpumask_test_cpu(src_cpu, span)) {
> > +				if (fits_llc_capacity(u, c))
> > +					return mig_forbid;
> > +			} else {
> > +				if (fits_llc_capacity(u + tsk_util, c))
> > +					return mig_forbid;
> > +			}
> > +		}
> > +	}
> > +
> > +	return mig_unrestricted;
> > +}
> > +
> >  /*
> >   * Check if task p can migrate from source LLC to
> >   * destination LLC in terms of cache aware load balance.
> > --
> > 2.34.1
> >
> >