netdev: intel: Eliminate duplicate barriers on weakly-ordered archs
Subsystems (4)
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
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
@@ -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
@@ -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
Bowers
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 archsCode includes
wmb()followed bywritel().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()towritel_relaxed().
Quoted 6 lines
Signed-off-by Sinan Kaya <[email protected]>Reviewed-by Alexander Duyck <[email protected]>--- 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(-)
PATCH v4 2/7 ixgbe: eliminate duplicate barriers on weakly-ordered archs
In reply to Sinan KayaCode includes wmb() followed by writel() in multiple places. writel() already has a barrier on some architectures like arm64.
This ends up CPU observing two barriers back to back before executing the register write.
Since code already has an explicit barrier call, changing writel() to writel_relaxed().
--- drivers/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
@@ -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
Bowers
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 archsCode includes
wmb()followed bywritel()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()towritel_relaxed().Signed-off-by Sinan Kaya <[email protected]>Reviewed-by Alexander Duyck <[email protected]>--- drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
PATCH v4 3/7 igbvf: eliminate duplicate barriers on weakly-ordered archs
In reply to Sinan KayaCode includes wmb() followed by writel(). writel() already has a barrier on some architectures like arm64.
This ends up CPU observing two barriers back to back before executing the register write.
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
@@ -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
PATCH v4 4/7 igb: eliminate duplicate barriers on weakly-ordered archs
In reply to Sinan KayaCode includes wmb() followed by writel(). writel() already has a barrier on some architectures like arm64.
This ends up CPU observing two barriers back to back before executing the register write.
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
@@ -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
PATCH v4 5/7 ixgbevf: keep writel() closer to wmb()
In reply to Sinan KayaRemove 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
@@ -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
@@ -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
Re: [PATCH REPOST v4 5/7] ixgbevf: keep writel() closer to wmb()
Quoted 9 lines · Sinan Kaya
Remove
ixgbevf_write_tail()in favor of movingwritel()close towmb().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 | 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.
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 movingwritel()close towmb().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 | 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.
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 movingwritel()close towmb().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 | 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
David Miller
maintainerRe: [PATCH REPOST v4 5/7] ixgbevf: keep writel() closer to wmb()
In reply to Jeff KirsherFrom: 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 movingwritel()close towmb().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 | 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.
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 -0700Quoted 11 lines · Sinan Kaya
Quoted 9 lines
Remove
ixgbevf_write_tail()in favor of movingwritel()close towmb().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 | 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.
PATCH v4 6/7 ixgbevf: eliminate duplicate barriers on weakly-ordered archs
In reply to Sinan KayaCode includes wmb() followed by writel() in multiple places. writel() already has a barrier on some architectures like arm64.
This ends up CPU observing two barriers back to back before executing the register write.
Since code already has an explicit barrier call, changing writel() to writel_relaxed().
--- drivers/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
@@ -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
PATCH v4 7/7 fm10k: Eliminate duplicate barriers on weakly-ordered archs
In reply to Sinan KayaCode includes wmb() followed by writel(). writel() already has a barrier on some architectures like arm64.
This ends up CPU observing two barriers back to back before executing the register write.
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
@@ -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