RE: [PATCH v5 3/3] RDMA/qedr: eliminate duplicate barriers on weakly-ordered archs #2

[Kalderon, Michal]

> From: Jason Gunthorpe [mailto:jgg@xxxxxxxx]
> Sent: Tuesday, April 03, 2018 11:04 PM
> 
> On Tue, Apr 03, 2018 at 07:42:28AM +0000, Kalderon, Michal wrote:
> > > From: Sinan Kaya [mailto:okaya@xxxxxxxxxxxxxx]
> > > Sent: Tuesday, April 03, 2018 5:30 AM
> > > To: linux-rdma@xxxxxxxxxxxxxxx; timur@xxxxxxxxxxxxxx;
> > > sulrich@xxxxxxxxxxxxxx
> > > Cc: linux-arm-msm@xxxxxxxxxxxxxxx;
> > > linux-arm-kernel@xxxxxxxxxxxxxxxxxxx;
> > > Kalderon, Michal <Michal.Kalderon@xxxxxxxxxx>; Elior, Ariel
> > > <Ariel.Elior@xxxxxxxxxx>; Doug Ledford <dledford@xxxxxxxxxx>; Jason
> > > Gunthorpe <jgg@xxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx
> > > Subject: Re: [PATCH v5 3/3] RDMA/qedr: eliminate duplicate barriers
> > > on weakly-ordered archs #2
> > >
> > > On 3/22/2018 12:26 PM, Sinan Kaya wrote:
> > > > @@ -860,7 +860,7 @@ static void doorbell_cq(struct qedr_cq *cq,
> > > > u32
> > > cons, u8 flags)
> > > >  	wmb();
> > > >  	cq->db.data.agg_flags = flags;
> > > >  	cq->db.data.value = cpu_to_le32(cons);
> > > > -	writeq(cq->db.raw, cq->db_addr);
> > > > +	writeq_relaxed(cq->db.raw, cq->db_addr);
> > >
> > > Given the direction to get rid of wmb() in front of writeX()
> > > functions, I have been reviewing this code. Under normal
> > > circumstances, I can get rid of all
> > > wmb() as follows.
> > >
> > > However, I started having my doubts now. Are these wmb() used as a
> > > SMP barrier too?
> > > I can't find any smp_Xmb() in drivers/infiniband/hw/qedr directory.
> >
> > Your doubts are in place. You initial patch series modified writel to
> > writel_relaxed Simply removing the wmb is dangerous. The wmb before
> > writel are used to make sure the HW observes the changes in memory
> > before we trigger the doorbell. Smp barriers here wouldn't suffice, as
> > on a single processor. we still need to make sure memory is updated and
> not remained in cache when HW accesses it.
> > Reviewing the qedr barriers, I can find places where this may have not
> > been necessary, But definitely you can't simply remove this wmb barriers.
> 
> As Sinan said, the consensus is that wmb();writel(); is redundant if the only
> purpose of the wmb is to order DMA and system memory.
> 
> So can you review these patches on that basis please? Is the WMB doing
> something else, eg SMP related? If yes, please send a patch adding
> appropriate comments.

Thanks Sinan and Jason for the references and explanations, I've reviewed the wmb
usages in qedr and am about to send a patch that replaces two of them with smp_wmb
and completely removes two of them that given your explanation, turned out to be redundant,
thanks.

> 
> Thanks,
> Jason