[Intel-wired-lan] [PATCH v7 1/7] i40e/i40evf: Eliminate duplicate barriers on weakly-ordered archs

[Intel-wired-lan] [PATCH v7 1/7] i40e/i40evf: Eliminate duplicate barriers on weakly-ordered archs

[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 <okaya@codeaurora.org>
---
 drivers/net/ethernet/intel/i40e/i40e_txrx.c   | 14 ++++++++++----
 drivers/net/ethernet/intel/i40evf/i40e_txrx.c |  4 ++--
 2 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
index c6972bd..d970350 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
@@ -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:
@@ -1529,7 +1535,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);
 }
 
 /**
@@ -2412,7 +2418,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;
@@ -3437,7 +3443,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
diff --git a/drivers/net/ethernet/intel/i40evf/i40e_txrx.c b/drivers/net/ethernet/intel/i40evf/i40e_txrx.c
index 1ae112f..5db5c50c 100644
--- a/drivers/net/ethernet/intel/i40evf/i40e_txrx.c
+++ b/drivers/net/ethernet/intel/i40evf/i40e_txrx.c
@@ -810,7 +810,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);
 }
 
 /**
@@ -2379,7 +2379,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
-- 
2.7.4

[Intel-wired-lan] [PATCH v7 2/7] ixgbe: eliminate duplicate barriers on weakly-ordered archs

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

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index e3b32ea..fb80edb 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -1701,7 +1701,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);
 	}
 }
 
@@ -2470,7 +2470,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();
 	}
@@ -8101,7 +8101,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
@@ -10038,7 +10038,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;
 }
-- 
2.7.4

[Intel-wired-lan] [PATCH v7 3/7] igbvf: eliminate duplicate barriers on weakly-ordered archs

[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 <okaya@codeaurora.org>
---
 drivers/net/ethernet/intel/igbvf/netdev.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/igbvf/netdev.c b/drivers/net/ethernet/intel/igbvf/netdev.c
index fa07876..df2283b 100644
--- a/drivers/net/ethernet/intel/igbvf/netdev.c
+++ b/drivers/net/ethernet/intel/igbvf/netdev.c
@@ -252,7 +252,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);
 	}
 }
 
@@ -2298,7 +2298,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
 	 */
-- 
2.7.4

[Intel-wired-lan] [PATCH v7 4/7] igb: eliminate duplicate barriers on weakly-ordered archs

[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 <okaya@codeaurora.org>
---
 drivers/net/ethernet/intel/igb/igb_main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 3d4ff3c..c501378 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -5674,7 +5674,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
@@ -8079,7 +8079,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);
 	}
 }
 
-- 
2.7.4

[Intel-wired-lan] [PATCH v7 5/7] fm10k: Eliminate duplicate barriers on weakly-ordered archs

[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 <okaya@codeaurora.org>
---
 drivers/net/ethernet/intel/fm10k/fm10k_main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_main.c b/drivers/net/ethernet/intel/fm10k/fm10k_main.c
index 409554d..bb94f04 100644
--- a/drivers/net/ethernet/intel/fm10k/fm10k_main.c
+++ b/drivers/net/ethernet/intel/fm10k/fm10k_main.c
@@ -180,7 +180,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);
 	}
 }
 
@@ -1055,7 +1055,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
-- 
2.7.4

[Intel-wired-lan] [PATCH v7 6/7] ixgbevf: keep writel() closer to wmb()

[Sinan Kaya]

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

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 drivers/net/ethernet/intel/ixgbevf/ixgbevf.h      | 5 -----
 drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 6 +++---
 2 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf.h b/drivers/net/ethernet/intel/ixgbevf/ixgbevf.h
index f712646..f97091d 100644
--- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf.h
+++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf.h
@@ -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)	\
diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
index 3d9033f..815cb1a 100644
--- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
+++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
@@ -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:
-- 
2.7.4

[Intel-wired-lan] [PATCH v7 7/7] ixgbevf: eliminate duplicate barriers on weakly-ordered archs

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

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
index 815cb1a..7277647 100644
--- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
+++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
@@ -725,7 +725,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);
 	}
 }
 
@@ -1232,7 +1232,7 @@ static int ixgbevf_clean_rx_irq(struct ixgbevf_q_vector *q_vector,
 		 * know there are new descriptors to fetch.
 		 */
 		wmb();
-		writel(xdp_ring->next_to_use, xdp_ring->tail);
+		writel_relaxed(xdp_ring->next_to_use, xdp_ring->tail);
 	}
 
 	u64_stats_update_begin(&rx_ring->syncp);
@@ -4004,7 +4004,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:
-- 
2.7.4

[Intel-wired-lan] [PATCH v7 0/7] netdev: intel: Eliminate duplicate barriers on weakly-ordered archs

[Alexander Duyck]

On Fri, Mar 23, 2018 at 11:52 AM, Sinan Kaya <okaya@codeaurora.org> wrote:
> 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.

I looked over the set and they seem good.

Reviewed-by: Alexander Duyck <alexander.h.duyck@intel.com>

>
> Changes since v6:
> clean up between 2..6 and then make your Alex's changes on 1 and 7
>     The mmiowb shouldn't be needed for Rx. Only one CPU will be running
>     NAPI for the queue and we will synchronize this with a full writel
>     anyway when we re-enable the interrupts.
>
> 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     |  4 ++--
>  drivers/net/ethernet/intel/i40e/i40e_txrx.c       | 14 ++++++++++----
>  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 | 11 ++++++++---
>  8 files changed, 30 insertions(+), 24 deletions(-)
>
> --
> 2.7.4
>

[Intel-wired-lan] [PATCH v7 0/7] netdev: intel: Eliminate duplicate barriers on weakly-ordered archs

[Jeff Kirsher]

On Fri, 2018-03-23 at 14:53 -0700, Alexander Duyck wrote:
> On Fri, Mar 23, 2018 at 11:52 AM, Sinan Kaya <okaya@codeaurora.org>
> wrote:
> > 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.
> 
> I looked over the set and they seem good.
> 
> Reviewed-by: Alexander Duyck <alexander.h.duyck@intel.com>

Grrr, patch 1 does not apply cleanly to my next-queue tree (dev-queue
branch).  I will deal with this series in a day or two, after I have dealt
with my driver pull requests.

> > 
> > Changes since v6:
> > clean up between 2..6 and then make your Alex's changes on 1 and 7
> >     The mmiowb shouldn't be needed for Rx. Only one CPU will be running
> >     NAPI for the queue and we will synchronize this with a full writel
> >     anyway when we re-enable the interrupts.
> > 
> > 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     |  4 ++--
> >  drivers/net/ethernet/intel/i40e/i40e_txrx.c       | 14 ++++++++++----
> >  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 | 11 ++++++++---
> >  8 files changed, 30 insertions(+), 24 deletions(-)
> > 
> > --
> > 2.7.4
> > 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20180323/777698b2/attachment.asc>

[Intel-wired-lan] [PATCH v7 0/7] netdev: intel: Eliminate duplicate barriers on weakly-ordered archs

[okaya]

On 2018-03-23 19:58, Jeff Kirsher wrote:
> On Fri, 2018-03-23 at 14:53 -0700, Alexander Duyck wrote:
>> On Fri, Mar 23, 2018 at 11:52 AM, Sinan Kaya <okaya@codeaurora.org>
>> wrote:
>> > 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.
>> 
>> I looked over the set and they seem good.
>> 
>> Reviewed-by: Alexander Duyck <alexander.h.duyck@intel.com>
> 
> Grrr, patch 1 does not apply cleanly to my next-queue tree (dev-queue
> branch).  I will deal with this series in a day or two, after I have 
> dealt
> with my driver pull requests.

Sorry, you will have to replace the ones you took from me.
> 
>> >
>> > Changes since v6:
>> > clean up between 2..6 and then make your Alex's changes on 1 and 7
>> >     The mmiowb shouldn't be needed for Rx. Only one CPU will be running
>> >     NAPI for the queue and we will synchronize this with a full writel
>> >     anyway when we re-enable the interrupts.
>> >
>> > 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     |  4 ++--
>> >  drivers/net/ethernet/intel/i40e/i40e_txrx.c       | 14 ++++++++++----
>> >  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 | 11 ++++++++---
>> >  8 files changed, 30 insertions(+), 24 deletions(-)
>> >
>> > --
>> > 2.7.4
>> >

[Intel-wired-lan] [PATCH v7 0/7] netdev: intel: Eliminate duplicate barriers on weakly-ordered archs

[Sinan Kaya]

Jeff,

On 3/23/2018 10:34 PM, okaya at codeaurora.org wrote:
> On 2018-03-23 19:58, Jeff Kirsher wrote:
>> On Fri, 2018-03-23 at 14:53 -0700, Alexander Duyck wrote:
>>> On Fri, Mar 23, 2018 at 11:52 AM, Sinan Kaya <okaya@codeaurora.org>
>>> wrote:
>>> > 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.
>>>
>>> I looked over the set and they seem good.
>>>
>>> Reviewed-by: Alexander Duyck <alexander.h.duyck@intel.com>
>>
>> Grrr, patch 1 does not apply cleanly to my next-queue tree (dev-queue
>> branch).? I will deal with this series in a day or two, after I have dealt
>> with my driver pull requests.
> 
> Sorry, you will have to replace the ones you took from me.

Double sorry now.

I don't know if you have been following "RFC on writel and writel_relaxed" thread
or not but there are some new developments about wmb() requirement. 

Basically, wmb() should never be used before writel() as writel() seem to
provide coherency and observability guarantee.

wmb()+writel_relaxed() is slower on some architectures than plain writel()

I'll have to rework these patches to have writel() only. 

Are you able to drop the applied ones so that I can post V8 or is it too late?


Sinan

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

[Intel-wired-lan] [PATCH v7 0/7] netdev: intel: Eliminate duplicate barriers on weakly-ordered archs

[Lino Sanfilippo]

Hi,

> Double sorry now.
> 
> I don't know if you have been following "RFC on writel and writel_relaxed" thread
> or not but there are some new developments about wmb() requirement. 

Just out of interest: Where can this thread be found?

> 
> Basically, wmb() should never be used before writel() as writel() seem to
> provide coherency and observability guarantee.
> 

AFAIU memory-barriers.txt writel() only guarantees correct order of accesses to
IO-memory not RAM vs. IO-memory (this may be the case for some architectures 
where the writel() implementation contains a wmb() but not for all).
For the RAM vs. IO-memory case at least a a wmb()/rmb() has to be used. 
Is this not correct? 

Regards,
Lino

[Intel-wired-lan] [PATCH v7 0/7] netdev: intel: Eliminate duplicate barriers on weakly-ordered archs

[Sinan Kaya]

On 3/27/2018 10:04 AM, Lino Sanfilippo wrote:
> Hi,
> 
>> Double sorry now.
>>
>> I don't know if you have been following "RFC on writel and writel_relaxed" thread
>> or not but there are some new developments about wmb() requirement. 
> 
> Just out of interest: Where can this thread be found?

https://www.spinics.net/lists/linux-rdma/msg62570.html

https://patchwork.kernel.org/patch/10309913/


> 
>>
>> Basically, wmb() should never be used before writel() as writel() seem to
>> provide coherency and observability guarantee.
>>
> 
> AFAIU memory-barriers.txt writel() only guarantees correct order of accesses to
> IO-memory not RAM vs. IO-memory (this may be the case for some architectures 
> where the writel() implementation contains a wmb() but not for all).
> For the RAM vs. IO-memory case at least a a wmb()/rmb() has to be used. 
> Is this not correct? 

We are being told that if you use writel(), then you don't need a wmb() on
all architectures.

Jason is seeking behavior clarification for write combined buffers.

> 
> Regards,
> Lino
> 


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

[Intel-wired-lan] [PATCH v7 0/7] netdev: intel: Eliminate duplicate barriers on weakly-ordered archs

[Lino Sanfilippo]

>
> On 3/27/2018 10:04 AM, Lino Sanfilippo wrote:
> > Hi,
> > 
> >> Double sorry now.
> >>
> >> I don't know if you have been following "RFC on writel and writel_relaxed" thread
> >> or not but there are some new developments about wmb() requirement. 
> > 
> > Just out of interest: Where can this thread be found?
> 
> https://www.spinics.net/lists/linux-rdma/msg62570.html
> 
> https://patchwork.kernel.org/patch/10309913/
> 
> 
> > 
> >>
> >> Basically, wmb() should never be used before writel() as writel() seem to
> >> provide coherency and observability guarantee.
> >>
> > 
> > AFAIU memory-barriers.txt writel() only guarantees correct order of accesses to
> > IO-memory not RAM vs. IO-memory (this may be the case for some architectures 
> > where the writel() implementation contains a wmb() but not for all).
> > For the RAM vs. IO-memory case at least a a wmb()/rmb() has to be used. 
> > Is this not correct? 
> 
> We are being told that if you use writel(), then you don't need a wmb() on
> all architectures.
> 
> Jason is seeking behavior clarification for write combined buffers.
> 

Interesting, thanks for the information!

Lino

[Intel-wired-lan] [PATCH v7 0/7] netdev: intel: Eliminate duplicate barriers on weakly-ordered archs

[Alexander Duyck]

On Tue, Mar 27, 2018 at 7:23 AM, Sinan Kaya <okaya@codeaurora.org> wrote:
> On 3/27/2018 10:04 AM, Lino Sanfilippo wrote:
>> Hi,
>>
>>> Double sorry now.
>>>
>>> I don't know if you have been following "RFC on writel and writel_relaxed" thread
>>> or not but there are some new developments about wmb() requirement.
>>
>> Just out of interest: Where can this thread be found?
>
> https://www.spinics.net/lists/linux-rdma/msg62570.html
>
> https://patchwork.kernel.org/patch/10309913/
>
>
>>
>>>
>>> Basically, wmb() should never be used before writel() as writel() seem to
>>> provide coherency and observability guarantee.
>>>
>>
>> AFAIU memory-barriers.txt writel() only guarantees correct order of accesses to
>> IO-memory not RAM vs. IO-memory (this may be the case for some architectures
>> where the writel() implementation contains a wmb() but not for all).
>> For the RAM vs. IO-memory case at least a a wmb()/rmb() has to be used.
>> Is this not correct?
>
> We are being told that if you use writel(), then you don't need a wmb() on
> all architectures.

I'm not sure who told you that but that is incorrect, at least for
x86. If you attempt to use writel() without the wmb() we will have to
NAK the patches. We will accept the wmb() with writel_releaxed() since
that solves things for ARM.

> Jason is seeking behavior clarification for write combined buffers.

Don't bother. I can tell you right now that for x86 you have to have a
wmb() before the writel().

Based on the comment in
(https://www.spinics.net/lists/linux-rdma/msg62666.html):
    Replacing wmb() + writel() with wmb() + writel_relaxed() will work on
    PPC, it will just not give you a benefit today.

I say the patch set stays. This gives benefit on ARM, and has no
effect on x86 and PowerPC. If you want to look at trying to optimize
things further on PowerPC and such then go for it in terms of trying
to implement the writel_relaxed(). Otherwise I say we call the ARM
goodness a win and don't get ourselves too wrapped up in trying to fix
this for all architectures.

Thanks.

- Alex

[Intel-wired-lan] [PATCH v7 0/7] netdev: intel: Eliminate duplicate barriers on weakly-ordered archs

[Sinan Kaya]

On 3/27/2018 10:38 AM, Alexander Duyck wrote:
>> We are being told that if you use writel(), then you don't need a wmb() on
>> all architectures.
> I'm not sure who told you that but that is incorrect, at least for
> x86. If you attempt to use writel() without the wmb() we will have to
> NAK the patches. We will accept the wmb() with writel_releaxed() since
> that solves things for ARM.
> 

I added netdev and you to the RFC on writel and writel_relaxed list. 
Feel free to raise your concerns.

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

[Intel-wired-lan] [PATCH v7 0/7] netdev: intel: Eliminate duplicate barriers on weakly-ordered archs

[Jeff Kirsher]

On Tue, 2018-03-27 at 08:42 -0400, Sinan Kaya wrote:
> On 3/23/2018 10:34 PM, okaya at codeaurora.org wrote:
> > On 2018-03-23 19:58, Jeff Kirsher wrote:
> > > On Fri, 2018-03-23 at 14:53 -0700, Alexander Duyck wrote:
> > > > On Fri, Mar 23, 2018 at 11:52 AM, Sinan Kaya <okaya@codeaurora.
> > > > org>
> > > > wrote:
> > > > > 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.
> > > > 
> > > > I looked over the set and they seem good.
> > > > 
> > > > Reviewed-by: Alexander Duyck <alexander.h.duyck@intel.com>
> > > 
> > > Grrr, patch 1 does not apply cleanly to my next-queue tree (dev-
> > > queue
> > > branch).  I will deal with this series in a day or two, after I
> > > have dealt
> > > with my driver pull requests.
> > 
> > Sorry, you will have to replace the ones you took from me.
> 
> Double sorry now.
> 
> I don't know if you have been following "RFC on writel and
> writel_relaxed" thread
> or not but there are some new developments about wmb() requirement. 
> 
> Basically, wmb() should never be used before writel() as writel()
> seem to
> provide coherency and observability guarantee.
> 
> wmb()+writel_relaxed() is slower on some architectures than plain
> writel()
> 
> I'll have to rework these patches to have writel() only. 
> 
> Are you able to drop the applied ones so that I can post V8 or is it
> too late?

Currently I do not have any of your patches applied to my next-queue
tree (dev-queue branch).  So feel free to do any revisions you need to
do and to re-submit to intel-wired-lan at lists.osuosl.org (IWL) mailing
list.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20180327/a20aa26b/attachment.asc>

[Intel-wired-lan] [PATCH v7 0/7] netdev: intel: Eliminate duplicate barriers on weakly-ordered archs

[Sinan Kaya]

On 3/27/2018 12:54 PM, Jeff Kirsher wrote:
> On Tue, 2018-03-27 at 08:42 -0400, Sinan Kaya wrote:
>> On 3/23/2018 10:34 PM, okaya at codeaurora.org wrote:
>>> On 2018-03-23 19:58, Jeff Kirsher wrote:
>>>> On Fri, 2018-03-23 at 14:53 -0700, Alexander Duyck wrote:
>>>>> On Fri, Mar 23, 2018 at 11:52 AM, Sinan Kaya <okaya@codeaurora.
>>>>> org>
>>>>> wrote:
>>>>>> 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.
>>>>>
>>>>> I looked over the set and they seem good.
>>>>>
>>>>> Reviewed-by: Alexander Duyck <alexander.h.duyck@intel.com>
>>>>
>>>> Grrr, patch 1 does not apply cleanly to my next-queue tree (dev-
>>>> queue
>>>> branch).  I will deal with this series in a day or two, after I
>>>> have dealt
>>>> with my driver pull requests.
>>>
>>> Sorry, you will have to replace the ones you took from me.
>>
>> Double sorry now.
>>
>> I don't know if you have been following "RFC on writel and
>> writel_relaxed" thread
>> or not but there are some new developments about wmb() requirement. 
>>
>> Basically, wmb() should never be used before writel() as writel()
>> seem to
>> provide coherency and observability guarantee.
>>
>> wmb()+writel_relaxed() is slower on some architectures than plain
>> writel()
>>
>> I'll have to rework these patches to have writel() only. 
>>
>> Are you able to drop the applied ones so that I can post V8 or is it
>> too late?
> 
> Currently I do not have any of your patches applied to my next-queue
> tree (dev-queue branch).  So feel free to do any revisions you need to
> do and to re-submit to intel-wired-lan at lists.osuosl.org (IWL) mailing
> list.
> 

Thanks, Good to know. 

I'm waiting for the discussion to settle. I'll update as soon as I get
a clear direction.

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