> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
