Messages in this thread | | | Date | Mon, 8 Nov 2021 12:14:27 +0100 | From | Peter Zijlstra <> | Subject | Re: [PATCH] sched/fair: Adjust the allowed NUMA imbalance when SD_NUMA spans multiple LLCs |
| |
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 */ }
| |