Re: [RFC PATCH v3 08/17] sched/cache: Introduce helpers for task migration decisions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] sched/cache: can_migrate_llcs() misapplies node walk when dst precedes src
--

commit c7cd924467c2448429791ac003c69c23764da05a
Author: Jianyong Wu <wujianyong@hygon.cn>

sched/cache: Introduce helpers for task migration decisions

This commit introduces get_node_stats(), can_migrate_llcs(), and
if_to_prefer(), and extends fits_llc_capacity() to support cache-aware task
migration decisions across multiple LLCs and NUMA nodes.

> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index fe357d79b6e06..4eb7cede9080f 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c

[ ... ]

> @@ -10614,13 +10617,185 @@ static enum llc_mig can_migrate_llc(int src_cpu, int dst_cpu,

[ ... ]

> +static enum llc_mig __maybe_unused can_migrate_llcs(int src_cpu, int dst_cpu,
> +				     struct task_struct *p, bool to_pref)
> +{
> +	unsigned long src_util, src_pre, src_cap, dst_util, dst_cap, dst_pre;
> +	int target_node, src_node, dst_node, dst_distance;
> +	int node, target_cpu = src_cpu, n_llc;
> +	unsigned long tsk_util = 0;
> +	bool got_src = false;
> +
> +	if (!get_llc_stats(src_cpu, &src_util, &src_cap) ||
> +	    !get_llc_stats(dst_cpu, &dst_util, &dst_cap))
> +		return mig_unrestricted;
> +
> +	if (p) {
> +		struct mm_struct *mm = p->mm;
> +
> +		if (mm)
> +			target_cpu = READ_ONCE(mm->sc_stat.cpu);
> +		target_cpu = target_cpu >= 0 ? target_cpu : src_cpu;
> +
> +		tsk_util = task_util(p);
> +	}

[ ... ]

> +	src_node = cpu_to_node(src_cpu);
> +	dst_node = cpu_to_node(dst_cpu);
> +	target_node = cpu_to_node(target_cpu);
> +	dst_distance = node_distance(target_node, dst_node);
> +
> +	/*
> +	 * For moves away from the cache preferred node, examine the source,
> +	 * intervening nodes and destination in the cached affinity order.
> +	 */
> +	for_each_affi_node(target_cpu, node) {
> +		unsigned long nu = 0, nc = 0;
> +
> +		if (!got_src) {
> +			if (node != src_node)
> +				continue;
> +			got_src = true;

[ ... ]

> +			/* Cross-node moves must leave the source node filled. */
> +			if (get_node_stats(src_node, &nu, &nc, &n_llc) && n_llc) {
> +				nu = nu < tsk_util ? 0 : nu - tsk_util;
> +				if (fits_llc_capacity(nu, nc, false))
> +					return mig_forbid;
> +			}
> +			continue;
> +		}
> +
> +		if (node != dst_node) {
> +			if (!get_node_stats(node, &nu, &nc, &n_llc) || !n_llc)
> +				continue;
> +
> +			/*
> +			 * Estimate average LLC headroom in intervening nodes.
> +			 * Charge one task per LLC, or per pair of LLCs when
> +			 * this node is closer to the target than the destination.
> +			 */
> +			if (node_distance(target_node, node) < dst_distance)
> +				nu += DIV_ROUND_UP(n_llc, 2) * tsk_util;
> +			else
> +				nu += n_llc * tsk_util;
> +
> +			if (fits_llc_capacity(nu, nc, true))
> +				return mig_forbid;
> +			continue;
> +		}
> +
> +		if (fits_llc_capacity(dst_util, dst_cap, true))
> +			return mig_llc;
> +
> +		if (util_greater(src_pre, dst_pre) && src_util >= dst_util)
> +			return mig_llc;
> +
> +		break;
> +	}

[Severity: Medium]
In kernel/sched/fair.c, what happens in can_migrate_llcs() when to_pref is
false and src_node != dst_node, yet dst_node precedes src_node in the
for_each_affi_node(target_cpu, node) walk?

The loop skips every node before src_node while got_src is false, so
dst_node is passed before got_src becomes true. Once node == src_node sets
got_src = true, every subsequent node in the affinity sequence satisfies
node != dst_node. Those trailing nodes are then evaluated as intervening
nodes and can veto the move with mig_forbid, while the destination checks at
node == dst_node are never reached, contrary to the commit message guarantee
that nodes following the destination do not participate in the decision.

When wired up to can_migrate_llc_task() and alb_break_llc() in the
subsequent commit ("sched/cache: Apply multi-LLC aggregation policy to task
migration"), dst_node can precede src_node in target_cpu's affinity order
even when to_pref is false in several cases:

1. Both can_migrate_llc_task() and alb_break_llc() read mm->sc_stat.cpu once
   to compute pref_llc for if_to_prefer(), and can_migrate_llcs() reads
   mm->sc_stat.cpu a second time via READ_ONCE(mm->sc_stat.cpu). A
   concurrent update in task_cache_work() or account_mm_sched() can cause
   if_to_prefer() to return false for the old CPU while can_migrate_llcs()
   reads a new target_cpu whose target_node ranks dst_node before src_node.

2. In alb_break_llc(), pref_llc is only derived from mm->sc_stat.cpu when
   cur->sched_class == &fair_sched_class (otherwise remaining -1 so
   if_to_prefer() returns false), whereas can_migrate_llcs() unconditionally
   reads target_cpu = READ_ONCE(p->mm->sc_stat.cpu).

3. In if_to_prefer(), llc_to_node(pref_llc) can return -1 (for example, if
   rebuild_llc_node_map() failed allocation), causing if_to_prefer() to
   return false while can_migrate_llcs() resolves target_node via
   cpu_to_node(target_cpu) and walks for_each_affi_node(target_cpu, node).

Should the caller pass a single consistent target_cpu or preferred node to
both helpers, or should can_migrate_llcs() handle encountering dst_node
before src_node during the affinity walk?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029022036.4002331-1-wujianyong@hygon.cn?part=8