[Intel-wired-lan] [PATCH 0/3] Stop using dev_kfree_skb_any in NAPI Tx clean-up

[Intel-wired-lan] [PATCH 0/3] Stop using dev_kfree_skb_any in NAPI Tx clean-up

[Alexander Duyck]

This change replaces the call to dev_kfree_skb_any in several NAPI based Tx
clean-up paths with dev_kfree_skb.  Where I made the replacement the
function appeared to always be in softirq context so we can avoid the
unnecessary time spent determining if we are in a hardirq context or not.

---

Alexander Duyck (3):
      igb, ixgbe(vf), i40e(vf), fm10k: Use dev_kfree_skb in Tx cleanup path
      r8169: Use dev_kfree_skb in Tx cleanup path
      bnx2: Use dev_kfree_skb in Tx cleanup path


 drivers/net/ethernet/broadcom/bnx2.c              |    2 +-
 drivers/net/ethernet/intel/fm10k/fm10k_main.c     |    2 +-
 drivers/net/ethernet/intel/i40e/i40e_txrx.c       |    2 +-
 drivers/net/ethernet/intel/i40evf/i40e_txrx.c     |    2 +-
 drivers/net/ethernet/intel/igb/igb_main.c         |    2 +-
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c     |    2 +-
 drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c |    2 +-
 drivers/net/ethernet/realtek/r8169.c              |    2 +-
 8 files changed, 8 insertions(+), 8 deletions(-)

--

[PATCH 0/3] Stop using dev_kfree_skb_any in NAPI Tx clean-up

[Alexander Duyck]

This change replaces the call to dev_kfree_skb_any in several NAPI based Tx
clean-up paths with dev_kfree_skb.  Where I made the replacement the
function appeared to always be in softirq context so we can avoid the
unnecessary time spent determining if we are in a hardirq context or not.

---

Alexander Duyck (3):
      igb, ixgbe(vf), i40e(vf), fm10k: Use dev_kfree_skb in Tx cleanup path
      r8169: Use dev_kfree_skb in Tx cleanup path
      bnx2: Use dev_kfree_skb in Tx cleanup path


 drivers/net/ethernet/broadcom/bnx2.c              |    2 +-
 drivers/net/ethernet/intel/fm10k/fm10k_main.c     |    2 +-
 drivers/net/ethernet/intel/i40e/i40e_txrx.c       |    2 +-
 drivers/net/ethernet/intel/i40evf/i40e_txrx.c     |    2 +-
 drivers/net/ethernet/intel/igb/igb_main.c         |    2 +-
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c     |    2 +-
 drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c |    2 +-
 drivers/net/ethernet/realtek/r8169.c              |    2 +-
 8 files changed, 8 insertions(+), 8 deletions(-)

--

[Intel-wired-lan] [PATCH 1/3] igb, ixgbe(vf), i40e(vf), fm10k: Use dev_kfree_skb in Tx cleanup path

[Alexander Duyck]

This change replaces the use of either dev_kfree_skb_any or
dev_consume_skb_any in the Tx cleanup paths of these drivers with
dev_kfree_skb.  There isn't any need for the "_any" version of these
functions since the NAPI cleanup context is not a hard irq context.

This change allows us to drop one function call from the clean-up path
since dev_kfree_skb_any was just calling into dev_kfree_skb in these paths
anyway.

Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
---
 drivers/net/ethernet/intel/fm10k/fm10k_main.c     |    2 +-
 drivers/net/ethernet/intel/i40e/i40e_txrx.c       |    2 +-
 drivers/net/ethernet/intel/i40evf/i40e_txrx.c     |    2 +-
 drivers/net/ethernet/intel/igb/igb_main.c         |    2 +-
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c     |    2 +-
 drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c |    2 +-
 6 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_main.c b/drivers/net/ethernet/intel/fm10k/fm10k_main.c
index 4eafdad318b4..1e3fe455cdea 100644
--- a/drivers/net/ethernet/intel/fm10k/fm10k_main.c
+++ b/drivers/net/ethernet/intel/fm10k/fm10k_main.c
@@ -1227,7 +1227,7 @@ static bool fm10k_clean_tx_irq(struct fm10k_q_vector *q_vector,
 		total_packets += tx_buffer->gso_segs;
 
 		/* free the skb */
-		dev_consume_skb_any(tx_buffer->skb);
+		dev_kfree_skb(tx_buffer->skb);
 
 		/* unmap skb header data */
 		dma_unmap_single(tx_ring->dev,
diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
index 4bd3a80aba82..52a6e2a609a7 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
@@ -726,7 +726,7 @@ static bool i40e_clean_tx_irq(struct i40e_ring *tx_ring, int budget)
 		total_packets += tx_buf->gso_segs;
 
 		/* free the skb */
-		dev_consume_skb_any(tx_buf->skb);
+		dev_kfree_skb(tx_buf->skb);
 
 		/* unmap skb header data */
 		dma_unmap_single(tx_ring->dev,
diff --git a/drivers/net/ethernet/intel/i40evf/i40e_txrx.c b/drivers/net/ethernet/intel/i40evf/i40e_txrx.c
index b077e02a0cc7..976b93ca996a 100644
--- a/drivers/net/ethernet/intel/i40evf/i40e_txrx.c
+++ b/drivers/net/ethernet/intel/i40evf/i40e_txrx.c
@@ -245,7 +245,7 @@ static bool i40e_clean_tx_irq(struct i40e_ring *tx_ring, int budget)
 		total_packets += tx_buf->gso_segs;
 
 		/* free the skb */
-		dev_kfree_skb_any(tx_buf->skb);
+		dev_kfree_skb(tx_buf->skb);
 
 		/* unmap skb header data */
 		dma_unmap_single(tx_ring->dev,
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index cab50593c315..699902618a6f 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -6425,7 +6425,7 @@ static bool igb_clean_tx_irq(struct igb_q_vector *q_vector)
 		total_packets += tx_buffer->gso_segs;
 
 		/* free the skb */
-		dev_consume_skb_any(tx_buffer->skb);
+		dev_kfree_skb(tx_buffer->skb);
 
 		/* unmap skb header data */
 		dma_unmap_single(tx_ring->dev,
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index d3f4b0ceb3f7..e7586663a2cf 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -1114,7 +1114,7 @@ static bool ixgbe_clean_tx_irq(struct ixgbe_q_vector *q_vector,
 		total_packets += tx_buffer->gso_segs;
 
 		/* free the skb */
-		dev_consume_skb_any(tx_buffer->skb);
+		dev_kfree_skb(tx_buffer->skb);
 
 		/* unmap skb header data */
 		dma_unmap_single(tx_ring->dev,
diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
index cca8936f2fc3..0d5b94be1f0d 100644
--- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
+++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
@@ -326,7 +326,7 @@ static bool ixgbevf_clean_tx_irq(struct ixgbevf_q_vector *q_vector,
 		total_packets += tx_buffer->gso_segs;
 
 		/* free the skb */
-		dev_kfree_skb_any(tx_buffer->skb);
+		dev_kfree_skb(tx_buffer->skb);
 
 		/* unmap skb header data */
 		dma_unmap_single(tx_ring->dev,

[PATCH 1/3] igb, ixgbe(vf), i40e(vf), fm10k: Use dev_kfree_skb in Tx cleanup path

[Alexander Duyck]

This change replaces the use of either dev_kfree_skb_any or
dev_consume_skb_any in the Tx cleanup paths of these drivers with
dev_kfree_skb.  There isn't any need for the "_any" version of these
functions since the NAPI cleanup context is not a hard irq context.

This change allows us to drop one function call from the clean-up path
since dev_kfree_skb_any was just calling into dev_kfree_skb in these paths
anyway.

Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
---
 drivers/net/ethernet/intel/fm10k/fm10k_main.c     |    2 +-
 drivers/net/ethernet/intel/i40e/i40e_txrx.c       |    2 +-
 drivers/net/ethernet/intel/i40evf/i40e_txrx.c     |    2 +-
 drivers/net/ethernet/intel/igb/igb_main.c         |    2 +-
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c     |    2 +-
 drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c |    2 +-
 6 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_main.c b/drivers/net/ethernet/intel/fm10k/fm10k_main.c
index 4eafdad318b4..1e3fe455cdea 100644
--- a/drivers/net/ethernet/intel/fm10k/fm10k_main.c
+++ b/drivers/net/ethernet/intel/fm10k/fm10k_main.c
@@ -1227,7 +1227,7 @@ static bool fm10k_clean_tx_irq(struct fm10k_q_vector *q_vector,
 		total_packets += tx_buffer->gso_segs;
 
 		/* free the skb */
-		dev_consume_skb_any(tx_buffer->skb);
+		dev_kfree_skb(tx_buffer->skb);
 
 		/* unmap skb header data */
 		dma_unmap_single(tx_ring->dev,
diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
index 4bd3a80aba82..52a6e2a609a7 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
@@ -726,7 +726,7 @@ static bool i40e_clean_tx_irq(struct i40e_ring *tx_ring, int budget)
 		total_packets += tx_buf->gso_segs;
 
 		/* free the skb */
-		dev_consume_skb_any(tx_buf->skb);
+		dev_kfree_skb(tx_buf->skb);
 
 		/* unmap skb header data */
 		dma_unmap_single(tx_ring->dev,
diff --git a/drivers/net/ethernet/intel/i40evf/i40e_txrx.c b/drivers/net/ethernet/intel/i40evf/i40e_txrx.c
index b077e02a0cc7..976b93ca996a 100644
--- a/drivers/net/ethernet/intel/i40evf/i40e_txrx.c
+++ b/drivers/net/ethernet/intel/i40evf/i40e_txrx.c
@@ -245,7 +245,7 @@ static bool i40e_clean_tx_irq(struct i40e_ring *tx_ring, int budget)
 		total_packets += tx_buf->gso_segs;
 
 		/* free the skb */
-		dev_kfree_skb_any(tx_buf->skb);
+		dev_kfree_skb(tx_buf->skb);
 
 		/* unmap skb header data */
 		dma_unmap_single(tx_ring->dev,
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index cab50593c315..699902618a6f 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -6425,7 +6425,7 @@ static bool igb_clean_tx_irq(struct igb_q_vector *q_vector)
 		total_packets += tx_buffer->gso_segs;
 
 		/* free the skb */
-		dev_consume_skb_any(tx_buffer->skb);
+		dev_kfree_skb(tx_buffer->skb);
 
 		/* unmap skb header data */
 		dma_unmap_single(tx_ring->dev,
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index d3f4b0ceb3f7..e7586663a2cf 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -1114,7 +1114,7 @@ static bool ixgbe_clean_tx_irq(struct ixgbe_q_vector *q_vector,
 		total_packets += tx_buffer->gso_segs;
 
 		/* free the skb */
-		dev_consume_skb_any(tx_buffer->skb);
+		dev_kfree_skb(tx_buffer->skb);
 
 		/* unmap skb header data */
 		dma_unmap_single(tx_ring->dev,
diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
index cca8936f2fc3..0d5b94be1f0d 100644
--- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
+++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
@@ -326,7 +326,7 @@ static bool ixgbevf_clean_tx_irq(struct ixgbevf_q_vector *q_vector,
 		total_packets += tx_buffer->gso_segs;
 
 		/* free the skb */
-		dev_kfree_skb_any(tx_buffer->skb);
+		dev_kfree_skb(tx_buffer->skb);
 
 		/* unmap skb header data */
 		dma_unmap_single(tx_ring->dev,

[Intel-wired-lan] [PATCH 1/3] igb, ixgbe(vf), i40e(vf), fm10k: Use dev_kfree_skb in Tx cleanup path

[Jeff Kirsher]

On Thu, 2015-04-30 at 13:58 -0700, Alexander Duyck wrote:
> This change replaces the use of either dev_kfree_skb_any or
> dev_consume_skb_any in the Tx cleanup paths of these drivers with
> dev_kfree_skb.  There isn't any need for the "_any" version of these
> functions since the NAPI cleanup context is not a hard irq context.
> 
> This change allows us to drop one function call from the clean-up path
> since dev_kfree_skb_any was just calling into dev_kfree_skb in these
> paths
> anyway.
> 
> Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>

Acked-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>

> ---
>  drivers/net/ethernet/intel/fm10k/fm10k_main.c     |    2 +-
>  drivers/net/ethernet/intel/i40e/i40e_txrx.c       |    2 +-
>  drivers/net/ethernet/intel/i40evf/i40e_txrx.c     |    2 +-
>  drivers/net/ethernet/intel/igb/igb_main.c         |    2 +-
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c     |    2 +-
>  drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c |    2 +-
>  6 files changed, 6 insertions(+), 6 deletions(-)


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20150430/61e5a6bb/attachment.asc>

Re: [PATCH 1/3] igb, ixgbe(vf), i40e(vf), fm10k: Use dev_kfree_skb in Tx cleanup path

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 1079 bytes --]

On Thu, 2015-04-30 at 13:58 -0700, Alexander Duyck wrote:
> This change replaces the use of either dev_kfree_skb_any or
> dev_consume_skb_any in the Tx cleanup paths of these drivers with
> dev_kfree_skb.  There isn't any need for the "_any" version of these
> functions since the NAPI cleanup context is not a hard irq context.
> 
> This change allows us to drop one function call from the clean-up path
> since dev_kfree_skb_any was just calling into dev_kfree_skb in these
> paths
> anyway.
> 
> Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>

Acked-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>

> ---
>  drivers/net/ethernet/intel/fm10k/fm10k_main.c     |    2 +-
>  drivers/net/ethernet/intel/i40e/i40e_txrx.c       |    2 +-
>  drivers/net/ethernet/intel/i40evf/i40e_txrx.c     |    2 +-
>  drivers/net/ethernet/intel/igb/igb_main.c         |    2 +-
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c     |    2 +-
>  drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c |    2 +-
>  6 files changed, 6 insertions(+), 6 deletions(-)



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[Intel-wired-lan] [PATCH 2/3] r8169: Use dev_kfree_skb in Tx cleanup path

[Alexander Duyck]

This change replaces the use of either dev_kfree_skb_any or
dev_consume_skb_any in the Tx cleanup path of this driver with
dev_kfree_skb.  There isn't any need for the "_any" version of these
functions since the NAPI cleanup context is not a hard irq context.

This change allows us to drop one function call from the clean-up path
since dev_kfree_skb_any was just calling into dev_kfree_skb in these paths
anyway.

Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
---
 drivers/net/ethernet/realtek/r8169.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index c70ab40d8698..ca94040b0ec2 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -7215,7 +7215,7 @@ static void rtl_tx(struct net_device *dev, struct rtl8169_private *tp)
 			tp->tx_stats.packets++;
 			tp->tx_stats.bytes += tx_skb->skb->len;
 			u64_stats_update_end(&tp->tx_stats.syncp);
-			dev_kfree_skb_any(tx_skb->skb);
+			dev_kfree_skb(tx_skb->skb);
 			tx_skb->skb = NULL;
 		}
 		dirty_tx++;

[PATCH 2/3] r8169: Use dev_kfree_skb in Tx cleanup path

[Alexander Duyck]

This change replaces the use of either dev_kfree_skb_any or
dev_consume_skb_any in the Tx cleanup path of this driver with
dev_kfree_skb.  There isn't any need for the "_any" version of these
functions since the NAPI cleanup context is not a hard irq context.

This change allows us to drop one function call from the clean-up path
since dev_kfree_skb_any was just calling into dev_kfree_skb in these paths
anyway.

Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
---
 drivers/net/ethernet/realtek/r8169.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index c70ab40d8698..ca94040b0ec2 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -7215,7 +7215,7 @@ static void rtl_tx(struct net_device *dev, struct rtl8169_private *tp)
 			tp->tx_stats.packets++;
 			tp->tx_stats.bytes += tx_skb->skb->len;
 			u64_stats_update_end(&tp->tx_stats.syncp);
-			dev_kfree_skb_any(tx_skb->skb);
+			dev_kfree_skb(tx_skb->skb);
 			tx_skb->skb = NULL;
 		}
 		dirty_tx++;

[Intel-wired-lan] [PATCH 2/3] r8169: Use dev_kfree_skb in Tx cleanup path

[Francois Romieu]

Alexander Duyck <alexander.h.duyck@redhat.com> :
> This change replaces the use of either dev_kfree_skb_any or
> dev_consume_skb_any in the Tx cleanup path of this driver with
> dev_kfree_skb.  There isn't any need for the "_any" version of these
> functions since the NAPI cleanup context is not a hard irq context.

netconsole ?

__dev_kfree_skb_any contains a big "if (... || irqs_disabled())" test
and the NAPI poll handler is called with irq disabled from
netpoll_send_skb_on_dev.

-- 
Ueimor

Re: [PATCH 2/3] r8169: Use dev_kfree_skb in Tx cleanup path

[Francois Romieu]

Alexander Duyck <alexander.h.duyck@redhat.com> :
> This change replaces the use of either dev_kfree_skb_any or
> dev_consume_skb_any in the Tx cleanup path of this driver with
> dev_kfree_skb.  There isn't any need for the "_any" version of these
> functions since the NAPI cleanup context is not a hard irq context.

netconsole ?

__dev_kfree_skb_any contains a big "if (... || irqs_disabled())" test
and the NAPI poll handler is called with irq disabled from
netpoll_send_skb_on_dev.

-- 
Ueimor

[Intel-wired-lan] [PATCH 2/3] r8169: Use dev_kfree_skb in Tx cleanup path

[Lino Sanfilippo]

On 01.05.2015 11:34, Francois Romieu wrote:
> Alexander Duyck <alexander.h.duyck@redhat.com> :
>> This change replaces the use of either dev_kfree_skb_any or
>> dev_consume_skb_any in the Tx cleanup path of this driver with
>> dev_kfree_skb.  There isn't any need for the "_any" version of these
>> functions since the NAPI cleanup context is not a hard irq context.
> 
> netconsole ?
> 
> __dev_kfree_skb_any contains a big "if (... || irqs_disabled())" test
> and the NAPI poll handler is called with irq disabled from
> netpoll_send_skb_on_dev.
> 

Indeed. Also there have been changes from Eric Biederman not long ago,
which did just the opposite: replace dev_kfree() with dev_kfree_any()
for that reason. See

http://marc.info/?l=linux-stable-commits&m=142965098111361&w=2

[CCing Eric]

Regards,
Lino

Re: [PATCH 2/3] r8169: Use dev_kfree_skb in Tx cleanup path

[Lino Sanfilippo]

On 01.05.2015 11:34, Francois Romieu wrote:
> Alexander Duyck <alexander.h.duyck@redhat.com> :
>> This change replaces the use of either dev_kfree_skb_any or
>> dev_consume_skb_any in the Tx cleanup path of this driver with
>> dev_kfree_skb.  There isn't any need for the "_any" version of these
>> functions since the NAPI cleanup context is not a hard irq context.
> 
> netconsole ?
> 
> __dev_kfree_skb_any contains a big "if (... || irqs_disabled())" test
> and the NAPI poll handler is called with irq disabled from
> netpoll_send_skb_on_dev.
> 

Indeed. Also there have been changes from Eric Biederman not long ago,
which did just the opposite: replace dev_kfree() with dev_kfree_any()
for that reason. See

http://marc.info/?l=linux-stable-commits&m=142965098111361&w=2

[CCing Eric]

Regards,
Lino

[Intel-wired-lan] [PATCH 2/3] r8169: Use dev_kfree_skb in Tx cleanup path

[Lino Sanfilippo]

On 01.05.2015 14:06, Lino Sanfilippo wrote:

> which did just the opposite: replace dev_kfree() with dev_kfree_any()
> for that reason. See
> 

Um, this should of course be dev_kfree_skb() and dev_kfree_skb_any().

Lino

Re: [PATCH 2/3] r8169: Use dev_kfree_skb in Tx cleanup path

[Lino Sanfilippo]

On 01.05.2015 14:06, Lino Sanfilippo wrote:

> which did just the opposite: replace dev_kfree() with dev_kfree_any()
> for that reason. See
> 

Um, this should of course be dev_kfree_skb() and dev_kfree_skb_any().

Lino

[Intel-wired-lan] [PATCH 2/3] r8169: Use dev_kfree_skb in Tx cleanup path

[David Miller]

From: Lino Sanfilippo <LinoSanfilippo@gmx.de>
Date: Fri, 01 May 2015 14:15:42 +0200

> On 01.05.2015 14:06, Lino Sanfilippo wrote:
> 
>> which did just the opposite: replace dev_kfree() with dev_kfree_any()
>> for that reason. See
>> 
> 
> Um, this should of course be dev_kfree_skb() and dev_kfree_skb_any().

Indeed.

Alex, you can't make this change.  netconsole transmits can occur
from any context whatsoever.

Re: [PATCH 2/3] r8169: Use dev_kfree_skb in Tx cleanup path

[David Miller]

From: Lino Sanfilippo <LinoSanfilippo@gmx.de>
Date: Fri, 01 May 2015 14:15:42 +0200

> On 01.05.2015 14:06, Lino Sanfilippo wrote:
> 
>> which did just the opposite: replace dev_kfree() with dev_kfree_any()
>> for that reason. See
>> 
> 
> Um, this should of course be dev_kfree_skb() and dev_kfree_skb_any().

Indeed.

Alex, you can't make this change.  netconsole transmits can occur
from any context whatsoever.

[Intel-wired-lan] [PATCH 3/3] bnx2: Use dev_kfree_skb in Tx cleanup path

[Alexander Duyck]

This change replaces the use of either dev_kfree_skb_any or
dev_consume_skb_any in the Tx cleanup path of this driver with
dev_kfree_skb.  There isn't any need for the "_any" version of these
functions since the NAPI cleanup context is not a hard irq context.

This change allows us to drop one function call from the clean-up path
since dev_kfree_skb_any was just calling into dev_kfree_skb in these paths
anyway.

Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
---
 drivers/net/ethernet/broadcom/bnx2.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/bnx2.c b/drivers/net/ethernet/broadcom/bnx2.c
index 2b66ef3d8217..a76f21a90a4e 100644
--- a/drivers/net/ethernet/broadcom/bnx2.c
+++ b/drivers/net/ethernet/broadcom/bnx2.c
@@ -2887,7 +2887,7 @@ bnx2_tx_int(struct bnx2 *bp, struct bnx2_napi *bnapi, int budget)
 		sw_cons = BNX2_NEXT_TX_BD(sw_cons);
 
 		tx_bytes += skb->len;
-		dev_kfree_skb_any(skb);
+		dev_kfree_skb(skb);
 		tx_pkt++;
 		if (tx_pkt == budget)
 			break;

[PATCH 3/3] bnx2: Use dev_kfree_skb in Tx cleanup path

[Alexander Duyck]

This change replaces the use of either dev_kfree_skb_any or
dev_consume_skb_any in the Tx cleanup path of this driver with
dev_kfree_skb.  There isn't any need for the "_any" version of these
functions since the NAPI cleanup context is not a hard irq context.

This change allows us to drop one function call from the clean-up path
since dev_kfree_skb_any was just calling into dev_kfree_skb in these paths
anyway.

Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
---
 drivers/net/ethernet/broadcom/bnx2.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/bnx2.c b/drivers/net/ethernet/broadcom/bnx2.c
index 2b66ef3d8217..a76f21a90a4e 100644
--- a/drivers/net/ethernet/broadcom/bnx2.c
+++ b/drivers/net/ethernet/broadcom/bnx2.c
@@ -2887,7 +2887,7 @@ bnx2_tx_int(struct bnx2 *bp, struct bnx2_napi *bnapi, int budget)
 		sw_cons = BNX2_NEXT_TX_BD(sw_cons);
 
 		tx_bytes += skb->len;
-		dev_kfree_skb_any(skb);
+		dev_kfree_skb(skb);
 		tx_pkt++;
 		if (tx_pkt == budget)
 			break;

[Intel-wired-lan] [PATCH 0/3] Stop using dev_kfree_skb_any in NAPI Tx clean-up

[David Miller]

From: Alexander Duyck <alexander.h.duyck@redhat.com>
Date: Thu, 30 Apr 2015 13:58:12 -0700

> This change replaces the call to dev_kfree_skb_any in several NAPI based Tx
> clean-up paths with dev_kfree_skb.  Where I made the replacement the
> function appeared to always be in softirq context so we can avoid the
> unnecessary time spent determining if we are in a hardirq context or not.

It isn't always in softirq context.

We can do a netconsole transmit from any context, and that code
can and will invoke ->poll().

As Francois noted, netpoll_send_skb_on_dev() loops trying to transmit,
and if the TX does nto give NETDEV_TX_OK it will try to execute TX
reclaim by invoking ->poll().

It's hard to trigger this, but if you do... the dev_kfree_skb_any()
really is necessary.

Re: [PATCH 0/3] Stop using dev_kfree_skb_any in NAPI Tx clean-up

[David Miller]

From: Alexander Duyck <alexander.h.duyck@redhat.com>
Date: Thu, 30 Apr 2015 13:58:12 -0700

> This change replaces the call to dev_kfree_skb_any in several NAPI based Tx
> clean-up paths with dev_kfree_skb.  Where I made the replacement the
> function appeared to always be in softirq context so we can avoid the
> unnecessary time spent determining if we are in a hardirq context or not.

It isn't always in softirq context.

We can do a netconsole transmit from any context, and that code
can and will invoke ->poll().

As Francois noted, netpoll_send_skb_on_dev() loops trying to transmit,
and if the TX does nto give NETDEV_TX_OK it will try to execute TX
reclaim by invoking ->poll().

It's hard to trigger this, but if you do... the dev_kfree_skb_any()
really is necessary.

[Intel-wired-lan] [PATCH 0/3] Stop using dev_kfree_skb_any in NAPI Tx clean-up

[Alexander Duyck]

On 05/01/2015 08:42 AM, David Miller wrote:
> From: Alexander Duyck <alexander.h.duyck@redhat.com>
> Date: Thu, 30 Apr 2015 13:58:12 -0700
>
>> This change replaces the call to dev_kfree_skb_any in several NAPI based Tx
>> clean-up paths with dev_kfree_skb.  Where I made the replacement the
>> function appeared to always be in softirq context so we can avoid the
>> unnecessary time spent determining if we are in a hardirq context or not.
> It isn't always in softirq context.
>
> We can do a netconsole transmit from any context, and that code
> can and will invoke ->poll().

I knew ndo_start_xmit was affected by that, I didn't realize that the 
napi poll routine was as well.

> As Francois noted, netpoll_send_skb_on_dev() loops trying to transmit,
> and if the TX does nto give NETDEV_TX_OK it will try to execute TX
> reclaim by invoking ->poll().
>
> It's hard to trigger this, but if you do... the dev_kfree_skb_any()
> really is necessary.

Ugh, if anything it points out that there are probably a number of bugs 
in the various drivers out there since I think there were assumptions 
made on both sides about how all the NAPI and netpoll stuff works.  I'll 
probably be submitting fixes today for a number of drivers to address 
all the spots that have been missed and little things that need to be 
addressed.

- Alex

Re: [PATCH 0/3] Stop using dev_kfree_skb_any in NAPI Tx clean-up

[Alexander Duyck]

On 05/01/2015 08:42 AM, David Miller wrote:
> From: Alexander Duyck <alexander.h.duyck@redhat.com>
> Date: Thu, 30 Apr 2015 13:58:12 -0700
>
>> This change replaces the call to dev_kfree_skb_any in several NAPI based Tx
>> clean-up paths with dev_kfree_skb.  Where I made the replacement the
>> function appeared to always be in softirq context so we can avoid the
>> unnecessary time spent determining if we are in a hardirq context or not.
> It isn't always in softirq context.
>
> We can do a netconsole transmit from any context, and that code
> can and will invoke ->poll().

I knew ndo_start_xmit was affected by that, I didn't realize that the 
napi poll routine was as well.

> As Francois noted, netpoll_send_skb_on_dev() loops trying to transmit,
> and if the TX does nto give NETDEV_TX_OK it will try to execute TX
> reclaim by invoking ->poll().
>
> It's hard to trigger this, but if you do... the dev_kfree_skb_any()
> really is necessary.

Ugh, if anything it points out that there are probably a number of bugs 
in the various drivers out there since I think there were assumptions 
made on both sides about how all the NAPI and netpoll stuff works.  I'll 
probably be submitting fixes today for a number of drivers to address 
all the spots that have been missed and little things that need to be 
addressed.

- Alex