Skip to contentSkip to conversation

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

PATCH Superseded Sinan Kaya 5 participants 15 messages Mar 21, 2018 – Mar 22, 2018

Subsystems (4)

v4 of 8 Superseded Latest is v8 →

lore
Share & tools
lore

Sinan Kaya

author
#0 lore raw

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

repost:
- split into intel specific patches

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

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
ixgbevf: keep writel() closer to wmb()
ixgbevf: eliminate duplicate barriers on weakly-ordered archs
fm10k: Eliminate duplicate barriers on weakly-ordered archs

 drivers/net/ethernet/intel/fm10k/fm10k_main.c     | 4 ++--
 drivers/net/ethernet/intel/i40e/i40e_txrx.c       | 8 ++++----
 drivers/net/ethernet/intel/i40evf/i40e_txrx.c     | 4 ++--
 drivers/net/ethernet/intel/igb/igb_main.c         | 4 ++--
 drivers/net/ethernet/intel/igbvf/netdev.c         | 4 ++--
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c     | 8 ++++----
 drivers/net/ethernet/intel/ixgbevf/ixgbevf.h      | 5 -----
 drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 4 ++--
 8 files changed, 18 insertions(+), 23 deletions(-)
Signature · 1 line
2.7.4
author
#1 lore raw

PATCH v4 1/7 i40e/i40evf: 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/net/ethernet/intel/i40e/i40e_txrx.c   | 8 ++++----
 drivers/net/ethernet/intel/i40evf/i40e_txrx.c | 4 ++--
 2 files changed, 6 insertions(+), 6 deletions(-)
drivers/net/ethernet/intel/i40e/i40e_txrx.c +4 -4
index e554aa6cf..9455869 100644
@@ -185,7 +185,7 @@ 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);
 	return 0;
 
 dma_fail:
@@ -1375,7 +1375,7 @@ static inline void i40e_release_rx_desc(struct i40e_ring *rx_ring, u32 val)
 	 * such as IA-64).
 	 */
 	wmb();
-	writel(val, rx_ring->tail);
+	writel_relaxed(val, rx_ring->tail);
 }
 
 /**
@@ -2258,7 +2258,7 @@ static int i40e_clean_rx_irq(struct i40e_ring *rx_ring, int budget)
 		 */
 		wmb();
 
-		writel(xdp_ring->next_to_use, xdp_ring->tail);
+		writel_relaxed(xdp_ring->next_to_use, xdp_ring->tail);
 	}
 
 	rx_ring->skb = skb;
@@ -3286,7 +3286,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 +2 -2
index 357d605..56eea20 100644
@@ -667,7 +667,7 @@ static inline void i40e_release_rx_desc(struct i40e_ring *rx_ring, u32 val)
 	 * such as IA-64).
 	 */
 	wmb();
-	writel(val, rx_ring->tail);
+	writel_relaxed(val, rx_ring->tail);
 }
 
 /**
@@ -2243,7 +2243,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
#2 lore raw

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

Quoted 21 lines

-----Original Message-----
From: Intel-wired-lan [mailto:intel-wired-lan-bounces at osuosl.org] On Behalf Of Sinan Kaya
Sent: Wednesday, March 21, 2018 11:56 AM
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 REPOST v4 1/7] i40e/i40evf: 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().

Quoted 6 lines
---
 drivers/net/ethernet/intel/i40e/i40e_txrx.c   | 8 ++++----
 drivers/net/ethernet/intel/i40evf/i40e_txrx.c | 4 ++--
 2 files changed, 6 insertions(+), 6 deletions(-)
author
#3 lore raw

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

In reply to 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().

---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +4 -4
index 0da5aa2..58ed70f 100644
@@ -1692,7 +1692,7 @@ void ixgbe_alloc_rx_buffers(struct ixgbe_ring *rx_ring, u16 cleaned_count)
 		 * such as IA-64).
 		 */
 		wmb();
-		writel(i, rx_ring->tail);
+		writel_relaxed(i, rx_ring->tail);
 	}
 }
 
@@ -2453,7 +2453,7 @@ static int ixgbe_clean_rx_irq(struct ixgbe_q_vector *q_vector,
 		 * know there are new descriptors to fetch.
 		 */
 		wmb();
-		writel(ring->next_to_use, ring->tail);
+		writel_relaxed(ring->next_to_use, ring->tail);
 
 		xdp_do_flush_map();
 	}
@@ -8078,7 +8078,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
@@ -10014,7 +10014,7 @@ static void ixgbe_xdp_flush(struct net_device *dev)
 	 * are new descriptors to fetch.
 	 */
 	wmb();
-	writel(ring->next_to_use, ring->tail);
+	writel_relaxed(ring->next_to_use, ring->tail);
 
 	return;
 }
Signature · 1 line
2.7.4
#4 lore raw

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

Quoted 18 lines

-----Original Message-----
From: Intel-wired-lan [mailto:intel-wired-lan-bounces at osuosl.org] On Behalf Of Sinan Kaya
Sent: Wednesday, March 21, 2018 11:56 AM
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 REPOST v4 2/7] ixgbe: 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.

Quoted 8 lines

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

---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
author
#5 lore raw

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

In reply to 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().

---
 drivers/net/ethernet/intel/igbvf/netdev.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/net/ethernet/intel/igbvf/netdev.c +2 -2
index 4214c15..edb1c34 100644
@@ -251,7 +251,7 @@ static void igbvf_alloc_rx_buffers(struct igbvf_ring *rx_ring,
 		 * such as IA-64).
 		*/
 		wmb();
-		writel(i, adapter->hw.hw_addr + rx_ring->tail);
+		writel_relaxed(i, adapter->hw.hw_addr + rx_ring->tail);
 	}
 }
 
@@ -2297,7 +2297,7 @@ static inline void igbvf_tx_queue_adv(struct igbvf_adapter *adapter,
 
 	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
author
#6 lore raw

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

In reply to 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().

---
 drivers/net/ethernet/intel/igb/igb_main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/net/ethernet/intel/igb/igb_main.c +2 -2
index b88fae7..82aea92 100644
@@ -5671,7 +5671,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
@@ -8072,7 +8072,7 @@ void igb_alloc_rx_buffers(struct igb_ring *rx_ring, u16 cleaned_count)
 		 * such as IA-64).
 		 */
 		wmb();
-		writel(i, rx_ring->tail);
+		writel_relaxed(i, rx_ring->tail);
 	}
 }
 
Signature · 1 line
2.7.4
author
#7 lore raw

PATCH v4 5/7 ixgbevf: keep writel() closer to wmb()

In reply to Sinan Kaya

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

---
 drivers/net/ethernet/intel/ixgbevf/ixgbevf.h      | 5 -----
 drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 4 ++--
 2 files changed, 2 insertions(+), 7 deletions(-)
drivers/net/ethernet/intel/ixgbevf/ixgbevf.h +0 -5
index f695242..11e893e 100644
@@ -244,11 +244,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 +2 -2
index 9b3d43d..6bf778a 100644
@@ -659,7 +659,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);
 	}
 }
 
@@ -3644,7 +3644,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
#8 lore raw

Re: [PATCH REPOST v4 5/7] ixgbevf: keep writel() closer to wmb()

Quoted 9 lines · Sinan Kaya

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

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

This patch fails to compile because there is a call to ixgbevf_write_tail() which you missed cleaning up.

#9 lore raw

Re: [PATCH REPOST v4 5/7] ixgbevf: keep writel() closer to wmb()

Quoted 11 lines · Jeff Kirsher
Quoted 9 lines · Sinan Kaya

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

---
 drivers/net/ethernet/intel/ixgbevf/ixgbevf.h      | 5 -----
 drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 4 ++--
 2 files changed, 2 insertions(+), 7 deletions(-)
Quoted 2 lines

This patch fails to compile because there is a call to ixgbevf_write_tail() which you missed cleaning up.

Hah, I did a compile test but maybe I missed something. I will get v6 of this patch only and leave the rest of the series as it is.

#10 lore raw

Re: [Intel-wired-lan] [PATCH REPOST v4 5/7] ixgbevf: keep writel() closer to wmb()

Quoted 15 lines · On Wed, Mar 21, 2018 at 2:51 PM, <[email protected]> wrote:
Quoted 14 lines · Jeff Kirsher
Quoted 10 lines · Sinan Kaya

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

---
 drivers/net/ethernet/intel/ixgbevf/ixgbevf.h      | 5 -----
 drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 4 ++--
 2 files changed, 2 insertions(+), 7 deletions(-)
Quoted 6 lines
Quoted 2 lines

This patch fails to compile because there is a call to ixgbevf_write_tail() which you missed cleaning up.

Hah, I did a compile test but maybe I missed something. I will get v6 of this patch only and leave the rest of the series as it is.

Actually you might want to just pull Jeff's tree and rebase before you submit your patches. I suspect the difference is the ixgbevf XDP code that is present in Jeff's tree and not in Dave's. The alternative is to wait for Jeff to push the ixgbevf code and then once Dave has pulled it you could rebase your patches.

Thanks.

- Alex

maintainer
#11 lore raw

Re: [PATCH REPOST v4 5/7] ixgbevf: keep writel() closer to wmb()

In reply to Jeff Kirsher

From: Jeff Kirsher <[email protected]>
Date: Wed, 21 Mar 2018 14:48:08 -0700

Quoted 11 lines · Sinan Kaya
Quoted 9 lines

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

---
 drivers/net/ethernet/intel/ixgbevf/ixgbevf.h      | 5 -----
 drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 4 ++--
 2 files changed, 2 insertions(+), 7 deletions(-)
Quoted 2 lines

This patch fails to compile because there is a call to ixgbevf_write_tail() which you missed cleaning up.

For a change with delicate side effects, it doesn't create much confidence if the code does not even compile.

Sinan, please put more care into the changes you are making.

Thank you.

#12 lore raw

Re: [PATCH REPOST v4 5/7] ixgbevf: keep writel() closer to wmb()

Quoted 14 lines · David Miller

From: Jeff Kirsher <[email protected]>
Date: Wed, 21 Mar 2018 14:48:08 -0700

Quoted 11 lines · Sinan Kaya
Quoted 9 lines

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

---
 drivers/net/ethernet/intel/ixgbevf/ixgbevf.h      | 5 -----
 drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 4 ++--
 2 files changed, 2 insertions(+), 7 deletions(-)
Quoted 7 lines
Quoted 2 lines

This patch fails to compile because there is a call to ixgbevf_write_tail() which you missed cleaning up.

For a change with delicate side effects, it doesn't create much confidence if the code does not even compile.

Sinan, please put more care into the changes you are making.

I think the issue is the tree that code is getting tested has undelivered code as Alex mentioned.

I was using linux-next 4.16 rc4 for testing.

I will rebase to Jeff's tree.

Quoted 2 lines

Thank you.

author
#13 lore raw

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

In reply to 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().

---
 drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c +2 -2
index 6bf778a..774b2a6 100644
@@ -659,7 +659,7 @@ static void ixgbevf_alloc_rx_buffers(struct ixgbevf_ring *rx_ring,
 		 * such as IA-64).
 		 */
 		wmb();
-		writel(i, rx_ring->tail);
+		writel_relaxed(i, rx_ring->tail);
 	}
 }
 
@@ -3644,7 +3644,7 @@ 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);
 
 	return;
 dma_error:
Signature · 1 line
2.7.4
author
#14 lore raw

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

In reply to 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().

---
 drivers/net/ethernet/intel/fm10k/fm10k_main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/net/ethernet/intel/fm10k/fm10k_main.c +2 -2
index 8e12aae..eebef01 100644
@@ -179,7 +179,7 @@ void fm10k_alloc_rx_buffers(struct fm10k_ring *rx_ring, u16 cleaned_count)
 		wmb();
 
 		/* notify hardware of new descriptors */
-		writel(i, rx_ring->tail);
+		writel_relaxed(i, rx_ring->tail);
 	}
 }
 
@@ -1054,7 +1054,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