Messages in this thread Patch in this message | | | From | Barry Song <> | Date | Mon, 21 Feb 2022 09:09:20 +1300 | Subject | Re: [PATCH] irqchip/gic-v3: use dsb(ishst) to synchronize data to smp before issuing ipi |
| |
On Mon, Feb 21, 2022 at 4:05 AM Russell King (Oracle) <linux@armlinux.org.uk> wrote: > > On Sun, Feb 20, 2022 at 02:30:24PM +0100, Ard Biesheuvel wrote: > > On Sat, 19 Feb 2022 at 10:57, Marc Zyngier <maz@kernel.org> wrote: > > > > > > On 2022-02-18 21:55, Barry Song wrote: > > > > dsb(ishst) should be enough here as we only need to guarantee the > > > > visibility of data to other CPUs in smp inner domain before we > > > > send the ipi. > > > > > > > > Signed-off-by: Barry Song <song.bao.hua@hisilicon.com> > > > > --- > > > > drivers/irqchip/irq-gic-v3.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/irqchip/irq-gic-v3.c > > > > b/drivers/irqchip/irq-gic-v3.c > > > > index 5e935d97207d..0efe1a9a9f3b 100644 > > > > --- a/drivers/irqchip/irq-gic-v3.c > > > > +++ b/drivers/irqchip/irq-gic-v3.c > > > > @@ -1211,7 +1211,7 @@ static void gic_ipi_send_mask(struct irq_data > > > > *d, const struct cpumask *mask) > > > > * Ensure that stores to Normal memory are visible to the > > > > * other CPUs before issuing the IPI. > > > > */ > > > > - wmb(); > > > > + dsb(ishst); > > > > > > > > for_each_cpu(cpu, mask) { > > > > u64 cluster_id = MPIDR_TO_SGI_CLUSTER_ID(cpu_logical_map(cpu)); > > > > > > I'm not opposed to that change, but I'm pretty curious whether this > > > makes > > > any visible difference in practice. Could you measure the effect of this > > > change > > > for any sort of IPI heavy workload? > > > > > > > Does this have to be a DSB ? > > Are you suggesting that smp_wmb() may suffice (which is a dmb(ishst)) ?
usually smp_wmb() is ok, for example drivers/irqchip/irq-bcm2836.c:
static void bcm2836_arm_irqchip_ipi_send_mask(struct irq_data *d, const struct cpumask *mask) { int cpu; void __iomem *mailbox0_base = intc.base + LOCAL_MAILBOX0_SET0;
/* * Ensure that stores to normal memory are visible to the * other CPUs before issuing the IPI. */ smp_wmb();
for_each_cpu(cpu, mask) writel_relaxed(BIT(d->hwirq), mailbox0_base + 16 * cpu); }
but the instruction to send ipi for irq-gic-v3.c isn't a store, so this driver has been changed by this commit from dmb(ish) to dsb(sy):
commit 21ec30c0ef5234fb1039cc7c7737d885bf875a9e Author: Shanker Donthineni <shankerd@codeaurora.org>
irqchip/gic-v3: Use wmb() instead of smb_wmb() in gic_raise_softirq()
A DMB instruction can be used to ensure the relative order of only memory accesses before and after the barrier. Since writes to system registers are not memory operations, barrier DMB is not sufficient for observability of memory accesses that occur before ICC_SGI1R_EL1 writes.
A DSB instruction ensures that no instructions that appear in program order after the DSB instruction, can execute until the DSB instruction has completed.
...
diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c index d71be9a1f9d2..d99cc07903ec 100644 --- a/drivers/irqchip/irq-gic-v3.c +++ b/drivers/irqchip/irq-gic-v3.c @@ -688,7 +688,7 @@ static void gic_raise_softirq(const struct cpumask *mask, unsigned int irq) * Ensure that stores to Normal memory are visible to the * other CPUs before issuing the IPI. */ - smp_wmb(); + wmb();
for_each_cpu(cpu, mask) { u64 cluster_id = MPIDR_TO_SGI_CLUSTER_ID(cpu_logical_map(cpu)); my understanding is that dtb(sy) is too strong as this case we are asking data to be observed by other CPUs in smp just as smp_wmb is doing that in other drivers by dmb(isb). we are not requiring data is observed by everyone in the system.
> > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Thanks Barry
| |