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: 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. 

> 
> Are you sure this is the right form of concurrency for qe_wr_id?
We avoid taking an additional lock this way. 

> 
> 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