ib: 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().

Signed-off-by Sinan Kaya <[email protected]>

 drivers/infiniband/hw/bnxt_re/qplib_rcfw.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/infiniband/hw/bnxt_re/qplib_rcfw.c +1 -0

index 8329ec6..4a6b981 100644
+++ b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c

@@ -181,10 +181,10 @@ static int __send_message(struct bnxt_qplib_rcfw *rcfw, struct cmdq_base *req,
 

Quoted 18 lines · Sinan Kaya

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().

Signed-off-by Sinan Kaya <[email protected]>

 drivers/infiniband/hw/bnxt_re/qplib_rcfw.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/infiniband/hw/bnxt_re/qplib_rcfw.c +1 -0

index 8329ec6..4a6b981 100644
+++ b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c

@@ -181,10 +181,10 @@ static int __send_message(struct bnxt_qplib_rcfw *rcfw, struct cmdq_base *req,
 

drivers/infiniband/hw/bnxt_re/qplib_rcfw.c static int __send_message(struct bnxt_qplib_rcfw *rcfw, struct cmdq_base *req, · line 187

Quoted 10 lines

drivers/infiniband/hw/bnxt_re/qplib_rcfw.c +4 -4

 	/* 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);

Woah, this may not be safe..

The definition of writel_relaxed() is that it is fully unordered, so the above two writes may change order now. Broadcom guys would have to ack if that it is OK or not for their hardware.

In general this is not an OK approach for a mechanical conversion.. Only the first writel can be convereted.

You need to check all your patches to make sure there are no subsequent writel's in the places touched.

Jason

Quoted 36 lines · Jason Gunthorpe

Quoted 18 lines · Sinan Kaya

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().

Signed-off-by Sinan Kaya <[email protected]>

 drivers/infiniband/hw/bnxt_re/qplib_rcfw.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/infiniband/hw/bnxt_re/qplib_rcfw.c +1 -0

index 8329ec6..4a6b981 100644
+++ b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c

@@ -181,10 +181,10 @@ static int __send_message(struct bnxt_qplib_rcfw *rcfw, struct cmdq_base *req,
 

drivers/infiniband/hw/bnxt_re/qplib_rcfw.c static int __send_message(struct bnxt_qplib_rcfw *rcfw, struct cmdq_base *req, · line 187

Quoted 10 lines

drivers/infiniband/hw/bnxt_re/qplib_rcfw.c +4 -4

 	/* 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);

Woah, this may not be safe..

The definition of writel_relaxed() is that it is fully unordered, so the above two writes may change order now. Broadcom guys would have to ack if that it is OK or not for their hardware.

Quoted 5 lines

In general this is not an OK approach for a mechanical conversion.. Only the first writel can be convereted.

You need to check all your patches to make sure there are no subsequent writel's in the places touched.

I paid special attention to this one and went to check the barriers document. According to the document, writes (whether it is relaxed or not) are always observed by the HW inorder with respect to each other. It just doesn't guarantee anything above writel_relaxed() to be observed. Since we already have a wmb(), this is taken care of.

If somebody wants things to be observed after register write, there should have been a wmb() or mmiowb() afterwards.

Quoted 5 lines

Jason

Never mind, it will break some architectures. I'll only change the first one.

(1) On some systems, I/O stores are not strongly ordered across all CPUs, and so for _all_ general drivers locks should be used and mmiowb() must be issued prior to unlocking the critical section.

Quoted 39 lines · Jason Gunthorpe

Quoted 18 lines · Sinan Kaya

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().

Signed-off-by Sinan Kaya <[email protected]>

 drivers/infiniband/hw/bnxt_re/qplib_rcfw.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/infiniband/hw/bnxt_re/qplib_rcfw.c +1 -0

index 8329ec6..4a6b981 100644
+++ b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c

@@ -181,10 +181,10 @@ static int __send_message(struct bnxt_qplib_rcfw *rcfw, struct cmdq_base *req,
 

drivers/infiniband/hw/bnxt_re/qplib_rcfw.c static int __send_message(struct bnxt_qplib_rcfw *rcfw, struct cmdq_base *req, · line 187

Quoted 10 lines

drivers/infiniband/hw/bnxt_re/qplib_rcfw.c +4 -4

 	/* 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);

Woah, this may not be safe..

The definition of writel_relaxed() is that it is fully unordered, so the above two writes may change order now. Broadcom guys would have to ack if that it is OK or not for their hardware.

In general this is not an OK approach for a mechanical conversion.. Only the first writel can be convereted.

Quoted 2 lines

You need to check all your patches to make sure there are no subsequent writel's in the places touched.

I paid special attention to this one and went to check the barriers document. According to the document, writes (whether it is relaxed or not) are always observed by the HW inorder with respect to each other.

Quoted 40 lines · Sinan Kaya

Quoted 39 lines · Jason Gunthorpe

Quoted 18 lines · Sinan Kaya

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().

Signed-off-by Sinan Kaya <[email protected]>

 drivers/infiniband/hw/bnxt_re/qplib_rcfw.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/infiniband/hw/bnxt_re/qplib_rcfw.c +1 -0

index 8329ec6..4a6b981 100644
+++ b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c

@@ -181,10 +181,10 @@ static int __send_message(struct bnxt_qplib_rcfw *rcfw, struct cmdq_base *req,
 

drivers/infiniband/hw/bnxt_re/qplib_rcfw.c static int __send_message(struct bnxt_qplib_rcfw *rcfw, struct cmdq_base *req, · line 187

Quoted 10 lines

drivers/infiniband/hw/bnxt_re/qplib_rcfw.c +4 -4

 	/* 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);

Woah, this may not be safe..

The definition of writel_relaxed() is that it is fully unordered, so the above two writes may change order now. Broadcom guys would have to ack if that it is OK or not for their hardware.

In general this is not an OK approach for a mechanical conversion.. Only the first writel can be convereted.

Quoted 6 lines

Quoted 2 lines

You need to check all your patches to make sure there are no subsequent writel's in the places touched.

I paid special attention to this one and went to check the barriers document. According to the document, writes (whether it is relaxed or not) are always observed by the HW inorder with respect to each other.

Oh interesting, that document got revised to make writel_relaxed less relaxed a few years ago, didn't know that. Thanks.

However, this is still not OK, the full code is:

        /* 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);
done:
        spin_unlock_irqrestore(&cmdq->lock, flags);

And the definition of _relaxed allows the writes to order outside the spinlock region, which is very likely to be wrong in this driver.

I'm not sure adding a mmiowb() just to use a writel_relaxed is any sort of win though?

Jason

Quoted 66 lines · Jason Gunthorpe

Quoted 40 lines · Sinan Kaya

Quoted 39 lines · Jason Gunthorpe

Quoted 18 lines · Sinan Kaya

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().

Signed-off-by Sinan Kaya <[email protected]>

 drivers/infiniband/hw/bnxt_re/qplib_rcfw.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/infiniband/hw/bnxt_re/qplib_rcfw.c +1 -0

index 8329ec6..4a6b981 100644
+++ b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c

@@ -181,10 +181,10 @@ static int __send_message(struct bnxt_qplib_rcfw *rcfw, struct cmdq_base *req,
 

drivers/infiniband/hw/bnxt_re/qplib_rcfw.c static int __send_message(struct bnxt_qplib_rcfw *rcfw, struct cmdq_base *req, · line 187

Quoted 10 lines

drivers/infiniband/hw/bnxt_re/qplib_rcfw.c +4 -4

 	/* 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);

Woah, this may not be safe..

The definition of writel_relaxed() is that it is fully unordered, so the above two writes may change order now. Broadcom guys would have to ack if that it is OK or not for their hardware.

In general this is not an OK approach for a mechanical conversion.. Only the first writel can be convereted.

Quoted 6 lines

Quoted 2 lines

You need to check all your patches to make sure there are no subsequent writel's in the places touched.

I paid special attention to this one and went to check the barriers document. According to the document, writes (whether it is relaxed or not) are always observed by the HW inorder with respect to each other.

Oh interesting, that document got revised to make writel_relaxed less relaxed a few years ago, didn't know that. Thanks.

However, this is still not OK, the full code is:

        /* 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);
done:
        spin_unlock_irqrestore(&cmdq->lock, flags);

And the definition of _relaxed allows the writes to order outside the spinlock region, which is very likely to be wrong in this driver.

Quoted 2 lines

I'm not sure adding a mmiowb() just to use a writel_relaxed is any sort of win though?

I'd prefer this.

mmiowb() on ARM64 is empty. mmiowb() guarantees that code also works for PPC too.

I'll switch to this instead so it works for everybody.

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().

Signed-off-by Sinan Kaya <[email protected]>

---
 drivers/infiniband/hw/mlx4/qp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/infiniband/hw/mlx4/qp.c +2 -2

index f045491..74b27b0 100644
@@ -3880,8 +3880,8 @@ int mlx4_ib_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr,
 		 */
 		wmb();
 
-		writel(qp->doorbell_qpn,
-		       to_mdev(ibqp->device)->uar_map + MLX4_SEND_DOORBELL);
+		writel_relaxed(qp->doorbell_qpn,
+			to_mdev(ibqp->device)->uar_map + MLX4_SEND_DOORBELL);
 
 		/*
 		 * Make sure doorbells don't leak out of SQ spinlock

Signature · 1 line

2.7.4

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.

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().

Signed-off-by Sinan Kaya <[email protected]>

 drivers/infiniband/hw/i40iw/i40iw_ctrl.c  |  6 ++++--
 drivers/infiniband/hw/i40iw/i40iw_osdep.h |  1 +
 drivers/infiniband/hw/i40iw/i40iw_uk.c    |  2 +-
 drivers/infiniband/hw/i40iw/i40iw_utils.c | 11 +++++++++++
 4 files changed, 17 insertions(+), 3 deletions(-)

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.

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().

Signed-off-by Sinan Kaya <[email protected]>

drivers/infiniband/hw/i40iw/i40iw_uk.c static void i40iw_cq_request_notification(struct i40iw_cq_uk *cq, · line 727

Quoted 10 lines · Sinan Kaya

drivers/infiniband/hw/i40iw/i40iw_uk.c +2 -1

index 8afa5a6..7f0ebed 100644
+++ b/drivers/infiniband/hw/i40iw/i40iw_uk.c

@@ -723,7 +723,7 @@ 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);
 }

Ah, this one is probably not OK, i40iw_cq_request_notification is called here:

	spin_lock_irqsave(&iwcq->lock, flags);
	ukcq->ops.iw_cq_request_notification(ukcq, cq_notify);
	spin_unlock_irqrestore(&iwcq->lock, flags);

So this needs to add mmmiomb(); to keep the same semantics.

Generally I think you need to be very careful to ensure that any conversion to _relaxed isn't contained by a spinlock, or the mmiomb() is present.

Maybe even do a first series with this obviously correct pattern:

wmb();
writel() -> writel_relaxed()
writel() -> writel_relaxed()
[..]
mmiowmb();

Jason

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().

Signed-off-by Sinan Kaya <[email protected]>

 drivers/infiniband/hw/cxgb4/t4.h | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

drivers/infiniband/hw/cxgb4/t4.h +1 -0

index 8369c7c..6e5658a 100644
+++ b/drivers/infiniband/hw/cxgb4/t4.h

Quoted 1 line

Code includes wmb() followed by writel(). writel() already has a barrier

Quoted 3 lines

some architectures like arm64.

This ends up CPU observing two barriers back to back before executing

the

Review

Quoted 10 lines

register write.

Since code already has an explicit barrier call, changing writel() to writel_relaxed().

Signed-off-by Sinan Kaya <[email protected]>

 drivers/infiniband/hw/cxgb4/t4.h | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

(quoted diff) +0 -0

b/drivers/infiniband/hw/cxgb4/t4.h

Review static inline void pio_copy(u64 __iomem *dst, u64

Quoted 3 lines

index 8369c7c..6e5658a 100644
+++ b/drivers/infiniband/hw/cxgb4/t4.h

(quoted diff) +0 -0

@@ -457,7 +457,7 @@ static inline void pio_copy(u64 __iomem *dst, u64

*src)

Quoted 2 lines

drivers/infiniband/hw/cxgb4/t4.h +0 -0

 	int count = 8;
 

drivers/infiniband/hw/cxgb4/t4.h static inline void pio_copy(u64 __iomem *dst, u64 *src) · line 463

Quoted 6 lines

drivers/infiniband/hw/cxgb4/t4.h +1 -1

 	while (count) {
-		writeq(*src, dst);
+		writeq_relaxed(*src, dst);
 		src++;
 		dst++;
 		count--;

This is another case where writes can be re-ordered.. IIRC dst is WC BAR memory, so the NIC should tolerate re-ordering, but Steve will have to ack this.

Quoted 2 lines · Sinan Kaya

Quoted 1 line

Code includes wmb() followed by writel(). writel() already has a

on

Quoted 13 lines

Quoted 3 lines

some architectures like arm64.

This ends up CPU observing two barriers back to back before executing

the

Quoted 9 lines

register write.

Since code already has an explicit barrier call, changing writel() to writel_relaxed().

Signed-off-by Sinan Kaya <[email protected]>

 drivers/infiniband/hw/cxgb4/t4.h | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

Quoted 5 lines

Review

Quoted 1 line

(quoted diff) +0 -0

b/drivers/infiniband/hw/cxgb4/t4.h

Review static inline void pio_copy(u64 __iomem *dst,

Quoted 3 lines

index 8369c7c..6e5658a 100644
+++ b/drivers/infiniband/hw/cxgb4/t4.h

(quoted diff) +0 -0

@@ -457,7 +457,7 @@ static inline void pio_copy(u64 __iomem *dst,

u64

Quoted 14 lines

*src)

drivers/infiniband/hw/cxgb4/t4.h static inline void pio_copy(u64 __iomem *dst, u64 *src) · line 463

Quoted 8 lines

drivers/infiniband/hw/cxgb4/t4.h +1 -1

 	int count = 8;
 
 	while (count) {
-		writeq(*src, dst);
+		writeq_relaxed(*src, dst);
 		src++;
 		dst++;
 		count--;

This is another case where writes can be re-ordered.. IIRC dst is WC BAR memory, so the NIC should tolerate re-ordering, but Steve will have to ack this.

Yes, this is WC BAR memory. The goal is that pio_copy() will enable

combining this into a single 64B pci-e transaction.

Hi Sinan,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v4.16-rc6 next-20180321]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:
https://github.com/0day-ci/linux/commits/Sinan-Kaya/ib-Eliminate-duplicate-barriers-on-weakly-ordered-archs/20180321-091659
config: xtensa-allyesconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 7.2.0 reproduce:
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross
-O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree make.cross ARCH=xtensa

All errors (new ones prefixed by >>):

In file included from drivers/infiniband/hw/cxgb4/iw_cxgb4.h:73:0,
from drivers/infiniband/hw/cxgb4/device.c:40:
drivers/infiniband/hw/cxgb4/t4.h: In function 'pio_copy':

Quoted 3 lines

Quoted 3 lines

drivers/infiniband/hw/cxgb4/t4.h:460:3: error: implicit declaration of function 'writeq_relaxed'; did you mean 'writel_relaxed'? [-Werror=implicit-function-declaration]

      writeq_relaxed(*src, dst);
      ^~~~~~~~~~~~~~
      writel_relaxed

cc1: some warnings being treated as errors

vim +460 drivers/infiniband/hw/cxgb4/t4.h

   450
   451	/* This function copies 64 byte coalesced work request to memory
   452	 * mapped BAR2 space. For coalesced WRs, the SGE fetches data
   453	 * from the FIFO instead of from Host.
   454	 */
   455	static inline void pio_copy(u64 __iomem *dst, u64 *src)
   456	{
   457		int count = 8;
   458
   459		while (count) {

Quoted 1 line

460 writeq_relaxed(*src, dst);

   461			src++;
   462			dst++;
   463			count--;
   464		}
   465	}
   466

---
0-DAY kernel test infrastructure Open Source Technology
Center
https://lists.01.org/pipermail/kbuild-all Intel
Corporation

Quoted 9 lines · kbuild test robot

Hi Sinan,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v4.16-rc6 next-20180321]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

Quoted 12 lines

url:
https://github.com/0day-ci/linux/commits/Sinan-Kaya/ib-Eliminate-duplicate-barriers-on-weakly-ordered-archs/20180321-091659
config: xtensa-allyesconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 7.2.0 reproduce:
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross
-O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree make.cross ARCH=xtensa

Jason,

Can you remove the writeq change if it is too late for me to fix?

This is an infrastructural issue on xtensa arch.

Probably, it won't get fixed today.

Sinan

Quoted 39 lines

All errors (new ones prefixed by >>):

In file included from drivers/infiniband/hw/cxgb4/iw_cxgb4.h:73:0,
from drivers/infiniband/hw/cxgb4/device.c:40:
drivers/infiniband/hw/cxgb4/t4.h: In function 'pio_copy':

Quoted 3 lines

Quoted 3 lines

drivers/infiniband/hw/cxgb4/t4.h:460:3: error: implicit declaration of function 'writeq_relaxed'; did you mean 'writel_relaxed'? [-Werror=implicit-function-declaration]

      writeq_relaxed(*src, dst);
      ^~~~~~~~~~~~~~
      writel_relaxed

cc1: some warnings being treated as errors

vim +460 drivers/infiniband/hw/cxgb4/t4.h

450
451 /* This function copies 64 byte coalesced work request to
memory
452 * mapped BAR2 space. For coalesced WRs, the SGE fetches data
453 * from the FIFO instead of from Host.
454 */
455 static inline void pio_copy(u64 __iomem *dst, u64 *src)

   456	{
   457		int count = 8;
   458
   459		while (count) {

Quoted 1 line

460 writeq_relaxed(*src, dst);

   461			src++;
   462			dst++;
   463			count--;
   464		}
   465	}
   466

---
0-DAY kernel test infrastructure Open Source Technology
Center
https://lists.01.org/pipermail/kbuild-all Intel
Corporation

Quoted 5 lines

Can you remove the writeq change if it is too late for me to fix?

This is an infrastructural issue on xtensa arch.

Probably, it won't get fixed today.

AFAIS, even writeq won't compile on this arch. I started questioning this build test.

Quoted 5 lines · kbuild test robot

Quoted 4 lines

Hi Sinan,

Thank you for the patch! Yet something to improve:

Quoted 7 lines

Quoted 7 lines

[auto build test ERROR on linus/master]
[also build test ERROR on v4.16-rc6 next-20180321]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:
https://github.com/0day-ci/linux/commits/Sinan-Kaya/ib-Eliminate-

duplicate-barriers-on-weakly-ordered-archs/20180321-091659

Quoted 5 lines

Quoted 5 lines

config: xtensa-allyesconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 7.2.0
reproduce:
wget
https://raw.githubusercontent.com/intel/lkp-

tests/master/sbin/make.cross

Quoted 8 lines

Quoted 5 lines

-O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree make.cross ARCH=xtensa

Jason,

Quoted 5 lines

Can you remove the writeq change if it is too late for me to fix?

This is an infrastructural issue on xtensa arch.

Probably, it won't get fixed today.

AFAIS, even writeq won't compile on this arch. I started questioning this build test.

Quoted 24 lines

Sinan

Quoted 19 lines

All errors (new ones prefixed by >>):

In file included from drivers/infiniband/hw/cxgb4/iw_cxgb4.h:73:0,
from drivers/infiniband/hw/cxgb4/device.c:40:
drivers/infiniband/hw/cxgb4/t4.h: In function 'pio_copy':

Quoted 3 lines

Quoted 3 lines

drivers/infiniband/hw/cxgb4/t4.h:460:3: error: implicit declaration of function 'writeq_relaxed'; did you mean 'writel_relaxed'? [-Werror=implicit-function-declaration]

      writeq_relaxed(*src, dst);
      ^~~~~~~~~~~~~~
      writel_relaxed

cc1: some warnings being treated as errors

vim +460 drivers/infiniband/hw/cxgb4/t4.h

450
451 /* This function copies 64 byte coalesced work request to
memory
452 * mapped BAR2 space. For coalesced WRs, the SGE fetches

data

Quoted 20 lines

Quoted 20 lines

   453	 * from the FIFO instead of from Host.
   454	 */
   455	static inline void pio_copy(u64 __iomem *dst, u64 *src)
   456	{
   457		int count = 8;
   458
   459		while (count) {

Quoted 1 line

460 writeq_relaxed(*src, dst);

   461			src++;
   462			dst++;
   463			count--;
   464		}
   465	}
   466

---
0-DAY kernel test infrastructure Open Source Technology
Center
https://lists.01.org/pipermail/kbuild-all Intel
Corporation

I think all these iw_cxgb4 changes should be reverted until we really have a plan for multi-platform that works.

Quoted 2 lines · Steve Wise

I think all these iw_cxgb4 changes should be reverted until we really have a plan for multi-platform that works.

I know you are looking to have support for PowerPC.

Isn't this a PowerPC problem? Why penalize other architectures?

Do you see anything wrong with the code itself?

I started this thread with the PowerPC develoeprs on your request. "RFC on writel and writel_relaxed"

They are looking into adding the relaxed API support. Support can come in later. Why block this change now?

benh at kernel.crashing.org:
"I've been wanting to implement the relaxed accessors for a while but was battling with this to try to also better support WC, and due to other commitments, this somewhat fell down the cracks."

I have seen four different responses on this thread. Since this is an architecture change it will take a while to get the semantics right. It won't happen in the new few days.

| From: Steve Wise <[email protected]>
| Sent: Thursday, March 22, 2018 9:28 AM
|
| | From: Sinan Kaya <[email protected]>
| | Date: Thursday, March 22, 2018 7:52 AM
| |
| | Isn't this a PowerPC problem? Why penalize other architectures?
| 
| I worry it breaks PPC.

And all other architectures. Aparraently there isn't a formal API description for writel_relaxed() and Co., nor __raw_writel(), etc.

I'm guessing~ that this line in the documentation ~may~ imply the GCC ordering:

... Note that relaxed accesses to
the same peripheral are guaranteed to be ordered with respect to each other. ...

In any case, we really only have a few places where we (the various Chelsio drivers) need to worry about this: the "Fast Paths" where we have a lot of I/O to the device. I think we should leave everything else alone.

Yes, but ...

For instance, I see that the x86 writel() has "memory" in its asm(), which prevents GCC from reordering generated instructions. And it ~looks like~ arm64 ~sort of~ gets that with the inclusion of __iowmb() (which translates to wmb() then dsb(st) which finally holds the GCC "memory" barrier). Is this part of the documented semantic of the writel_relaxed()? The PowerPC stuff simply defines writel_relaxed() as writel() and I can't find the bottom of that Rabbit Hole ...

I'm guessing~ that this line in the documentation ~may~ imply the GCC ordering:

... Note that relaxed accesses to
the same peripheral are guaranteed to be ordered with respect to each other. ...

In any case, we really only have a few places where we (the various Chelsio drivers) need to worry about this: the "Fast Paths" where we have a lot of I/O to the device. I think we should leave everything else alone.

Casey

On 03/22/2018 08:48 AM, [1]okaya at codeaurora.org wrote:

Jason,
Can you remove the writeq change if it is too late for me to fix? This is an infrastructural issue on xtensa arch. Probably, it won't get fixed today.

AFAIS, even writeq won't compile on this arch. I started questioning this build test.

I found out that the solution is this:
#include <linux/io-64-nonatomic-hi-lo.h>

   [2]https://patchwork.ozlabs.org/patch/511801/
   I did a compile test with this change on xtensa and it passed. I'll
   repost with the added diff.
   +++ b/drivers/infiniband/hw/cxgb4/iw_cxgb4.h
   @@ -46,7 +46,7 @@
    #include <linux/timer.h>
    #include <linux/io.h>
    #include <linux/workqueue.h>
   -
   +#include <linux/io-64-nonatomic-hi-lo.h>

Quoted 25 lines · okaya at codeaurora.org

Quoted 9 lines · kbuild test robot

Hi Sinan,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v4.16-rc6 next-20180321]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

Quoted 12 lines

url:
https://github.com/0day-ci/linux/commits/Sinan-Kaya/ib-Eliminate-duplicate-barriers-on-weakly-ordered-archs/20180321-091659
config: xtensa-allyesconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 7.2.0 reproduce:
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross
-O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree make.cross ARCH=xtensa

Jason,

Quoted 5 lines

Can you remove the writeq change if it is too late for me to fix?

This is an infrastructural issue on xtensa arch.

Probably, it won't get fixed today.

AFAIS, even writeq won't compile on this arch. I started questioning this build test.

Quoted 2 lines

AFAIS, even writeq won't compile on this arch. I started questioning this build test.

I have the same confusion. Did you figure out an explanation?

Code includes barrier() 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 barrier().

Signed-off-by Sinan Kaya <[email protected]>

 drivers/infiniband/hw/nes/nes.h       |  5 +++++
 drivers/infiniband/hw/nes/nes_hw.c    | 21 ++++++++++++++-------
 drivers/infiniband/hw/nes/nes_mgt.c   | 15 ++++++++++-----
 drivers/infiniband/hw/nes/nes_nic.c   |  2 +-
 drivers/infiniband/hw/nes/nes_utils.c |  3 ++-
 drivers/infiniband/hw/nes/nes_verbs.c |  5 +++--
 6 files changed, 35 insertions(+), 16 deletions(-)

drivers/infiniband/hw/nes/nes.h +1 -0

index 00c27291..85e007d 100644
+++ b/drivers/infiniband/hw/nes/nes.h

Quoted 22 lines · Sinan Kaya

Code includes barrier() 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 barrier().

Signed-off-by Sinan Kaya <[email protected]>

 drivers/infiniband/hw/nes/nes.h       |  5 +++++
 drivers/infiniband/hw/nes/nes_hw.c    | 21 ++++++++++++++-------
 drivers/infiniband/hw/nes/nes_mgt.c   | 15 ++++++++++-----
 drivers/infiniband/hw/nes/nes_nic.c   |  2 +-
 drivers/infiniband/hw/nes/nes_utils.c |  3 ++-
 drivers/infiniband/hw/nes/nes_verbs.c |  5 +++--
 6 files changed, 35 insertions(+), 16 deletions(-)

drivers/infiniband/hw/nes/nes.h +1 -0

index 00c27291..85e007d 100644
+++ b/drivers/infiniband/hw/nes/nes.h

drivers/infiniband/hw/nes/nes.h static inline void nes_write_indexed(struct nes_device *nesdev, u32 reg_index, u · line 393

Quoted 7 lines

drivers/infiniband/hw/nes/nes.h +4 -0

@@ -387,6 +387,11 @@ static inline void nes_write_indexed(struct nes_device *nesdev, u32 reg_index, u
 	spin_unlock_irqrestore(&nesdev->indexed_regs_lock, flags);
 }
 
+static inline void nes_write32_relaxed(void __iomem *addr, u32 val)
+{
+	writel_relaxed(val, addr);
+}

This wrapper is pointless, let us not add more..

drivers/infiniband/hw/nes/nes_hw.c int nes_destroy_cqp(struct nes_device *nesdev) · line 1261

Quoted 13 lines

drivers/infiniband/hw/nes/nes.h +0 -0

 static inline void nes_write32(void __iomem *addr, u32 val)
 {
 	writel(val, addr);

drivers/infiniband/hw/nes/nes_hw.c +3 -1

index 18a7de1..568e17d 100644
+++ b/drivers/infiniband/hw/nes/nes_hw.c

@@ -1257,7 +1257,8 @@ int nes_destroy_cqp(struct nes_device *nesdev)
 
 	barrier();
 	/* Ring doorbell (5 WQEs) */
-	nes_write32(nesdev->regs+NES_WQE_ALLOC, 0x05800000 | nesdev->cqp.qp_id);
+	nes_write32_relaxed(nesdev->regs+NES_WQE_ALLOC,
+			    0x05800000 | nesdev->cqp.qp_id);

barrier() is not strong enough to order writel, so this doesn't seem right?

It is probably noteven strong enough for what this driver thinks it is doing.. This driver is essentially dead and broken, probably just don't change it.

Jason

Quoted 50 lines · Jason Gunthorpe

Quoted 22 lines · Sinan Kaya

Code includes barrier() 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 barrier().

Signed-off-by Sinan Kaya <[email protected]>

 drivers/infiniband/hw/nes/nes.h       |  5 +++++
 drivers/infiniband/hw/nes/nes_hw.c    | 21 ++++++++++++++-------
 drivers/infiniband/hw/nes/nes_mgt.c   | 15 ++++++++++-----
 drivers/infiniband/hw/nes/nes_nic.c   |  2 +-
 drivers/infiniband/hw/nes/nes_utils.c |  3 ++-
 drivers/infiniband/hw/nes/nes_verbs.c |  5 +++--
 6 files changed, 35 insertions(+), 16 deletions(-)

drivers/infiniband/hw/nes/nes.h +1 -0

index 00c27291..85e007d 100644
+++ b/drivers/infiniband/hw/nes/nes.h

drivers/infiniband/hw/nes/nes.h static inline void nes_write_indexed(struct nes_device *nesdev, u32 reg_index, u · line 393

Quoted 7 lines

drivers/infiniband/hw/nes/nes.h +4 -0

@@ -387,6 +387,11 @@ static inline void nes_write_indexed(struct nes_device *nesdev, u32 reg_index, u
 	spin_unlock_irqrestore(&nesdev->indexed_regs_lock, flags);
 }
 
+static inline void nes_write32_relaxed(void __iomem *addr, u32 val)
+{
+	writel_relaxed(val, addr);
+}

This wrapper is pointless, let us not add more..

drivers/infiniband/hw/nes/nes_hw.c int nes_destroy_cqp(struct nes_device *nesdev) · line 1261

Quoted 13 lines

drivers/infiniband/hw/nes/nes.h +0 -0

 static inline void nes_write32(void __iomem *addr, u32 val)
 {
 	writel(val, addr);

drivers/infiniband/hw/nes/nes_hw.c +3 -1

index 18a7de1..568e17d 100644
+++ b/drivers/infiniband/hw/nes/nes_hw.c

@@ -1257,7 +1257,8 @@ int nes_destroy_cqp(struct nes_device *nesdev)
 
 	barrier();
 	/* Ring doorbell (5 WQEs) */
-	nes_write32(nesdev->regs+NES_WQE_ALLOC, 0x05800000 | nesdev->cqp.qp_id);
+	nes_write32_relaxed(nesdev->regs+NES_WQE_ALLOC,
+			    0x05800000 | nesdev->cqp.qp_id);

barrier() is not strong enough to order writel, so this doesn't seem right?

Quoted 3 lines

It is probably noteven strong enough for what this driver thinks it is doing.. This driver is essentially dead and broken, probably just don't change it.

Just for the sake of other changes in netdev directory and my education...

barrier() on ARM is a wmb(). It should be a compiler barrier on intel.

You are saying barrier() should have been a wmb(), right?

I have gone through similar exercise on netdev directory and changed

barrier()
writel()

to

barrier()
writel_relaxed()

Do you see any problem with this?

Quoted 55 lines · Sinan Kaya

Quoted 50 lines · Jason Gunthorpe

Quoted 22 lines · Sinan Kaya

Code includes barrier() 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 barrier().

Signed-off-by Sinan Kaya <[email protected]>

 drivers/infiniband/hw/nes/nes.h       |  5 +++++
 drivers/infiniband/hw/nes/nes_hw.c    | 21 ++++++++++++++-------
 drivers/infiniband/hw/nes/nes_mgt.c   | 15 ++++++++++-----
 drivers/infiniband/hw/nes/nes_nic.c   |  2 +-
 drivers/infiniband/hw/nes/nes_utils.c |  3 ++-
 drivers/infiniband/hw/nes/nes_verbs.c |  5 +++--
 6 files changed, 35 insertions(+), 16 deletions(-)

drivers/infiniband/hw/nes/nes.h +1 -0

index 00c27291..85e007d 100644
+++ b/drivers/infiniband/hw/nes/nes.h

drivers/infiniband/hw/nes/nes.h static inline void nes_write_indexed(struct nes_device *nesdev, u32 reg_index, u · line 393

Quoted 7 lines

drivers/infiniband/hw/nes/nes.h +4 -0

@@ -387,6 +387,11 @@ static inline void nes_write_indexed(struct nes_device *nesdev, u32 reg_index, u
 	spin_unlock_irqrestore(&nesdev->indexed_regs_lock, flags);
 }
 
+static inline void nes_write32_relaxed(void __iomem *addr, u32 val)
+{
+	writel_relaxed(val, addr);
+}

This wrapper is pointless, let us not add more..

drivers/infiniband/hw/nes/nes_hw.c int nes_destroy_cqp(struct nes_device *nesdev) · line 1261

Quoted 13 lines

drivers/infiniband/hw/nes/nes.h +0 -0

 static inline void nes_write32(void __iomem *addr, u32 val)
 {
 	writel(val, addr);

drivers/infiniband/hw/nes/nes_hw.c +3 -1

index 18a7de1..568e17d 100644
+++ b/drivers/infiniband/hw/nes/nes_hw.c

@@ -1257,7 +1257,8 @@ int nes_destroy_cqp(struct nes_device *nesdev)
 
 	barrier();
 	/* Ring doorbell (5 WQEs) */
-	nes_write32(nesdev->regs+NES_WQE_ALLOC, 0x05800000 | nesdev->cqp.qp_id);
+	nes_write32_relaxed(nesdev->regs+NES_WQE_ALLOC,
+			    0x05800000 | nesdev->cqp.qp_id);

barrier() is not strong enough to order writel, so this doesn't seem right?

Quoted 3 lines

It is probably noteven strong enough for what this driver thinks it is doing.. This driver is essentially dead and broken, probably just don't change it.

Quoted 5 lines

Just for the sake of other changes in netdev directory and my education...

barrier() on ARM is a wmb(). It should be a compiler barrier on intel.

You are saying barrier() should have been a wmb(), right?

Yes, that is my understanding.. barrier() is supposed to be a very weak barrier that just ensures the CPU is locally consistent with itself. It doesn't say anything about DMA access, or SMP cases.

I don't think it is supposed to order anything related to writel_relaxed()

Quoted 7 lines

I have gone through similar exercise on netdev directory and changed

barrier()
writel()

to

Quoted 4 lines

barrier()
writel_relaxed()

Do you see any problem with this?

Seems dangerous as a mechanical change to me, it really depends on why the driver author put barrier() there.

In this case, I strongly suspect nes really intended to say wmb()

Jason

Quoted 1 line

In this case, I strongly suspect nes really intended to say wmb()

Should I change barrier() to wmb() or leave it alone? I have no idea if nes is actively being maintained or if it is EOL.

From: Sinan Kaya [mailto:okaya at codeaurora.org]
Sent: Tuesday, March 20, 2018 4:48 AM

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().

Signed-off-by Sinan Kaya <[email protected]>

---
 drivers/infiniband/hw/qedr/verbs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/infiniband/hw/qedr/verbs.c +1 -1

index ccd55f4..db60360 100644
@@ -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 */

drivers/infiniband/hw/qedr/verbs.c +1 -1

 	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();
--
2.7.4

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().

Signed-off-by Sinan Kaya <[email protected]>

 drivers/infiniband/hw/qedr/verbs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/infiniband/hw/qedr/verbs.c +2 -1

index ccd55f4..db60360 100644
+++ b/drivers/infiniband/hw/qedr/verbs.c

@@ -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();

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.

Feel free to apply patches individually.

Changes since v3:
- https://www.spinics.net/lists/arm-kernel/msg641851.html
- group patches together into subsystems ib:...
- collect reviewed and tested bys
- scrub barrier()

Sinan Kaya (6):
IB/mlx4: Eliminate duplicate barriers on weakly-ordered archs
infiniband: cxgb4: Eliminate duplicate barriers on weakly-ordered
archs

RDMA/bnxt_re: Eliminate duplicate barriers on weakly-ordered archs RDMA/i40iw: Eliminate duplicate barriers on weakly-ordered archs IB/nes: Eliminate duplicate barriers on weakly-ordered archs RDMA/qedr: eliminate duplicate barriers on weakly-ordered archs #2