Re: [RFC PATCH 1/6] sched/fair: Add SMT sibling push in periodic load balance

[Peter Zijlstra <peterz@infradead.org>]

On Tue, Oct 20, 2026 at 04:37:41PM +0800, Li kunyu wrote:

Just a few remarks on coding style while I try and make sense of the
whole thing. Don't go rush out a new version with this stuff fixed
before you've seen feedback on the higher level stuff.

> @@ -14226,6 +14244,94 @@ update_newidle_cost(struct sched_domain *sd, u64 cost, unsigned int success)
>  	return false;
>  }
>  
> +/*
> + * An exciting thing is that we have implemented an efficient push
> + * function process.
> + *
> + * It is applied in cycle load balancing and nohz_kick process.
> + * It can help us solve the problem of high workload of CFS tasks in
> + * the RQ queue.
> + */
> +static bool smt_buddy_move_task(struct rq *src_rq)
> +{
> +	bool is_overload, is_move = false;
> +	int src_cpu = cpu_of(src_rq), smt_cpu;
> +	unsigned long src_cpu_runnable, smt_cpu_runnable;
> +	struct sched_domain *smt_sd;
> +	struct task_struct *p;
> +	struct rq *smt_rq;
> +	struct rq_flags rf;
> +
> +	if (!sched_feat(SMT_BUDDY_ASSIST) || !sched_smt_active())
> +		goto exit;
> +
> +restart:
> +	if (src_rq->nr_running < 2)
> +		goto exit;

Did this want to be:

	while (src_rq->nr_running > 1) {

> +
> +	rcu_read_lock();

	scoped_guard (rcu) {

> +
> +	is_overload = false;
> +	src_cpu_runnable = cpu_runnable(src_rq);
> +	for_each_cpu(smt_cpu, cpu_smt_mask(src_cpu)) {
> +		if (smt_cpu == src_cpu)
> +			continue;
> +
> +		smt_cpu_runnable = cpu_runnable(cpu_rq(smt_cpu));
> +		if ((src_cpu_runnable * 100 > smt_cpu_runnable * sysctl_smt_buddy_imbalance_pct)
> +			&& cpu_active(smt_cpu)) {

This is horrifically bad indenting. Operators go at the end of the
previous line and then you indent to the level of whatever ( is active.
If you're a vim user 'cino=(0' gets you that.

> +			is_overload = true;
> +			break;
> +		}
> +	}
> +
> +	if (!is_overload) {
> +		rcu_read_unlock();
> +		goto exit;

This pattern should scream at you for doing it wrong.

> +	}
> +
> +	smt_sd = lowest_flag_domain(src_cpu, SD_SHARE_CPUCAPACITY);
> +	if (!smt_sd) {
> +		rcu_read_unlock();
> +		goto exit;

And again.

> +	}
> +
> +	rcu_read_unlock();
	}
> +
> +	rq_lock_irqsave(src_rq, &rf);

	scoped_guard (irqsave) {
		scoped_guard (rq_lock, src_rq)

> +	smt_rq = cpu_rq(smt_cpu);
> +
> +	struct lb_env env = {
> +		.sd             = smt_sd,
> +		.dst_cpu        = smt_cpu,
> +		.dst_rq         = smt_rq,
> +		.src_cpu        = src_cpu,
> +		.src_rq         = src_rq,
> +		.idle           = 0,		/* __CPU_NOT_IDLE */
> +		.flags          = LBF_ACTIVE_LB,
> +	};
> +
> +	/* Update the src_rq clock only once when migrating tasks. */
> +	if (!is_move)
> +		update_rq_clock(src_rq);
> +
> +	p = detach_one_task(&env);
> +	if (!p) {
> +		rq_unlock_irqrestore(src_rq, &rf);
> +		goto exit;

More of the same bad pattern.
	
> +	}
> +
> +	rq_unlock(src_rq, &rf);
		}
> +	attach_one_task(smt_rq, p);
> +	local_irq_restore(rf.flags);
	}
> +	is_move = true;
> +
> +	goto restart;
	}
	
> +
> +exit:
> +	return is_move;
> +}