[PATCH] irqchip/gic-v3: use dsb(ishst) to synchronize data to smp before issuing ipi

[Barry Song]

On Mon, Feb 21, 2022 at 4:05 AM Russell King (Oracle)
<linux at 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 at 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 at 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 at 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