On Wed, Mar 15, 2017 at 10:18 PM, Michael Ellerman <m...@ellerman.id.au> wrote: > Oliver O'Halloran <ooh...@gmail.com> writes: >> 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.
I think you're looking at this patch. With the full series applied we never pass a literal to set_cpus_related() directly: [12:14 oliver ~/.../powerpc/kernel (p9-sched $%)]$ gg set_cpus_related smp.c:391:static void set_cpus_related(int i, int j, bool related, struct cpumask *(*relation_fn)(int)) smp.c:647: set_cpus_related(cpu, cpu, add, cpu_core_mask); smp.c:651: set_cpus_related(cpu, i, add, cpu_core_mask); smp.c:685: set_cpus_related(cpu, cpu, onlining, mask_fn); smp.c:697: set_cpus_related(cpu, i, onlining, mask_fn); smp.c:721: set_cpus_related(cpu, base + i, onlining, cpu_sibling_mask); smp.c:736: set_cpus_related(cpu, cpu, onlining, cpu_core_mask); smp.c:746: set_cpus_related(cpu, i, onlining, cpu_core_mask); I agree that set_cpus_related() is probably a bad name, make_cpus_related() maybe? > > 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. I think this would be ok. > > 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)); Dunno, I was trying to get rid of this sort of thing since the logic is duplicated in a lot of places. Seemed to me that it was just pointlessly verbose rather than being helpfully explicit. > > 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