* [PATCH 1/3] igb, ixgbe(vf), i40e(vf), fm10k: Use dev_kfree_skb in Tx cleanup path
2015-04-30 20:58 [PATCH 0/3] Stop using dev_kfree_skb_any in NAPI Tx clean-up Alexander Duyck
@ 2015-04-30 20:58 ` Alexander Duyck
2015-05-01 1:46 ` Jeff Kirsher
2015-04-30 20:58 ` [PATCH 2/3] r8169: " Alexander Duyck
` (2 subsequent siblings)
3 siblings, 1 reply; 11+ messages in thread
From: Alexander Duyck @ 2015-04-30 20:58 UTC (permalink / raw)
To: netdev
Cc: nic_swsd, Dept-HSGLinuxNICDev, sony.chacko, intel-wired-lan,
jeffrey.t.kirsher, davem
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,
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH 1/3] igb, ixgbe(vf), i40e(vf), fm10k: Use dev_kfree_skb in Tx cleanup path
2015-04-30 20:58 ` [PATCH 1/3] igb, ixgbe(vf), i40e(vf), fm10k: Use dev_kfree_skb in Tx cleanup path Alexander Duyck
@ 2015-05-01 1:46 ` Jeff Kirsher
0 siblings, 0 replies; 11+ messages in thread
From: Jeff Kirsher @ 2015-05-01 1:46 UTC (permalink / raw)
To: Alexander Duyck
Cc: netdev, nic_swsd, Dept-HSGLinuxNICDev, sony.chacko,
intel-wired-lan, davem
[-- 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 --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/3] r8169: Use dev_kfree_skb in Tx cleanup path
2015-04-30 20:58 [PATCH 0/3] Stop using dev_kfree_skb_any in NAPI Tx clean-up Alexander Duyck
2015-04-30 20:58 ` [PATCH 1/3] igb, ixgbe(vf), i40e(vf), fm10k: Use dev_kfree_skb in Tx cleanup path Alexander Duyck
@ 2015-04-30 20:58 ` Alexander Duyck
2015-05-01 9:34 ` Francois Romieu
2015-04-30 20:58 ` [PATCH 3/3] bnx2: " Alexander Duyck
2015-05-01 15:42 ` [PATCH 0/3] Stop using dev_kfree_skb_any in NAPI Tx clean-up David Miller
3 siblings, 1 reply; 11+ messages in thread
From: Alexander Duyck @ 2015-04-30 20:58 UTC (permalink / raw)
To: netdev
Cc: nic_swsd, Dept-HSGLinuxNICDev, sony.chacko, intel-wired-lan,
jeffrey.t.kirsher, davem
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++;
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] r8169: Use dev_kfree_skb in Tx cleanup path
2015-04-30 20:58 ` [PATCH 2/3] r8169: " Alexander Duyck
@ 2015-05-01 9:34 ` Francois Romieu
2015-05-01 12:06 ` Lino Sanfilippo
0 siblings, 1 reply; 11+ messages in thread
From: Francois Romieu @ 2015-05-01 9:34 UTC (permalink / raw)
To: Alexander Duyck
Cc: netdev, nic_swsd, Dept-HSGLinuxNICDev, sony.chacko,
intel-wired-lan, jeffrey.t.kirsher, davem
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] r8169: Use dev_kfree_skb in Tx cleanup path
2015-05-01 9:34 ` Francois Romieu
@ 2015-05-01 12:06 ` Lino Sanfilippo
2015-05-01 12:15 ` Lino Sanfilippo
0 siblings, 1 reply; 11+ messages in thread
From: Lino Sanfilippo @ 2015-05-01 12:06 UTC (permalink / raw)
To: Francois Romieu, Alexander Duyck
Cc: netdev, nic_swsd, Dept-HSGLinuxNICDev, sony.chacko,
intel-wired-lan, jeffrey.t.kirsher, davem, ebiederm
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] r8169: Use dev_kfree_skb in Tx cleanup path
2015-05-01 12:06 ` Lino Sanfilippo
@ 2015-05-01 12:15 ` Lino Sanfilippo
2015-05-01 15:36 ` David Miller
0 siblings, 1 reply; 11+ messages in thread
From: Lino Sanfilippo @ 2015-05-01 12:15 UTC (permalink / raw)
To: Francois Romieu, Alexander Duyck
Cc: netdev, nic_swsd, Dept-HSGLinuxNICDev, sony.chacko,
intel-wired-lan, jeffrey.t.kirsher, davem, ebiederm
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] r8169: Use dev_kfree_skb in Tx cleanup path
2015-05-01 12:15 ` Lino Sanfilippo
@ 2015-05-01 15:36 ` David Miller
0 siblings, 0 replies; 11+ messages in thread
From: David Miller @ 2015-05-01 15:36 UTC (permalink / raw)
To: LinoSanfilippo
Cc: romieu, alexander.h.duyck, netdev, nic_swsd, Dept-HSGLinuxNICDev,
sony.chacko, intel-wired-lan, jeffrey.t.kirsher, ebiederm
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 3/3] bnx2: Use dev_kfree_skb in Tx cleanup path
2015-04-30 20:58 [PATCH 0/3] Stop using dev_kfree_skb_any in NAPI Tx clean-up Alexander Duyck
2015-04-30 20:58 ` [PATCH 1/3] igb, ixgbe(vf), i40e(vf), fm10k: Use dev_kfree_skb in Tx cleanup path Alexander Duyck
2015-04-30 20:58 ` [PATCH 2/3] r8169: " Alexander Duyck
@ 2015-04-30 20:58 ` Alexander Duyck
2015-05-01 15:42 ` [PATCH 0/3] Stop using dev_kfree_skb_any in NAPI Tx clean-up David Miller
3 siblings, 0 replies; 11+ messages in thread
From: Alexander Duyck @ 2015-04-30 20:58 UTC (permalink / raw)
To: netdev
Cc: nic_swsd, Dept-HSGLinuxNICDev, sony.chacko, intel-wired-lan,
jeffrey.t.kirsher, davem
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;
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 0/3] Stop using dev_kfree_skb_any in NAPI Tx clean-up
2015-04-30 20:58 [PATCH 0/3] Stop using dev_kfree_skb_any in NAPI Tx clean-up Alexander Duyck
` (2 preceding siblings ...)
2015-04-30 20:58 ` [PATCH 3/3] bnx2: " Alexander Duyck
@ 2015-05-01 15:42 ` David Miller
2015-05-01 16:45 ` Alexander Duyck
3 siblings, 1 reply; 11+ messages in thread
From: David Miller @ 2015-05-01 15:42 UTC (permalink / raw)
To: alexander.h.duyck
Cc: netdev, nic_swsd, Dept-HSGLinuxNICDev, sony.chacko,
intel-wired-lan, jeffrey.t.kirsher
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.
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 0/3] Stop using dev_kfree_skb_any in NAPI Tx clean-up
2015-05-01 15:42 ` [PATCH 0/3] Stop using dev_kfree_skb_any in NAPI Tx clean-up David Miller
@ 2015-05-01 16:45 ` Alexander Duyck
0 siblings, 0 replies; 11+ messages in thread
From: Alexander Duyck @ 2015-05-01 16:45 UTC (permalink / raw)
To: David Miller
Cc: netdev, nic_swsd, Dept-HSGLinuxNICDev, sony.chacko,
intel-wired-lan, jeffrey.t.kirsher
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
^ permalink raw reply [flat|nested] 11+ messages in thread