Re: [PATCH v3] sched_ext: Introduce NUMA awareness to the default idle selection policy

[Tejun Heo <tj@kernel.org>]

Hello,

On Sun, Oct 27, 2024 at 06:49:53PM +0100, Andrea Righi wrote:
...
> +static void update_selcpu_topology(void)
>  {
> +	bool enable_llc = false, enable_numa = false;
> +	s32 cpu;
>  
> +	rcu_read_lock();
> +	for_each_possible_cpu(cpu) {
> +		struct sched_domain *sd;
> +		const struct cpumask *cpus;
> +
> +		sd = rcu_dereference(per_cpu(sd_llc, cpu));
> +		if (sd) {
> +			cpus = sched_domain_span(sd);
> +			pr_debug("sched_ext: LLC cpu%d: %*pbl\n",
> +				 cpu, cpumask_pr_args(cpus));
> +			if (cpumask_weight(cpus) < num_possible_cpus())
> +				enable_llc = true;
> +		}
> +
> +		sd = highest_flag_domain(cpu, SD_NUMA);
> +		if (sd) {
> +			cpus = sched_group_span(sd->groups);
> +			pr_debug("sched_ext: NUMA cpu%d: %*pbl\n",
> +				 cpu, cpumask_pr_args(cpus));
> +			if (cpumask_weight(cpus) < num_possible_cpus())
> +				enable_numa = true;

This isn't a big problem but the loop looks a bit odd in that once both are
set, it's obvious that there's no reason to continue. Doesn't this need to
check only one CPU in fact? e.g. for the first possible CPU, if sd doesn't
exist or cpumask_weight(sd) == num_possible_cpu(), don't we know for sure
that llc or numa doesn't need to be enabled and vice-versa?

>  static s32 scx_select_cpu_dfl(struct task_struct *p, s32 prev_cpu,
>  			      u64 wake_flags, bool *found)
>  {
> +	const struct cpumask *llc_cpus = NULL;
> +	const struct cpumask *numa_cpus = NULL;
>  	s32 cpu;
>  
>  	*found = false;
...
> +	if (p->nr_cpus_allowed >= num_possible_cpus()) {
> +		if (static_branch_unlikely(&scx_selcpu_topo_numa))
> +			numa_cpus = cpumask_of_node(cpu_to_node(prev_cpu));
> +
> +		if (static_branch_unlikely(&scx_selcpu_topo_llc)) {

static_branch_maybe() is probably the better one for llc.

Thanks.

-- 
tejun