Re: [RFC PATCH v3 13/17] sched/cache: Allow active balance to spread load from a full LLC

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] sched/fair: missing 2-CPU utilization gap check in llc_spread_active_balance()
- [Medium] sched/cache: futile active load balance in llc_spread_active_balance()
--

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

sched/cache: Allow active balance to spread load from a full LLC

This commit introduces llc_spread_active_balance() so migrate_task can trigger
active load balancing across LLCs without waiting for nr_balance_failed to
exceed cache_nice_tries + 2 when the source LLC exceeds the aggregation
threshold and the destination LLC has spare capacity.

> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index c3f9ef8798d6c..1e286296cd65c 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -13558,6 +13558,57 @@ imbalanced_active_balance(struct lb_env *env)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_SCHED_CACHE
> +/*
> + * Cache-aware aggregation can trigger active balance via migrate_llc_task
> + * without waiting for nr_balance_failed. Spreading load via migrate_task
> + * normally waits for nr_balance_failed to exceed cache_nice_tries + 2.
> + * This can leave one LLC above the aggregation threshold while another
> + * LLC has room.
> + *
> + * Allow that spread when the source is above the aggregation threshold,
> + * the destination has room for one CPU's load, and the source leads by
> + * at least two CPUs' load. The gap prevents one migration from reversing
> + * their utilization ordering and immediately inviting a migration back.
> + * The task migration checks still decide whether the task may move.
> + */
> +static inline bool llc_spread_active_balance(struct lb_env *env)
> +{
> +	unsigned long src_util, src_cap, dst_util, dst_cap;
> +
> +	if (!sched_cache_enabled())
> +		return false;
> +
> +	if (env->migration_type != migrate_task)
> +		return false;

[Severity: Medium]
Can llc_spread_active_balance() trigger repeated futile active load balance
attempts when env->src_rq->nr_running > 1 and detach_tasks() was vetoed by
cache locality checks?

In sched_balance_rq(), when detach_tasks() fails to pull a task because
can_migrate_task() -> migrate_degrades_llc() -> can_migrate_llcs() returns
mig_forbid, can_migrate_task() sets LBF_LLC_PINNED in env.flags.

When ld_moved == 0, sched_balance_rq() skips incrementing
sd->nr_balance_failed if LBF_LLC_PINNED is set, and then calls
need_active_balance():

kernel/sched/fair.c:sched_balance_rq() {
    ...
		if (idle != CPU_NEWLY_IDLE &&
		    env.migration_type != migrate_misfit &&
		    !(env.flags & LBF_LLC_PINNED))
			sd->nr_balance_failed++;

		if (need_active_balance(&env)) {
    ...
}

In need_active_balance(), alb_break_llc() only checks can_migrate_llcs() when
env->src_rq->nr_running == 1:

kernel/sched/fair.c:alb_break_llc() {
    ...
	if (env->src_rq->nr_running == 1) {
		...
		if (can_migrate_llcs(env->src_cpu, env->dst_cpu,
				    cur, to_pref) == mig_forbid)
			return true;
	}

	return false;
}

When env->src_rq->nr_running > 1, alb_break_llc() returns false, and
llc_spread_active_balance() does not check env->flags & LBF_LLC_PINNED,
env->src_rq->nr_running == 1, or the post-departure and NUMA node constraints
in can_migrate_llcs().

If llc_spread_active_balance() returns true here, sched_balance_rq() queues
active_load_balance_cpu_stop() via stop_one_cpu_nowait() and resets
sd->balance_interval to sd->min_interval. When the CPU stopper runs
detach_one_task() -> can_migrate_task(), sd->nr_balance_failed is still 0, so
migrate_degrades_llc() rejects the migration for the same reason.

Should llc_spread_active_balance() bail out when env->flags & LBF_LLC_PINNED
is set (or check the post-departure state) to avoid kicking the CPU stopper
for a move that can_migrate_task() will reject?

> +
> +	if (cpus_share_cache(env->src_cpu, env->dst_cpu))
> +		return false;
> +
> +	if (!get_llc_stats(env->src_cpu, &src_util, &src_cap) ||
> +	    !get_llc_stats(env->dst_cpu, &dst_util, &dst_cap))
> +		return false;
> +
> +	if (fits_llc_capacity(src_util, src_cap, true))
> +		return false;
> +
> +	/*
> +	 * Reserve room for one CPU's load at the destination. A two-CPU gap
> +	 * keeps one migration from reversing the utilization ordering and
> +	 * causing the task to ping-pong between LLCs.
> +	 */
> +	if (!fits_llc_capacity(dst_util + SCHED_CAPACITY_SCALE, dst_cap, true))
> +		return false;
> +
> +	return util_greater(src_util, dst_util);
> +}

[Severity: Medium]
Does llc_spread_active_balance() actually enforce the two-CPU utilization gap
described in the comment and commit message?

Moving a task with one full CPU of utilization (SCHED_CAPACITY_SCALE) reduces
src_util by SCHED_CAPACITY_SCALE and increases dst_util by
SCHED_CAPACITY_SCALE, narrowing the gap between src_util and dst_util by
2 * SCHED_CAPACITY_SCALE.

However, the checks here only verify:
1. !fits_llc_capacity(src_util, src_cap, true), meaning src_util > threshold
2. fits_llc_capacity(dst_util + SCHED_CAPACITY_SCALE, dst_cap, true), meaning
   dst_util <= threshold - SCHED_CAPACITY_SCALE
3. util_greater(src_util, dst_util), meaning
   src_util * 100 > dst_util * (100 + llc_imb_pct)

For equal-capacity LLCs, the two fits_llc_capacity() checks only ensure
src_util - dst_util > SCHED_CAPACITY_SCALE (one CPU of load), and
util_greater() only requires a 20% relative difference (which is smaller than
2 * SCHED_CAPACITY_SCALE whenever dst_util < 10 * SCHED_CAPACITY_SCALE).

For example, on a 4-CPU LLC (cap = 4096, 50% threshold = 2048) with
src_util = 2050 and dst_util = 1024, all three checks pass even though
src_util - dst_util is only 1026 (~1 CPU). Migrating a 1-CPU task (1024)
drops src_util to 1026 (below the aggregation threshold) and raises dst_util
to 2048, reversing the utilization ordering between the two LLCs.

Furthermore, in can_migrate_llcs(), src_util has tsk_util subtracted before
checking the threshold and post-migration ordering:

kernel/sched/fair.c:can_migrate_llcs() {
    ...
			if (src_node == dst_node) {
				/* Leave the source LLC at or above the threshold. */
				if (fits_llc_capacity(src_util, src_cap, false))
					return mig_forbid;

				if (util_greater(src_pre, dst_pre) &&
				    src_util >= dst_util)
					return mig_llc;
    ...
}

Should llc_spread_active_balance() check that src_util remains at or above the
threshold after subtracting SCHED_CAPACITY_SCALE (for example, checking
fits_llc_capacity(src_util - SCHED_CAPACITY_SCALE, src_cap, false) or
explicitly checking src_util >= dst_util + 2 * SCHED_CAPACITY_SCALE) so that
the two-CPU gap is enforced?

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