RE: [PATCH] RDMA/qedr: Fix wmb usage in qedr

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> From: Jason Gunthorpe [mailto:jgg@xxxxxxxxxxxx]
> Sent: Thursday, April 05, 2018 1:18 AM
> 
> On Wed, Apr 04, 2018 at 07:02:40PM +0000, Kalderon, Michal wrote:
> > > From: Jason Gunthorpe [mailto:jgg@xxxxxxxxxxxx]
> > > Sent: Wednesday, April 04, 2018 8:48 PM
> > >
> > > On Wed, Apr 04, 2018 at 02:54:26PM +0300, Michal Kalderon wrote:
> > > > This patch comes as a result of Sinan Kaya's patch:
> > > > https://patchwork.kernel.org/patch/10301863/
> > > >
> > > > wmb usages in qedr driver have either been removed where they were
> > > > there only to order DMA accesses, and replaced with smp_wmb and
> > > > comments for the places that the barrier was there for SMP reasons.
> > > >
> > > > Signed-off-by: Michal Kalderon <Michal.Kalderon@xxxxxxxxxx>
> > > > Signed-off-by: Ariel Elior <Ariel.Elior@xxxxxxxxxx>
> > > > drivers/infiniband/hw/qedr/verbs.c | 10 ++++------
> > > >  1 file changed, 4 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/drivers/infiniband/hw/qedr/verbs.c
> > > > b/drivers/infiniband/hw/qedr/verbs.c
> > > > index 875b172..3868bf0 100644
> > > > +++ b/drivers/infiniband/hw/qedr/verbs.c
> > > > @@ -856,8 +856,6 @@ static inline void qedr_init_cq_params(struct
> > > > qedr_cq *cq,
> > > >
> > > >  static void doorbell_cq(struct qedr_cq *cq, u32 cons, u8 flags)  {
> > > > -	/* Flush data before signalling doorbell */
> > > > -	wmb();
> > > >  	cq->db.data.agg_flags = flags;
> > > >  	cq->db.data.value = cpu_to_le32(cons);
> > > >  	writeq(cq->db.raw, cq->db_addr); @@ -1869,7 +1867,6 @@ static
> > > > int qedr_update_qp_state(struct qedr_dev
> > > *dev,
> > > >  			 */
> > > >
> > > >  			if (rdma_protocol_roce(&dev->ibdev, 1)) {
> > > > -				wmb();
> > > >  				writel(qp->rq.db_data.raw, qp->rq.db);
> > > >  				/* Make sure write takes effect */
> > > >  				mmiowb();
> > > > @@ -3256,7 +3253,8 @@ int qedr_post_send(struct ib_qp *ibqp,
> > > > struct
> > > ib_send_wr *wr,
> > > >  	 * unchanged). For performance reasons we avoid checking for this
> > > >  	 * redundant doorbell.
> > > >  	 */
> > > > -	wmb();
> > > > +	/* qp->wqe_wr_id must be updated before completion process */
> > > > +	smp_wmb();
> > > >  	writel(qp->sq.db_data.raw, qp->sq.db);
> > > >
> > > >  	/* Make sure write sticks */
> > > > @@ -3343,8 +3341,8 @@ int qedr_post_recv(struct ib_qp *ibqp,
> > > > struct ib_recv_wr *wr,
> > > >
> > > >  		qedr_inc_sw_prod(&qp->rq);
> > > >
> > > > -		/* Flush all the writes before signalling doorbell */
> > > > -		wmb();
> > > > +		/* qp->rqe_wr_id must be updated before completion
> > > process */
> > > > +		smp_wmb();
> > > >
> > > >  		qp->rq.db_data.data.value++;
> > >
> > > If you are adding smp_wmb() then where is the matching smp_rmb?
> >
> > There is a rmb() in qedr_poll_cq before reading any cqe to make sure
> > all device Written memory is visible. This will also cover for the
> > smp_rmb required for the qe_wr_id.
> 
> Okay, fine by me. The comment should be a bit better though

Thanks, sent a v2 with a clearer comment

> 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




[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Photo]     [Yosemite News]     [Yosemite Photos]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux