netdev: intel: Eliminate duplicate barriers on weakly-ordered archs

PATCH In review Sinan Kaya 4 participants 16 messages Apr 2, 2018 – Apr 3, 2018

Subsystems (4)

Networking Network drivers Arm Qualcomm mailing list Intel Ethernet

lore

Share & tools

lore

Sinan Kaya

author

2018-04-02 19:06 UTC #0 lore raw

v8 cover netdev: intel: 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.

Feel free to apply patches individually.

Change since 7:

* API clarification with Linus and several architecture folks regarding writel()

https://www.mail-archive.com/netdev at vger.kernel.org/msg225806.html

* removed wmb() in front of writel() at several places.
* remove old IA64 comments regarding ordering.

Sinan Kaya (7):
i40e/i40evf: Eliminate duplicate barriers on weakly-ordered archs
ixgbe: eliminate duplicate barriers on weakly-ordered archs
igbvf: eliminate duplicate barriers on weakly-ordered archs
igb: eliminate duplicate barriers on weakly-ordered archs
fm10k: Eliminate duplicate barriers on weakly-ordered archs
ixgbevf: keep writel() closer to wmb()
ixgbevf: eliminate duplicate barriers on weakly-ordered archs

 drivers/net/ethernet/intel/fm10k/fm10k_main.c     | 15 ++-----------
 drivers/net/ethernet/intel/i40e/i40e_txrx.c       | 22 ++++++++----------
 drivers/net/ethernet/intel/i40evf/i40e_txrx.c     |  8 +------
 drivers/net/ethernet/intel/igb/igb_main.c         | 14 ++----------
 drivers/net/ethernet/intel/igbvf/netdev.c         | 16 +++++---------
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c     | 23 ++-----------------
 drivers/net/ethernet/intel/ixgbevf/ixgbevf.h      |  5 -----
 drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 27 ++++++++---------------
 8 files changed, 30 insertions(+), 100 deletions(-)

Signature · 1 line

2.7.4

Sinan Kaya

author

2018-04-02 19:06 UTC #1 lore raw

PATCH v8 1/7 i40e/i40evf: Eliminate duplicate barriers on weakly-ordered archs

memory-barriers.txt has been updated as follows:

"When using writel(), a prior wmb() is not needed to guarantee that the cache coherent memory writes have completed before writing to the MMIO region."

Remove old IA-64 comments in the code along with unneeded wmb() in front of writel().

There are places in the code where wmb() has been used as a double barrier for CPU and IO in place of smp_wmb() and wmb() as an optimization. For such places, keep the wmb() but replace the following writel() with writel_relaxed() to have a sequence as

wmb()
writel_relaxed()
mmio_wb()

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

---
 drivers/net/ethernet/intel/i40e/i40e_txrx.c   | 22 +++++++++-------------
 drivers/net/ethernet/intel/i40evf/i40e_txrx.c |  8 +-------
 2 files changed, 10 insertions(+), 20 deletions(-)

drivers/net/ethernet/intel/i40e/i40e_txrx.c +9 ~~-13~~

index f174c72..1b9fa7a 100644
@@ -186,7 +186,13 @@ static int i40e_program_fdir_filter(struct i40e_fdir_filter *fdir_data,
 	/* Mark the data descriptor to be watched */
 	first->next_to_watch = tx_desc;
 
-	writel(tx_ring->next_to_use, tx_ring->tail);
+	writel_relaxed(tx_ring->next_to_use, tx_ring->tail);
+
+	/* We need this if more than one processor can write to our tail
+	 * at a time, it synchronizes IO on IA64/Altix systems
+	 */
+	mmiowb();
+
 	return 0;
 
 dma_fail:

@@ -1523,12 +1529,6 @@ static inline void i40e_release_rx_desc(struct i40e_ring *rx_ring, u32 val)
 	/* update next to alloc since we have filled the ring */
 	rx_ring->next_to_alloc = val;
 
-	/* Force memory writes to complete before letting h/w
-	 * know there are new descriptors to fetch.  (Only
-	 * applicable for weak-ordered memory model archs,
-	 * such as IA-64).
-	 */
-	wmb();
 	writel(val, rx_ring->tail);
 }

@@ -2274,11 +2274,7 @@ static void i40e_rx_buffer_flip(struct i40e_ring *rx_ring,
 
 static inline void i40e_xdp_ring_update_tail(struct i40e_ring *xdp_ring)
 {
-	/* Force memory writes to complete before letting h/w
-	 * know there are new descriptors to fetch.
-	 */
-	wmb();
-	writel_relaxed(xdp_ring->next_to_use, xdp_ring->tail);
+	writel(xdp_ring->next_to_use, xdp_ring->tail);
 }
 
 /**

@@ -3444,7 +3440,7 @@ static inline int i40e_tx_map(struct i40e_ring *tx_ring, struct sk_buff *skb,
 
 	/* notify HW of packet */
 	if (netif_xmit_stopped(txring_txq(tx_ring)) || !skb->xmit_more) {
-		writel(i, tx_ring->tail);
+		writel_relaxed(i, tx_ring->tail);
 
 		/* we need this if more than one processor can write to our tail
 		 * at a time, it synchronizes IO on IA64/Altix systems

drivers/net/ethernet/intel/i40evf/i40e_txrx.c +1 -7

index 12bd937..eb5556e 100644
@@ -804,12 +804,6 @@ static inline void i40e_release_rx_desc(struct i40e_ring *rx_ring, u32 val)
 	/* update next to alloc since we have filled the ring */
 	rx_ring->next_to_alloc = val;
 
-	/* Force memory writes to complete before letting h/w
-	 * know there are new descriptors to fetch.  (Only
-	 * applicable for weak-ordered memory model archs,
-	 * such as IA-64).
-	 */
-	wmb();
 	writel(val, rx_ring->tail);
 }

@@ -2379,7 +2373,7 @@ static inline void i40evf_tx_map(struct i40e_ring *tx_ring, struct sk_buff *skb,
 
 	/* notify HW of packet */
 	if (netif_xmit_stopped(txring_txq(tx_ring)) || !skb->xmit_more) {
-		writel(i, tx_ring->tail);
+		writel_relaxed(i, tx_ring->tail);
 
 		/* we need this if more than one processor can write to our tail
 		 * at a time, it synchronizes IO on IA64/Altix systems

Signature · 1 line

2.7.4

Bowers

2018-04-03 18:52 UTC #2 lore raw

PATCH v8 1/7 i40e/i40evf: Eliminate duplicate barriers on weakly-ordered archs

Quoted 30 lines

-----Original Message-----
From: Intel-wired-lan [mailto:intel-wired-lan-bounces at osuosl.org] On Behalf Of Sinan Kaya
Sent: Monday, April 2, 2018 12:06 PM
To: Kirsher, Jeffrey T <[email protected]>
Cc: sulrich at codeaurora.org; netdev at vger.kernel.org; timur at codeaurora.org; linux-kernel at vger.kernel.org; Sinan Kaya <[email protected]>; intel-wired-lan at lists.osuosl.org; linux-arm- msm at vger.kernel.org; linux-arm-kernel at lists.infradead.org
Subject: [Intel-wired-lan] [PATCH v8 1/7] i40e/i40evf: Eliminate duplicate barriers on weakly-ordered archs
memory-barriers.txt has been updated as follows:
"When using writel(), a prior wmb() is not needed to guarantee that the cache coherent memory writes have completed before writing to the MMIO region."
Remove old IA-64 comments in the code along with unneeded wmb() in front of writel().
There are places in the code where wmb() has been used as a double barrier for CPU and IO in place of smp_wmb() and wmb() as an optimization. For such places, keep the wmb() but replace the following writel() with writel_relaxed() to have a sequence as
wmb()
writel_relaxed()
mmio_wb()

Quoted 5 lines

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

---
 drivers/net/ethernet/intel/i40e/i40e_txrx.c   | 22 +++++++++-------------
 drivers/net/ethernet/intel/i40evf/i40e_txrx.c |  8 +-------
 2 files changed, 10 insertions(+), 20 deletions(-)

Tested-by Andrew Bowers <[email protected]>

Sinan Kaya

author

2018-04-02 19:06 UTC #3 lore raw

PATCH v8 2/7 ixgbe: eliminate duplicate barriers on weakly-ordered archs

In reply to Sinan Kaya

memory-barriers.txt has been updated as follows:

"When using writel(), a prior wmb() is not needed to guarantee that the cache coherent memory writes have completed before writing to the MMIO region."

Remove old IA-64 comments in the code along with unneeded wmb() in front of writel().

wmb()
writel_relaxed()
mmio_wb()

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

---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 23 ++---------------------
 1 file changed, 2 insertions(+), 21 deletions(-)

drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +2 ~~-21~~

index afadba9..c17924b 100644
@@ -1696,12 +1696,6 @@ void ixgbe_alloc_rx_buffers(struct ixgbe_ring *rx_ring, u16 cleaned_count)
 		/* update next to alloc since we have filled the ring */
 		rx_ring->next_to_alloc = i;
 
-		/* Force memory writes to complete before letting h/w
-		 * know there are new descriptors to fetch.  (Only
-		 * applicable for weak-ordered memory model archs,
-		 * such as IA-64).
-		 */
-		wmb();
 		writel(i, rx_ring->tail);
 	}
 }

@@ -2467,10 +2461,6 @@ static int ixgbe_clean_rx_irq(struct ixgbe_q_vector *q_vector,
 	if (xdp_xmit) {
 		struct ixgbe_ring *ring = adapter->xdp_ring[smp_processor_id()];
 
-		/* Force memory writes to complete before letting h/w
-		 * know there are new descriptors to fetch.
-		 */
-		wmb();
 		writel(ring->next_to_use, ring->tail);
 
 		xdp_do_flush_map();

@@ -8080,12 +8070,7 @@ static int ixgbe_tx_map(struct ixgbe_ring *tx_ring,
 	/* set the timestamp */
 	first->time_stamp = jiffies;
 
-	/*
-	 * Force memory writes to complete before letting h/w know there
-	 * are new descriptors to fetch.  (Only applicable for weak-ordered
-	 * memory model archs, such as IA-64).
-	 *
-	 * We also need this memory barrier to make certain all of the
+	/* We need this memory barrier to make certain all of the
 	 * status bits have been updated before next_to_watch is written.
 	 */
 	wmb();

@@ -8102,7 +8087,7 @@ static int ixgbe_tx_map(struct ixgbe_ring *tx_ring,
 	ixgbe_maybe_stop_tx(tx_ring, DESC_NEEDED);
 
 	if (netif_xmit_stopped(txring_txq(tx_ring)) || !skb->xmit_more) {
-		writel(i, tx_ring->tail);
+		writel_relaxed(i, tx_ring->tail);
 
 		/* we need this if more than one processor can write to our tail
 		 * at a time, it synchronizes IO on IA64/Altix systems

@@ -10034,10 +10019,6 @@ static void ixgbe_xdp_flush(struct net_device *dev)
 	if (unlikely(!ring))
 		return;
 
-	/* Force memory writes to complete before letting h/w know there
-	 * are new descriptors to fetch.
-	 */
-	wmb();
 	writel(ring->next_to_use, ring->tail);
 
 	return;

Signature · 1 line

2.7.4

Bowers

2018-04-03 18:52 UTC #4 lore raw

PATCH v8 2/7 ixgbe: eliminate duplicate barriers on weakly-ordered archs

Quoted 26 lines

-----Original Message-----
From: Intel-wired-lan [mailto:intel-wired-lan-bounces at osuosl.org] On Behalf Of Sinan Kaya
Sent: Monday, April 2, 2018 12:06 PM
To: Kirsher, Jeffrey T <[email protected]>
Cc: sulrich at codeaurora.org; netdev at vger.kernel.org;
timur at codeaurora.org; linux-kernel at vger.kernel.org; Sinan Kaya <[email protected]>; intel-wired-lan at lists.osuosl.org; linux-arm- msm at vger.kernel.org; linux-arm-kernel at lists.infradead.org
Subject: [Intel-wired-lan] [PATCH v8 2/7] ixgbe: eliminate duplicate barriers on weakly-ordered archs
memory-barriers.txt has been updated as follows:
"When using writel(), a prior wmb() is not needed to guarantee that the cache coherent memory writes have completed before writing to the MMIO region."
Remove old IA-64 comments in the code along with unneeded wmb() in front of writel().
There are places in the code where wmb() has been used as a double barrier for CPU and IO in place of smp_wmb() and wmb() as an optimization. For such places, keep the wmb() but replace the following writel() with writel_relaxed() to have a sequence as

Quoted 8 lines

wmb()
writel_relaxed()
mmio_wb()
Signed-off-by Sinan Kaya <[email protected]>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 23 ++---------------------
 1 file changed, 2 insertions(+), 21 deletions(-)

Tested-by Andrew Bowers <[email protected]>

Sinan Kaya

author

2018-04-02 19:06 UTC #5 lore raw

PATCH v8 3/7 igbvf: eliminate duplicate barriers on weakly-ordered archs

In reply to Sinan Kaya

memory-barriers.txt has been updated as follows:

"When using writel(), a prior wmb() is not needed to guarantee that the cache coherent memory writes have completed before writing to the MMIO region."

Remove old IA-64 comments in the code along with unneeded wmb() in front of writel().

wmb()
writel_relaxed()
mmio_wb()

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

---
 drivers/net/ethernet/intel/igbvf/netdev.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

drivers/net/ethernet/intel/igbvf/netdev.c +5 ~~-11~~

index e2b7502..d9f186a 100644
@@ -246,12 +246,6 @@ static void igbvf_alloc_rx_buffers(struct igbvf_ring *rx_ring,
 		else
 			i--;
 
-		/* Force memory writes to complete before letting h/w
-		 * know there are new descriptors to fetch.  (Only
-		 * applicable for weak-ordered memory model archs,
-		 * such as IA-64).
-		*/
-		wmb();
 		writel(i, adapter->hw.hw_addr + rx_ring->tail);
 	}
 }

@@ -2289,16 +2283,16 @@ static inline void igbvf_tx_queue_adv(struct igbvf_adapter *adapter,
 	}
 
 	tx_desc->read.cmd_type_len |= cpu_to_le32(adapter->txd_cmd);
-	/* Force memory writes to complete before letting h/w
-	 * know there are new descriptors to fetch.  (Only
-	 * applicable for weak-ordered memory model archs,
-	 * such as IA-64).
+
+	/* We use this memory barrier to make certain all of the
+	 * status bits have been updated before next_to_watch is
+	 * written.
 	 */
 	wmb();
 
 	tx_ring->buffer_info[first].next_to_watch = tx_desc;
 	tx_ring->next_to_use = i;
-	writel(i, adapter->hw.hw_addr + tx_ring->tail);
+	writel_relaxed(i, adapter->hw.hw_addr + tx_ring->tail);
 	/* we need this if more than one processor can write to our tail
 	 * at a time, it synchronizes IO on IA64/Altix systems
 	 */

Signature · 1 line

2.7.4

Sinan Kaya

author

2018-04-02 19:06 UTC #6 lore raw

PATCH v8 4/7 igb: eliminate duplicate barriers on weakly-ordered archs

In reply to Sinan Kaya

memory-barriers.txt has been updated as follows:

"When using writel(), a prior wmb() is not needed to guarantee that the cache coherent memory writes have completed before writing to the MMIO region."

Remove old IA-64 comments in the code along with unneeded wmb() in front of writel().

wmb()
writel_relaxed()
mmio_wb()

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

---
 drivers/net/ethernet/intel/igb/igb_main.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

drivers/net/ethernet/intel/igb/igb_main.c +2 ~~-12~~

index c1c0bc3..c3f7130 100644
@@ -5652,11 +5652,7 @@ static int igb_tx_map(struct igb_ring *tx_ring,
 	/* set the timestamp */
 	first->time_stamp = jiffies;
 
-	/* Force memory writes to complete before letting h/w know there
-	 * are new descriptors to fetch.  (Only applicable for weak-ordered
-	 * memory model archs, such as IA-64).
-	 *
-	 * We also need this memory barrier to make certain all of the
+	/* We need this memory barrier to make certain all of the
 	 * status bits have been updated before next_to_watch is written.
 	 */
 	wmb();

@@ -5674,7 +5670,7 @@ static int igb_tx_map(struct igb_ring *tx_ring,
 	igb_maybe_stop_tx(tx_ring, DESC_NEEDED);
 
 	if (netif_xmit_stopped(txring_txq(tx_ring)) || !skb->xmit_more) {
-		writel(i, tx_ring->tail);
+		writel_relaxed(i, tx_ring->tail);
 
 		/* we need this if more than one processor can write to our tail
 		 * at a time, it synchronizes IO on IA64/Altix systems

@@ -8073,12 +8069,6 @@ void igb_alloc_rx_buffers(struct igb_ring *rx_ring, u16 cleaned_count)
 		/* update next to alloc since we have filled the ring */
 		rx_ring->next_to_alloc = i;
 
-		/* Force memory writes to complete before letting h/w
-		 * know there are new descriptors to fetch.  (Only
-		 * applicable for weak-ordered memory model archs,
-		 * such as IA-64).
-		 */
-		wmb();
 		writel(i, rx_ring->tail);
 	}
 }

Signature · 1 line

2.7.4

Sinan Kaya

author

2018-04-02 19:06 UTC #7 lore raw

PATCH v8 5/7 fm10k: Eliminate duplicate barriers on weakly-ordered archs

In reply to Sinan Kaya

memory-barriers.txt has been updated as follows:

"When using writel(), a prior wmb() is not needed to guarantee that the cache coherent memory writes have completed before writing to the MMIO region."

Remove old IA-64 comments in the code along with unneeded wmb() in front of writel().

wmb()
writel_relaxed()
mmio_wb()

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

---
 drivers/net/ethernet/intel/fm10k/fm10k_main.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

drivers/net/ethernet/intel/fm10k/fm10k_main.c +2 ~~-13~~

index df86070..41e3aaa 100644
@@ -172,13 +172,6 @@ void fm10k_alloc_rx_buffers(struct fm10k_ring *rx_ring, u16 cleaned_count)
 		/* update next to alloc since we have filled the ring */
 		rx_ring->next_to_alloc = i;
 
-		/* Force memory writes to complete before letting h/w
-		 * know there are new descriptors to fetch.  (Only
-		 * applicable for weak-ordered memory model archs,
-		 * such as IA-64).
-		 */
-		wmb();
-
 		/* notify hardware of new descriptors */
 		writel(i, rx_ring->tail);
 	}

@@ -1036,11 +1029,7 @@ static void fm10k_tx_map(struct fm10k_ring *tx_ring,
 	/* record SW timestamp if HW timestamp is not available */
 	skb_tx_timestamp(first->skb);
 
-	/* Force memory writes to complete before letting h/w know there
-	 * are new descriptors to fetch.  (Only applicable for weak-ordered
-	 * memory model archs, such as IA-64).
-	 *
-	 * We also need this memory barrier to make certain all of the
+	/* We need this memory barrier to make certain all of the
 	 * status bits have been updated before next_to_watch is written.
 	 */
 	wmb();

@@ -1055,7 +1044,7 @@ static void fm10k_tx_map(struct fm10k_ring *tx_ring,
 
 	/* notify HW of packet */
 	if (netif_xmit_stopped(txring_txq(tx_ring)) || !skb->xmit_more) {
-		writel(i, tx_ring->tail);
+		writel_relaxed(i, tx_ring->tail);
 
 		/* we need this if more than one processor can write to our tail
 		 * at a time, it synchronizes IO on IA64/Altix systems

Signature · 1 line

2.7.4

Sinan Kaya

author

2018-04-02 19:06 UTC #8 lore raw

PATCH v8 6/7 ixgbevf: keep writel() closer to wmb()

In reply to Sinan Kaya

Remove ixgbevf_write_tail() in favor of moving writel() close to wmb().

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

Reviewed-by Alexander Duyck <[email protected]>

---
 drivers/net/ethernet/intel/ixgbevf/ixgbevf.h      | 5 -----
 drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 6 +++---
 2 files changed, 3 insertions(+), 8 deletions(-)

drivers/net/ethernet/intel/ixgbevf/ixgbevf.h +0 -5

index 447ce1d..c75ea1f 100644
@@ -312,11 +312,6 @@ static inline u16 ixgbevf_desc_unused(struct ixgbevf_ring *ring)
 	return ((ntc > ntu) ? 0 : ring->count) + ntc - ntu - 1;
 }
 
-static inline void ixgbevf_write_tail(struct ixgbevf_ring *ring, u32 value)
-{
-	writel(value, ring->tail);
-}
-
 #define IXGBEVF_RX_DESC(R, i)	\
 	(&(((union ixgbe_adv_rx_desc *)((R)->desc))[i]))
 #define IXGBEVF_TX_DESC(R, i)	\

drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c +3 -3

index e3d04f2..757dac6 100644
@@ -725,7 +725,7 @@ static void ixgbevf_alloc_rx_buffers(struct ixgbevf_ring *rx_ring,
 		 * such as IA-64).
 		 */
 		wmb();
-		ixgbevf_write_tail(rx_ring, i);
+		writel(i, rx_ring->tail);
 	}
 }

@@ -1232,7 +1232,7 @@ static int ixgbevf_clean_rx_irq(struct ixgbevf_q_vector *q_vector,
 		 * know there are new descriptors to fetch.
 		 */
 		wmb();
-		ixgbevf_write_tail(xdp_ring, xdp_ring->next_to_use);
+		writel(xdp_ring->next_to_use, xdp_ring->tail);
 	}
 
 	u64_stats_update_begin(&rx_ring->syncp);

@@ -4004,7 +4004,7 @@ static void ixgbevf_tx_map(struct ixgbevf_ring *tx_ring,
 	tx_ring->next_to_use = i;
 
 	/* notify HW of packet */
-	ixgbevf_write_tail(tx_ring, i);
+	writel(i, tx_ring->tail);
 
 	return;
 dma_error:

Signature · 1 line

2.7.4

Bowers

2018-04-03 18:51 UTC #9 lore raw

PATCH v8 6/7 ixgbevf: keep writel() closer to wmb()

Quoted 12 lines

-----Original Message-----
From: Intel-wired-lan [mailto:intel-wired-lan-bounces at osuosl.org] On Behalf Of Sinan Kaya
Sent: Monday, April 2, 2018 12:06 PM
To: Kirsher, Jeffrey T <[email protected]>
Cc: sulrich at codeaurora.org; netdev at vger.kernel.org; timur at codeaurora.org; linux-kernel at vger.kernel.org; Sinan Kaya <[email protected]>; intel-wired-lan at lists.osuosl.org; linux-arm- msm at vger.kernel.org; linux-arm-kernel at lists.infradead.org
Subject: [Intel-wired-lan] [PATCH v8 6/7] ixgbevf: keep writel() closer to wmb()

Quoted 8 lines

Remove ixgbevf_write_tail() in favor of moving writel() close to wmb().
Signed-off-by Sinan Kaya <[email protected]>
Reviewed-by Alexander Duyck <[email protected]>
---
 drivers/net/ethernet/intel/ixgbevf/ixgbevf.h      | 5 -----
 drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 6 +++---
 2 files changed, 3 insertions(+), 8 deletions(-)

Tested-by Andrew Bowers <[email protected]>

Sinan Kaya

author

2018-04-02 19:06 UTC #10 lore raw

PATCH v8 7/7 ixgbevf: eliminate duplicate barriers on weakly-ordered archs

In reply to Sinan Kaya

memory-barriers.txt has been updated as follows:

"When using writel(), a prior wmb() is not needed to guarantee that the cache coherent memory writes have completed before writing to the MMIO region."

Remove old IA-64 comments in the code along with unneeded wmb() in front of writel().

wmb()
writel_relaxed()
mmio_wb()

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

---
 drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 23 +++++++----------------
 1 file changed, 7 insertions(+), 16 deletions(-)

drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c +7 ~~-16~~

index 757dac6..29b71a7 100644
@@ -719,12 +719,6 @@ static void ixgbevf_alloc_rx_buffers(struct ixgbevf_ring *rx_ring,
 		/* update next to alloc since we have filled the ring */
 		rx_ring->next_to_alloc = i;
 
-		/* Force memory writes to complete before letting h/w
-		 * know there are new descriptors to fetch.  (Only
-		 * applicable for weak-ordered memory model archs,
-		 * such as IA-64).
-		 */
-		wmb();
 		writel(i, rx_ring->tail);
 	}
 }

@@ -1228,10 +1222,6 @@ static int ixgbevf_clean_rx_irq(struct ixgbevf_q_vector *q_vector,
 		struct ixgbevf_ring *xdp_ring =
 			adapter->xdp_ring[rx_ring->queue_index];
 
-		/* Force memory writes to complete before letting h/w
-		 * know there are new descriptors to fetch.
-		 */
-		wmb();
 		writel(xdp_ring->next_to_use, xdp_ring->tail);
 	}

@@ -3985,11 +3975,7 @@ static void ixgbevf_tx_map(struct ixgbevf_ring *tx_ring,
 	/* set the timestamp */
 	first->time_stamp = jiffies;
 
-	/* Force memory writes to complete before letting h/w know there
-	 * are new descriptors to fetch.  (Only applicable for weak-ordered
-	 * memory model archs, such as IA-64).
-	 *
-	 * We also need this memory barrier (wmb) to make certain all of the
+	/* We also need this memory barrier (wmb) to make certain all of the
 	 * status bits have been updated before next_to_watch is written.
 	 */
 	wmb();

@@ -4004,7 +3990,12 @@ static void ixgbevf_tx_map(struct ixgbevf_ring *tx_ring,
 	tx_ring->next_to_use = i;
 
 	/* notify HW of packet */
-	writel(i, tx_ring->tail);
+	writel_relaxed(i, tx_ring->tail);
+
+	/* We need this if more than one processor can write to our tail
+	 * at a time, it synchronizes IO on IA64/Altix systems
+	 */
+	mmiowb();
 
 	return;
 dma_error:

Signature · 1 line

2.7.4

Bowers

2018-04-03 18:51 UTC #11 lore raw

PATCH v8 7/7 ixgbevf: eliminate duplicate barriers on weakly-ordered archs

Quoted 30 lines

-----Original Message-----
From: Intel-wired-lan [mailto:intel-wired-lan-bounces at osuosl.org] On Behalf Of Sinan Kaya
Sent: Monday, April 2, 2018 12:07 PM
To: Kirsher, Jeffrey T <[email protected]>
Cc: sulrich at codeaurora.org; netdev at vger.kernel.org;
timur at codeaurora.org; linux-kernel at vger.kernel.org; Sinan Kaya <[email protected]>; intel-wired-lan at lists.osuosl.org; linux-arm- msm at vger.kernel.org; linux-arm-kernel at lists.infradead.org
Subject: [Intel-wired-lan] [PATCH v8 7/7] ixgbevf: eliminate duplicate barriers on weakly-ordered archs
memory-barriers.txt has been updated as follows:
"When using writel(), a prior wmb() is not needed to guarantee that the cache coherent memory writes have completed before writing to the MMIO region."
Remove old IA-64 comments in the code along with unneeded wmb() in front of writel().
There are places in the code where wmb() has been used as a double barrier for CPU and IO in place of smp_wmb() and wmb() as an optimization. For such places, keep the wmb() but replace the following writel() with writel_relaxed() to have a sequence as
wmb()
writel_relaxed()
mmio_wb()

Quoted 5 lines

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

---
 drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 23 +++++++-------------
---
 1 file changed, 7 insertions(+), 16 deletions(-)

Tested-by Andrew Bowers <[email protected]>

Sinan Kaya

author

2018-04-03 02:59 UTC #12 lore raw

In reply to Sinan Kaya

Alex,

Quoted 30 lines · Sinan Kaya

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

Quoted 8 lines

* API clarification with Linus and several architecture folks regarding writel()
https://www.mail-archive.com/netdev at vger.kernel.org/msg225806.html
* removed wmb() in front of writel() at several places.
* remove old IA64 comments regarding ordering.

What do you think about this version? Did I miss any SMP barriers?

Quoted 9 lines

Sinan Kaya (7):
i40e/i40evf: Eliminate duplicate barriers on weakly-ordered archs
ixgbe: eliminate duplicate barriers on weakly-ordered archs
igbvf: eliminate duplicate barriers on weakly-ordered archs
igb: eliminate duplicate barriers on weakly-ordered archs
fm10k: Eliminate duplicate barriers on weakly-ordered archs
ixgbevf: keep writel() closer to wmb()
ixgbevf: eliminate duplicate barriers on weakly-ordered archs

Quoted 10 lines

 drivers/net/ethernet/intel/fm10k/fm10k_main.c     | 15 ++-----------
 drivers/net/ethernet/intel/i40e/i40e_txrx.c       | 22 ++++++++----------
 drivers/net/ethernet/intel/i40evf/i40e_txrx.c     |  8 +------
 drivers/net/ethernet/intel/igb/igb_main.c         | 14 ++----------
 drivers/net/ethernet/intel/igbvf/netdev.c         | 16 +++++---------
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c     | 23 ++-----------------
 drivers/net/ethernet/intel/ixgbevf/ixgbevf.h      |  5 -----
 drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 27 ++++++++---------------
 8 files changed, 30 insertions(+), 100 deletions(-)

Sinan

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.

Alexander Duyck

2018-04-03 17:47 UTC #13 lore raw

Quoted 36 lines · Sinan Kaya

Alex,
Quoted 33 lines · Sinan Kaya
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.
Change since 7:
* API clarification with Linus and several architecture folks regarding writel()

Quoted 7 lines

Quoted 5 lines
https://www.mail-archive.com/netdev at vger.kernel.org/msg225806.html
* removed wmb() in front of writel() at several places.
* remove old IA64 comments regarding ordering.
What do you think about this version? Did I miss any SMP barriers?

I would say we should probably just drop the whole set and start over. If we don't need the wmb() we are going to need to go through and clean up all of these paths and consider the barriers when updating the layout of the code.

For example I have been thinking about it and in the case of x86 we are probably better off not bothering with the wmb() and writel_relaxed() and instead switch over to the smp_wmb() and writel() since in the case of a strongly ordered system like x86 or sparc this ends up translating out to a couple of compile barriers.

I will also need time to reevaluate the Rx paths since dropping the wmb() may have other effects which I need to verify.

Thanks.

- Alex

Sinan Kaya

author

2018-04-03 17:50 UTC #14 lore raw

Quoted 50 lines · Alexander Duyck

Quoted 36 lines · Sinan Kaya
Alex,
Quoted 33 lines · Sinan Kaya
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.
Change since 7:
* API clarification with Linus and several architecture folks regarding writel()
Quoted 7 lines
Quoted 5 lines
https://www.mail-archive.com/netdev at vger.kernel.org/msg225806.html
* removed wmb() in front of writel() at several places.
* remove old IA64 comments regarding ordering.
What do you think about this version? Did I miss any SMP barriers?
I would say we should probably just drop the whole set and start over. If we don't need the wmb() we are going to need to go through and clean up all of these paths and consider the barriers when updating the layout of the code.

Quoted 8 lines

For example I have been thinking about it and in the case of x86 we are probably better off not bothering with the wmb() and writel_relaxed() and instead switch over to the smp_wmb() and writel() since in the case of a strongly ordered system like x86 or sparc this ends up translating out to a couple of compile barriers.
I will also need time to reevaluate the Rx paths since dropping the wmb() may have other effects which I need to verify.

Sounds good, I'll let you work on it.

@Jeff Kirsher: can you drop this version from your development branch until Alex posts the next version?

Quoted 5 lines

Thanks.
- Alex

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.

Jeff Kirsher

2018-04-03 18:25 UTC #15 lore raw

Quoted 17 lines · Sinan Kaya

Quoted 17 lines
Quoted 2 lines
What do you think about this version? Did I miss any SMP barriers?
I would say we should probably just drop the whole set and start over.
If we don't need the wmb() we are going to need to go through and clean up all of these paths and consider the barriers when updating the layout of the code.
For example I have been thinking about it and in the case of x86 we are probably better off not bothering with the wmb() and writel_relaxed() and instead switch over to the smp_wmb() and writel()
since in the case of a strongly ordered system like x86 or sparc this
ends up translating out to a couple of compile barriers.

Quoted 8 lines

Quoted 2 lines
I will also need time to reevaluate the Rx paths since dropping the wmb() may have other effects which I need to verify.
Sounds good, I'll let you work on it.
@Jeff Kirsher: can you drop this version from your development branch until
Alex posts the next version?

Already on it, I will work with Alex on any possible future versions of these patches.