| Message ID | 1522842866-27176-1-git-send-email-Michal.Kalderon@cavium.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On 4/4/2018 7:54 AM, 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@cavium.com> > Signed-off-by: Ariel Elior <Ariel.Elior@cavium.com> > --- Thanks, this version looks much better for me. Reviewed-by: Sinan Kaya <okaya@codeaurora.org>
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@cavium.com> > Signed-off-by: Ariel Elior <Ariel.Elior@cavium.com> > 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? Are you sure this is the right form of concurrency for qe_wr_id? Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> From: Jason Gunthorpe [mailto:jgg@mellanox.com] > 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@cavium.com> > > Signed-off-by: Ariel Elior <Ariel.Elior@cavium.com> > > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Apr 04, 2018 at 07:02:40PM +0000, Kalderon, Michal wrote: > > From: Jason Gunthorpe [mailto:jgg@mellanox.com] > > 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@cavium.com> > > > Signed-off-by: Ariel Elior <Ariel.Elior@cavium.com> > > > 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 Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> From: Jason Gunthorpe [mailto:jgg@mellanox.com] > 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@mellanox.com] > > > 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@cavium.com> > > > > Signed-off-by: Ariel Elior <Ariel.Elior@cavium.com> > > > > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/infiniband/hw/qedr/verbs.c b/drivers/infiniband/hw/qedr/verbs.c index 875b172..3868bf0 100644 --- a/drivers/infiniband/hw/qedr/verbs.c +++ 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++;