| Message ID | 20170302004920.21948-2-oohall@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
Oliver O'Halloran <oohall@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
On Wed, Mar 15, 2017 at 10:18 PM, Michael Ellerman <mpe@ellerman.id.au> wrote: > Oliver O'Halloran <oohall@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
Oliver O'Halloran <oohall@gmail.com> writes: > On Wed, Mar 15, 2017 at 10:18 PM, Michael Ellerman <mpe@ellerman.id.au> wrote: >> Oliver O'Halloran <oohall@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. Yeah I guess I was. > 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); Yeah I guess it's a bit better. But it's still very non-obvious what each of those lines is doing. > I agree that set_cpus_related() is probably a bad name, > make_cpus_related() maybe? Except in the clearing case it's unrelating them. I think the fact that we can't think of a meaningful name is a sign it's not a good API. >> 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. It's annoyingly verbose perhaps, but I don't think pointlessly. Your version passes two CPU numbers, a bool and a function to another function, and based on the bool calls a third or fourth function and passes the CPU numbers and result of the first function. My version(s) above takes two cpu numbers and a mask, and calls one function passing it the CPU numbers and the mask. So it's clearly simpler and more explicit, and because the function names are actually meaningful you can have some hope of determining what they do without looking at the definition every time you see it. cheers
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)); + } +} + void __init smp_prepare_cpus(unsigned int max_cpus) { unsigned int cpu; @@ -616,17 +635,9 @@ static void traverse_siblings_chip_id(int cpu, bool add, int chipid) const struct cpumask *mask = add ? cpu_online_mask : cpu_present_mask; int i; - for_each_cpu(i, mask) { - if (cpu_to_chip_id(i) == chipid) { - if (add) { - cpumask_set_cpu(cpu, cpu_core_mask(i)); - cpumask_set_cpu(i, cpu_core_mask(cpu)); - } else { - cpumask_clear_cpu(cpu, cpu_core_mask(i)); - cpumask_clear_cpu(i, cpu_core_mask(cpu)); - } - } - } + for_each_cpu(i, mask) + if (cpu_to_chip_id(i) == chipid) + set_cpus_related(cpu, i, add, cpu_core_mask); } /* Must be called when no change can occur to cpu_present_mask, @@ -666,23 +677,17 @@ static void traverse_core_siblings(int cpu, bool add) return; } - /* if the chip-id fails then threads which share L2 cache are */ - + /* if the chip-id fails then group siblings by the L2 cache */ l2_cache = cpu_to_l2cache(cpu); mask = add ? cpu_online_mask : cpu_present_mask; for_each_cpu(i, mask) { np = cpu_to_l2cache(i); if (!np) continue; - if (np == l2_cache) { - if (add) { - cpumask_set_cpu(cpu, cpu_core_mask(i)); - cpumask_set_cpu(i, cpu_core_mask(cpu)); - } else { - cpumask_clear_cpu(cpu, cpu_core_mask(i)); - cpumask_clear_cpu(i, cpu_core_mask(cpu)); - } - } + + if (np == l2_cache) + set_cpus_related(cpu, i, add, cpu_core_mask); + of_node_put(np); } of_node_put(l2_cache); @@ -720,15 +725,13 @@ void start_secondary(void *unused) for (i = 0; i < threads_per_core; i++) { if (cpu_is_offline(base + i) && (cpu != base + i)) continue; - cpumask_set_cpu(cpu, cpu_sibling_mask(base + i)); - cpumask_set_cpu(base + i, cpu_sibling_mask(cpu)); + set_cpus_related(cpu, base + i, true, cpu_sibling_mask); /* cpu_core_map should be a superset of * cpu_sibling_map even if we don't have cache * information, so update the former here, too. */ - cpumask_set_cpu(cpu, cpu_core_mask(base + i)); - cpumask_set_cpu(base + i, cpu_core_mask(cpu)); + set_cpus_related(cpu, base + i, true, cpu_core_mask); } traverse_core_siblings(cpu, true); @@ -818,10 +821,8 @@ int __cpu_disable(void) /* Update sibling maps */ base = cpu_first_thread_sibling(cpu); for (i = 0; i < threads_per_core && base + i < nr_cpu_ids; i++) { - cpumask_clear_cpu(cpu, cpu_sibling_mask(base + i)); - cpumask_clear_cpu(base + i, cpu_sibling_mask(cpu)); - cpumask_clear_cpu(cpu, cpu_core_mask(base + i)); - cpumask_clear_cpu(base + i, cpu_core_mask(cpu)); + set_cpus_related(cpu, base + i, false, cpu_sibling_mask); + set_cpus_related(cpu, base + i, false, cpu_core_mask); } traverse_core_siblings(cpu, false);
Add a helper function for updating the per-cpu core and sibling thread cpumasks. This helper just sets (or clears) the relevant bit in the cpumasks each CPU. This is open-coded in several places inside the mask setup code so moving it into a seperate function is a sensible cleanup. Signed-off-by: Oliver O'Halloran <oohall@gmail.com> --- arch/powerpc/kernel/smp.c | 61 ++++++++++++++++++++++++----------------------- 1 file changed, 31 insertions(+), 30 deletions(-)