Re: [PATCH] sched/fair: Adjust the allowed NUMA imbalance when SD_NUMA spans multiple LLCs

[Peter Zijlstra <peterz@infradead.org>]

On Thu, Oct 28, 2021 at 02:03:05PM +0100, Mel Gorman wrote:

> @@ -1926,8 +1926,8 @@ static void task_numa_find_cpu(struct task_numa_env *env,
>  		src_running = env->src_stats.nr_running - 1;
>  		dst_running = env->dst_stats.nr_running + 1;
>  		imbalance = max(0, dst_running - src_running);
> -		imbalance = adjust_numa_imbalance(imbalance, dst_running,
> -							env->dst_stats.weight);
> +		imbalance = adjust_numa_imbalance(imbalance, env->dst_cpu,
> +					dst_running, env->dst_stats.weight);

Can we please align at (0 ?

>  
>  		/* Use idle CPU if there is no imbalance */
>  		if (!imbalance) {

> diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
> index 4e8698e62f07..08fb02510967 100644
> --- a/kernel/sched/topology.c
> +++ b/kernel/sched/topology.c
> @@ -644,6 +644,7 @@ static void destroy_sched_domains(struct sched_domain *sd)
>  DEFINE_PER_CPU(struct sched_domain __rcu *, sd_llc);
>  DEFINE_PER_CPU(int, sd_llc_size);
>  DEFINE_PER_CPU(int, sd_llc_id);
> +DEFINE_PER_CPU(int, sd_numaimb_shift);

Why does it make sense for this to be a per-cpu variable? Yes, I suppose
people can get creative with cpusets, but what you're trying to capture
seems like a global system propery, no?

At most this seems to want to be a sched_domain value.

>  DEFINE_PER_CPU(struct sched_domain_shared __rcu *, sd_llc_shared);
>  DEFINE_PER_CPU(struct sched_domain __rcu *, sd_numa);
>  DEFINE_PER_CPU(struct sched_domain __rcu *, sd_asym_packing);
> @@ -672,6 +673,20 @@ static void update_top_cache_domain(int cpu)
>  	sd = lowest_flag_domain(cpu, SD_NUMA);
>  	rcu_assign_pointer(per_cpu(sd_numa, cpu), sd);
>  
> +	/*
> +	 * Save the threshold where an imbalance is allowed between SD_NUMA
> +	 * domains. If LLC spans the entire node, then imbalances are allowed
> +	 * until 25% of the domain is active. Otherwise, allow an imbalance
> +	 * up to the point where LLCs between NUMA nodes should be balanced
> +	 * to maximise cache and memory bandwidth utilisation.
> +	 */
> +	if (sd) {
> +		if (sd->span_weight == size)
> +			per_cpu(sd_numaimb_shift, cpu) = 2;
> +		else
> +			per_cpu(sd_numaimb_shift, cpu) = max(2, ilog2(sd->span_weight / size * num_online_nodes()));
> +	}
> +
>  	sd = highest_flag_domain(cpu, SD_ASYM_PACKING);
>  	rcu_assign_pointer(per_cpu(sd_asym_packing, cpu), sd);

I think I'm with Valentin here, this seems like something that wants to
use the sd/sd->child relation.

That also makes this the wrong place to do things since this is after
the degenerate code.

Perhaps this can be done in sd_init(), after all, we build the thing
bottom-up, so by the time we initialize the NODE, the MC level should
already be present.

I'm thinking you can perhaps use something like:

	if (!(sd->flags & SD_SHARE_PKG_RESROUCES) &&
	    (child->flags & SD_SHARE_PKG_RESOURCES)) {

		/* this is the first domain not sharing LLC */
		sd->new_magic_imb = /*  magic incantation goes here */
	}