ib: Eliminate duplicate barriers on weakly-ordered archs
Subsystems (4)
v5 cover ib: Eliminate duplicate barriers on weakly-ordered archs
Code includes wmb() followed by writel() in multiple places. writel() already has a barrier on some architectures like arm64.
This ends up CPU observing two barriers back to back before executing the register write.
Since code already has an explicit barrier call, changing writel() to writel_relaxed().
I did a regex search for wmb() followed by writel() in each drivers directory.
I scrubbed the ones I care about in this series.
I considered "ease of change", "popular usage" and "performance critical path" as the determining criteria for my filtering.
We used relaxed API heavily on ARM for a long time but
it did not exist on other architectures. For this reason, relaxed architectures have been paying double penalty in order to use the common drivers.
Now that relaxed API is present on all architectures, we can go and scrub all drivers to see what needs to change and what can remain.
We start with mostly used ones and hope to increase the coverage over time. It will take a while to cover all drivers.
Changes since v4:
- collect reviewed and acked bys
- drop cxgb4
Sinan Kaya (3):
RDMA/bnxt_re: Eliminate duplicate barriers on weakly-ordered archs RDMA/i40iw: Eliminate duplicate barriers on weakly-ordered archs RDMA/qedr: eliminate duplicate barriers on weakly-ordered archs #2
drivers/infiniband/hw/bnxt_re/qplib_rcfw.c | 9 +++++---- drivers/infiniband/hw/i40iw/i40iw_ctrl.c | 6 ++++-- drivers/infiniband/hw/i40iw/i40iw_osdep.h | 1 + drivers/infiniband/hw/i40iw/i40iw_uk.c | 3 ++- drivers/infiniband/hw/i40iw/i40iw_utils.c | 11 +++++++++++ drivers/infiniband/hw/qedr/verbs.c | 4 ++-- 6 files changed, 25 insertions(+), 9 deletions(-)
Signature · 1 line
2.7.4
PATCH v5 1/3 RDMA/bnxt_re: Eliminate duplicate barriers on weakly-ordered archs
Code includes wmb() followed by writel(). writel() already has a barrier on some architectures like arm64.
This ends up CPU observing two barriers back to back before executing the register write.
Since code already has an explicit barrier call, changing writel() to writel_relaxed().
--- drivers/infiniband/hw/bnxt_re/qplib_rcfw.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
drivers/infiniband/hw/bnxt_re/qplib_rcfw.c +5 -4
@@ -181,10 +181,11 @@ static int __send_message(struct bnxt_qplib_rcfw *rcfw, struct cmdq_base *req,/* ring CMDQ DB */ wmb (); -writel (cmdq_prod, rcfw->cmdq_bar_reg_iomem + - rcfw->cmdq_bar_reg_prod_off); -writel (RCFW_CMDQ_TRIG_VAL , rcfw->cmdq_bar_reg_iomem + - rcfw->cmdq_bar_reg_trig_off); +writel_relaxed (cmdq_prod, rcfw->cmdq_bar_reg_iomem + + rcfw->cmdq_bar_reg_prod_off); +writel_relaxed (RCFW_CMDQ_TRIG_VAL , rcfw->cmdq_bar_reg_iomem + + rcfw->cmdq_bar_reg_trig_off); +mmiowb (); done:spin_unlock_irqrestore (&cmdq->lock, flags);/* Return the CREQ response pointer */
Signature · 1 line
2.7.4
PATCH v5 2/3 RDMA/i40iw: Eliminate duplicate barriers on weakly-ordered archs
In reply to Sinan KayaCode includes wmb() followed by writel(). writel() already has a barrier on some architectures like arm64.
This ends up CPU observing two barriers back to back before executing the register write.
Create a new wrapper function with relaxed write operator. Use the new wrapper when a write is following a wmb().
Since code already has an explicit barrier call, changing writel() to writel_relaxed().
--- drivers/infiniband/hw/i40iw/i40iw_ctrl.c | 6 ++++-- drivers/infiniband/hw/i40iw/i40iw_osdep.h | 1 + drivers/infiniband/hw/i40iw/i40iw_uk.c | 3 ++- drivers/infiniband/hw/i40iw/i40iw_utils.c | 11 +++++++++++ 4 files changed, 18 insertions(+), 3 deletions(-)
drivers/infiniband/hw/i40iw/i40iw_ctrl.c +4 -2
@@ -706,9 +706,11 @@ static void i40iw_sc_ccq_arm(struct i40iw_sc_cq *ccq)wmb ();/* make sure shadow area is updated before arming */ if (ccq->dev->is_pf) -i40iw_wr32 (ccq->dev->hw,I40E_PFPE_CQARM , ccq->cq_uk.cq_id); +i40iw_wr32_relaxed (ccq->dev->hw,I40E_PFPE_CQARM , + ccq->cq_uk.cq_id);else -i40iw_wr32 (ccq->dev->hw,I40E_VFPE_CQARM1 , ccq->cq_uk.cq_id); +i40iw_wr32_relaxed (ccq->dev->hw,I40E_VFPE_CQARM1 , + ccq->cq_uk.cq_id); }/**
drivers/infiniband/hw/i40iw/i40iw_osdep.h +1 -0
@@ -213,5 +213,6 @@ void i40iw_hw_stats_start_timer(struct i40iw_sc_vsi *vsi);void i40iw_hw_stats_stop_timer (struct i40iw_sc_vsi *vsi);#define i40iw_mmiowb () mmiowb()void i40iw_wr32 (struct i40iw_hw *hw,u32 reg,u32 value); +void i40iw_wr32_relaxed (struct i40iw_hw *hw,u32 reg,u32 value);u32 i40iw_rd32 (struct i40iw_hw *hw,u32 reg);#endif /* _I40IW_OSDEP_H_ */
drivers/infiniband/hw/i40iw/i40iw_uk.c +2 -1
@@ -723,7 +723,8 @@ static void i40iw_cq_request_notification(struct i40iw_cq_uk *cq,wmb ();/* make sure WQE is populated before valid bit is set */ -writel (cq->cq_id, cq->cqe_alloc_reg); +writel_relaxed (cq->cq_id, cq->cqe_alloc_reg); +mmiowb (); }/**
drivers/infiniband/hw/i40iw/i40iw_utils.c +11 -0
@@ -125,6 +125,17 @@ inline void i40iw_wr32(struct i40iw_hw *hw, u32 reg, u32 value) }/** +* i40iw_wr32_relaxed - write 32 bits to hw register without ordering +* @hw: hardware information including registers +* @reg: register offset +* @value: vvalue to write to register +*/ +inline void i40iw_wr32_relaxed (struct i40iw_hw *hw,u32 reg,u32 value) +{ +writel_relaxed (value, hw->hw_addr + reg); +} + +/** * i40iw_rd32 - read a 32 bit hw register * @hw: hardware information including registers * @reg: register offset
Signature · 1 line
2.7.4
Re: [PATCH v5 2/3] RDMA/i40iw: Eliminate duplicate barriers on weakly-ordered archs
drivers/infiniband/hw/i40iw/i40iw_ctrl.c static void i40iw_sc_ccq_arm(struct i40iw_sc_cq *ccq) · line 713
Quoted 12 lines · Sinan Kaya
+++
b/drivers/infiniband/hw/i40iw/i40iw_ctrl.cdrivers/infiniband/hw/i40iw/i40iw_ctrl.c +4
-2@@ -706,9 +706,11 @@ static void i40iw_sc_ccq_arm(struct i40iw_sc_cq *ccq)wmb ();/* make sure shadow area is updated before arming */ if (ccq->dev->is_pf) -i40iw_wr32 (ccq->dev->hw,I40E_PFPE_CQARM , ccq->cq_uk.cq_id); +i40iw_wr32_relaxed (ccq->dev->hw,I40E_PFPE_CQARM , + ccq->cq_uk.cq_id);else -i40iw_wr32 (ccq->dev->hw,I40E_VFPE_CQARM1 , ccq->cq_uk.cq_id); +i40iw_wr32_relaxed (ccq->dev->hw,I40E_VFPE_CQARM1 , + ccq->cq_uk.cq_id);
do we want an mmiowb() here?
Quoted 1 line
(quoted diff) +0
-0}
Signature · 3 lines
Sinan Kaya Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
PATCH v5 3/3 RDMA/qedr: eliminate duplicate barriers on weakly-ordered archs #2
In reply to Sinan KayaCode includes wmb() followed by writel() in multiple places. writel() already has a barrier on some architectures like arm64.
This ends up CPU observing two barriers back to back before executing the register write.
Since code already has an explicit barrier call, changing writel() to writel_relaxed().
--- drivers/infiniband/hw/qedr/verbs.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/infiniband/hw/qedr/verbs.c +2 -2
@@ -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);/* Make sure write would stick */ mmiowb ();
@@ -3338,7 +3338,7 @@ int qedr_post_recv(struct ib_qp *ibqp, struct ib_recv_wr *wr, qp->rq.db_data.data.value++; -writel (qp->rq.db_data.raw, qp->rq.db); +writel_relaxed (qp->rq.db_data.raw, qp->rq.db);/* Make sure write sticks */ mmiowb ();
Signature · 1 line
2.7.4
Re: [PATCH v5 3/3] RDMA/qedr: eliminate duplicate barriers on weakly-ordered archs #2
drivers/infiniband/hw/qedr/verbs.c static void doorbell_cq(struct qedr_cq *cq, u32 cons, u8 flags) · line 863
Quoted 6 lines · Sinan Kaya
drivers/infiniband/hw/qedr/verbs.c +1
-1@@ -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.
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);
@@ -1870,8 +1868,7 @@ static int qedr_update_qp_state(struct qedr_dev *dev,
*/
if (rdma_protocol_roce(&dev->ibdev, 1)) {
- wmb();
- writel_relaxed(qp->rq.db_data.raw, qp->rq.db);
+ writel(qp->rq.db_data.raw, qp->rq.db);
/* Make sure write takes effect */
mmiowb();
}@@ -3275,8 +3272,7 @@ 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(); - writel_relaxed(qp->sq.db_data.raw, qp->sq.db); + writel(qp->sq.db_data.raw, qp->sq.db);
/* Make sure write sticks */
mmiowb();@@ -3362,9 +3358,6 @@ 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();
Signature · 3 lines
Sinan Kaya Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
RE: [PATCH v5 3/3] RDMA/qedr: eliminate duplicate barriers on weakly-ordered archs #2
Quoted 24 lines
From: Sinan Kaya [mailto:okaya at codeaurora.org]
Sent: Tuesday, April 03, 2018 5:30 AM
To: linux-rdma at vger.kernel.org; timur at codeaurora.org; sulrich at codeaurora.org
Cc: linux-arm-msm at vger.kernel.org; linux-arm-kernel at lists.infradead.org; Kalderon, Michal <[email protected]>; Elior, Ariel <[email protected]>; Doug Ledford <[email protected]>; Jason Gunthorpe <[email protected]>; linux-kernel at vger.kernel.org
Subject: Re: [PATCH v5 3/3] RDMA/qedr: eliminate duplicate barriers on weakly-ordered archs #2Review static void doorbell_cq(struct qedr_cq *cq, u32
Quoted 1 line · Sinan Kaya
(quoted diff) +0
-0@@ -860,7 +860,7 @@ static void doorbell_cq(struct qedr_cq *cq, u32cons, u8 flags)
drivers/infiniband/hw/qedr/verbs.c static void doorbell_cq(struct qedr_cq *cq, u32 cons, u8 flags) · line 863
Quoted 5 lines
drivers/infiniband/hw/qedr/verbs.c +1
-1wmb (); 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 ofwriteX()functions, I have been reviewing this code. Under normal circumstances, I can get rid of allwmb()as follows.
Quoted 3 lines
However, I started having my doubts now. Are these
wmb()used as a SMP barrier too?
I can't find anysmp_Xmb()indrivers/infiniband/hw/qedrdirectory.
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.
Quoted 46 lines
static void
doorbell_cq(struct qedr_cq *cq, u32 cons, u8 flags){(quoted diff) +0
-2- /* 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); @@ -1870,8 +1868,7 @@ static intqedr_update_qp_state(struct
qedr_dev*dev,*/ if (rdma_protocol_roce(&dev->ibdev, 1)) {(quoted diff) +2
-4- wmb(); - writel_relaxed(qp->rq.db_data.raw, qp->rq.db); + writel(qp->rq.db_data.raw, qp->rq.db); /* Make sure write takes effect */ mmiowb(); }@@ -3275,8 +3272,7 @@ 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(); - writel_relaxed(qp->sq.db_data.raw, qp->sq.db); + writel(qp->sq.db_data.raw, qp->sq.db); /* Make sure write sticks */mmiowb();(quoted diff) +0
-3@@ -3362,9 +3358,6 @@ 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(); --Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Re: [PATCH v5 3/3] RDMA/qedr: eliminate duplicate barriers on weakly-ordered archs #2
Quoted 2 lines · Michal
The wmb before writel are used to make sure the
HW observes the changes in memory before we trigger the doorbell.
According to Linus, writel() guarantees observability. No extra barrier is necessary.
https://www.mail-archive.com/netdev at vger.kernel.org/msg225806.html
There shouldn't be any wmb() in drivers unless it is used for a very well-known reason.
APIs like readX() and writeX() guarantee observability.
Signature · 3 lines
Sinan Kaya Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Re: [PATCH v5 3/3] RDMA/qedr: eliminate duplicate barriers on weakly-ordered archs #2
In reply to Kalderon, MichalQuoted 28 lines · Michal
Quoted 24 lines
From: Sinan Kaya [mailto:okaya at codeaurora.org]
Sent: Tuesday, April 03, 2018 5:30 AM
To: linux-rdma at vger.kernel.org; timur at codeaurora.org; sulrich at codeaurora.org
Cc: linux-arm-msm at vger.kernel.org; linux-arm-kernel at lists.infradead.org; Kalderon, Michal <[email protected]>; Elior, Ariel <[email protected]>; Doug Ledford <[email protected]>; Jason Gunthorpe <[email protected]>; linux-kernel at vger.kernel.org
Subject: Re: [PATCH v5 3/3] RDMA/qedr: eliminate duplicate barriers on weakly-ordered archs #2Review static void doorbell_cq(struct qedr_cq *cq, u32
Quoted 1 line · Sinan Kaya
(quoted diff) +0
-0@@ -860,7 +860,7 @@ static void doorbell_cq(struct qedr_cq *cq, u32cons, u8 flags)
drivers/infiniband/hw/qedr/verbs.c static void doorbell_cq(struct qedr_cq *cq, u32 cons, u8 flags) · line 863
Quoted 5 lines
drivers/infiniband/hw/qedr/verbs.c +1
-1wmb (); 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 ofwriteX()functions, I have been reviewing this code. Under normal circumstances, I can get rid of allwmb()as follows.Quoted 3 lines
However, I started having my doubts now. Are these
wmb()used as a SMP barrier too?
I can't find anysmp_Xmb()indrivers/infiniband/hw/qedrdirectory.
Quoted 7 lines
Your doubts are in place. You initial patch series modified writel to
writel_relaxedSimply 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,
Jason
RE: [PATCH v5 3/3] RDMA/qedr: eliminate duplicate barriers on weakly-ordered archs #2
Quoted 44 lines
From: Jason Gunthorpe [mailto:jgg at ziepe.ca]
Sent: Tuesday, April 03, 2018 11:04 PMQuoted 31 lines · Michal
Quoted 27 lines
From: Sinan Kaya [mailto:okaya at codeaurora.org]
Sent: Tuesday, April 03, 2018 5:30 AM
To: linux-rdma at vger.kernel.org; timur at codeaurora.org; sulrich at codeaurora.org
Cc: linux-arm-msm at vger.kernel.org;
linux-arm-kernel at lists.infradead.org;
Kalderon, Michal <[email protected]>; Elior, Ariel <[email protected]>; Doug Ledford <[email protected]>; Jason Gunthorpe <[email protected]>; linux-kernel at vger.kernel.org
Subject: Re: [PATCH v5 3/3] RDMA/qedr: eliminate duplicate barriers on weakly-ordered archs #2Quoted 2 lines · Sinan Kaya
(quoted diff) +0
-0@@ -860,7 +860,7 @@ static void doorbell_cq(struct qedr_cq *cq,u32
cons, u8 flags)
drivers/infiniband/hw/qedr/verbs.c static void doorbell_cq(struct qedr_cq *cq, u32 cons, u8 flags) · line 863
Quoted 5 lines
drivers/infiniband/hw/qedr/verbs.c +1
-1wmb (); 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 ofwriteX()functions, I have been reviewing this code. Under normal circumstances, I can get rid of allwmb()as follows.Quoted 3 lines
However, I started having my doubts now. Are these
wmb()used as a SMP barrier too?
I can't find anysmp_Xmb()indrivers/infiniband/hw/qedrdirectory.Quoted 5 lines
Your doubts are in place. You initial patch series modified writel to
writel_relaxedSimply 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 andnot remained in cache when HW accesses it.
Quoted 2 lines
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.
Quoted 6 lines
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.
Quoted 3 lines
Thanks,
Jason