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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87wmhs1063.ffs@tglx>
Date: Mon, 28 Oct 2024 20:45:40 +0100
From: Thomas Gleixner <tglx@...utronix.de>
To: Aleksandar Rikalo <arikalo@...il.com>, Thomas Bogendoerfer
 <tsbogend@...ha.franken.de>
Cc: Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, Vladimir Kondratiev
 <vladimir.kondratiev@...ileye.com>, Gregory CLEMENT
 <gregory.clement@...tlin.com>, Theo Lebrun <theo.lebrun@...tlin.com>, Arnd
 Bergmann <arnd@...db.de>, devicetree@...r.kernel.org, Djordje Todorovic
 <djordje.todorovic@...cgroup.com>, Chao-ying Fu <cfu@...ecomp.com>, Daniel
 Lezcano <daniel.lezcano@...aro.org>, Geert Uytterhoeven
 <geert@...ux-m68k.org>, Greg Ungerer <gerg@...nel.org>, Hauke Mehrtens
 <hauke@...ke-m.de>, Ilya Lipnitskiy <ilya.lipnitskiy@...il.com>, Jiaxun
 Yang <jiaxun.yang@...goat.com>, linux-kernel@...r.kernel.org,
 linux-mips@...r.kernel.org, Marc Zyngier <maz@...nel.org>, Paul Burton
 <paulburton@...nel.org>, Peter Zijlstra <peterz@...radead.org>, Serge
 Semin <fancer.lancer@...il.com>, Tiezhu Yang <yangtiezhu@...ngson.cn>,
 Aleksandar Rikalo <arikalo@...il.com>
Subject: Re: [PATCH v8 04/13] irqchip/mips-gic: Multi-cluster support

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