Re: [PATCH v8 04/13] irqchip/mips-gic: Multi-cluster support

[Thomas Gleixner <tglx@linutronix.de>]

On Mon, Oct 28 2024 at 18:59, Aleksandar Rikalo wrote:
> + * In summary, if this function returns true then the caller should access GIC
> + * registers using redirect register block accessors & then call
> + * mips_cm_unlock_other() when done. If this function returns false then the
> + * caller should trivially access GIC registers in the local cluster.
> + *
> + * Returns true if locking performed, else false.
> + */
> +static bool gic_irq_lock_cluster(struct irq_data *d)
> +{
> +	unsigned int cpu, cl;
> +
> +	cpu = cpumask_first(irq_data_get_effective_affinity_mask(d));
> +	BUG_ON(cpu >= NR_CPUS);
> +
> +	cl = cpu_cluster(&cpu_data[cpu]);
> +	if (cl == cpu_cluster(&current_cpu_data))
> +		return false;

Not that I personally care much about the performance of this. But why
aren't you caching the cluster or at least the target CPU in irq_data
somewhere? cpumask_first() is not cheap on a large system when the cpu
is at the very end of the bitmask. AFAICT nothing here uses chip_data,
so you can do:

       unsigned long cl = (unsigned long)irq_data_get_irq_chip_data(d);

which is a single load operation and you can update it in the
set_affinity() callback and during setup. No?

I'll take the irqchip bits as is if nobody complains within the next
days and you can optimize on top if you care.

Thanks,

        tglx