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

[Michael Ellerman]

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.

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