[PATCH 2/5] powerpc/smp: add set_cpus_related()

[Oliver O'Halloran]

On Wed, Mar 15, 2017 at 10:18 PM, Michael Ellerman <mpe at ellerman.id.au> wrote:
> Oliver O'Halloran <oohall at 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