Re: [PATCH 2/5] powerpc/smp: add set_cpus_related()
Oliver O'Halloran writes:
> On Wed, Mar 15, 2017 at 10:18 PM, Michael Ellerman
> wrote:
>> Oliver O'Halloran 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
Re: [PATCH 2/5] powerpc/smp: add set_cpus_related()
On Wed, Mar 15, 2017 at 10:18 PM, Michael Ellerman wrote:
> Oliver O'Halloran 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
Re: [PATCH 2/5] powerpc/smp: add set_cpus_related()
Oliver O'Halloran 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
