Re: [PATCH 2/5] powerpc/smp: add set_cpus_related()
From: Michael Ellerman <mpe@ellerman.id.au>
Date: 2017-03-15 11:18:31
Oliver O'Halloran [off-list ref] writes:
quoted hunk ↗ jump to hunk
diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c index dfe0e1d9cd06..1c531887ca51 100644 --- a/arch/powerpc/kernel/smp.c +++ b/arch/powerpc/kernel/smp.c@@ -377,6 +377,25 @@ static void smp_store_cpu_info(int id) #endif } +/* + * Relationships between CPUs are maintained in a set of per-cpu cpumasks. We + * need to ensure that they are kept consistant between CPUs when they are + * changed. + * + * This is slightly tricky since the core mask must be a strict superset of + * the sibling mask. + */ +static void set_cpus_related(int i, int j, bool related, struct cpumask *(*relation_fn)(int)) +{ + if (related) { + cpumask_set_cpu(i, relation_fn(j)); + cpumask_set_cpu(j, relation_fn(i)); + } else { + cpumask_clear_cpu(i, relation_fn(j)); + cpumask_clear_cpu(j, relation_fn(i)); + } +}
I think you pushed the abstraction one notch too far on this one, or
perhaps not far enough.
We end up with a function called "set" that might clear, depending on a
bool you pass. Which is hard to parse, eg:
set_cpus_related(cpu, base + i, false, cpu_sibling_mask);
And I know there's two places where we pass an existing bool "add", but
there's four where we pass true or false.
If we want to push it in that direction I think we should just pass the
set/clear routine instead of the flag, so:
do_cpus_related(cpu, base + i, cpumask_clear_cpu, cpu_sibling_mask);
But that might be overdoing it.
So I think we should just do:
static void set_cpus_related(int i, int j, struct cpumask *(*mask_func)(int))
{
cpumask_set_cpu(i, mask_func(j));
cpumask_set_cpu(j, mask_func(i));
}
static void clear_cpus_related(int i, int j, struct cpumask *(*mask_func)(int))
{
cpumask_clear_cpu(i, mask_func(j));
cpumask_clear_cpu(j, mask_func(i));
}
So the cases with add become:
if (add)
set_cpus_related(cpu, i, cpu_core_mask(i));
else
clear_cpus_related(cpu, i, cpu_core_mask(i));
Which is not as pretty but more explicit.
And the other cases look much better, eg:
clear_cpus_related(cpu, base + i, cpu_sibling_mask);
??
cheers