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. 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