[PATCH net-next] mlx4: use dev_kfree_skb() instead of dev_kfree_skb_any()

From: Eric Dumazet <edumazet@google.com>

Since commit e22979d96a5 (mlx4_en: Moving to Interrupts for TX
completions), we no longer can free TX skb from hard IRQ, but only from
normal softirq or process context.

Therefore, we can directly call dev_kfree_skb() from
mlx4_en_free_tx_desc() like other conventional NAPI drivers.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Yevgeny Petrilin <redacted>
Cc: Or Gerlitz <redacted>
Cc: Ying Cai <redacted>
---
 drivers/net/ethernet/mellanox/mlx4/en_tx.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_tx.c b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
index 10bba09..e182762 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_tx.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_tx.c

@@ -266,7 +266,7 @@ static u32 mlx4_en_free_tx_desc(struct mlx4_en_priv *priv,
 		}
 
 	}
-	dev_kfree_skb_any(skb);
+	dev_kfree_skb(skb);
 	return tx_info->nr_txbb;
 }
 

On Mon, Sep 17, 2012 at 10:29 AM, Eric Dumazet [off-list ref] wrote:

Since commit e22979d96a5 (mlx4_en: Moving to Interrupts for TX
completions), we no longer can free TX skb from hard IRQ, but only from
normal softirq or process context.

Therefore, we can directly call dev_kfree_skb() from
mlx4_en_free_tx_desc() like other conventional NAPI drivers.

Hi Eric,

The team is all off till tomorrow, we will look and get back to you

Or.

Since commit e22979d96a5 (mlx4_en: Moving to Interrupts for TX
completions), we no longer can free TX skb from hard IRQ, but only from
normal softirq or process context.

Therefore, we can directly call dev_kfree_skb() from
mlx4_en_free_tx_desc() like other conventional NAPI drivers.

Hi Eric,
At the moment the TX completion processing is done from IRQ context.
So I think we need to change the driver to work with NAPI for TX completions
before making this change.

I'll send the patch in a few days.

Yevgeny

On Wed, 2012-09-19 at 07:58 +0000, Yevgeny Petrilin wrote:

quoted

Since commit e22979d96a5 (mlx4_en: Moving to Interrupts for TX
completions), we no longer can free TX skb from hard IRQ, but only from
normal softirq or process context.

Therefore, we can directly call dev_kfree_skb() from
mlx4_en_free_tx_desc() like other conventional NAPI drivers.

Hi Eric,
At the moment the TX completion processing is done from IRQ context.
So I think we need to change the driver to work with NAPI for TX completions
before making this change.

I'll send the patch in a few days.

Oops you're right, it seems I misread e22979d96 commit.

irq term is a bit generic, you might add soft/hard qualifiers to
distinguish the variant.

Thanks

Hi Yevgeny,

It seems all the TxQs are sharing the same interrupt for Tx
completions. Will it be better to have separate interrupt per
num_tx_rings_p_up (8) queues? E.g. for a 16 core system, with 16 * 8
Tx queues, to have 16 interrupts for Tx completions of those 128 Tx
queues?

Also I'm looking at mlx4_en_select_queue(), it is using
__skb_tx_hash(). Use something to achieve XPS may bring better
performances.

Thanks.

On Wed, Sep 19, 2012 at 5:12 AM, Eric Dumazet [off-list ref] wrote:

On Wed, 2012-09-19 at 07:58 +0000, Yevgeny Petrilin wrote:

quoted

quoted

Since commit e22979d96a5 (mlx4_en: Moving to Interrupts for TX
completions), we no longer can free TX skb from hard IRQ, but only
from
normal softirq or process context.

Therefore, we can directly call dev_kfree_skb() from
mlx4_en_free_tx_desc() like other conventional NAPI drivers.

Hi Eric,
At the moment the TX completion processing is done from IRQ context.
So I think we need to change the driver to work with NAPI for TX
completions
before making this change.

I'll send the patch in a few days.

Oops you're right, it seems I misread e22979d96 commit.

irq term is a bit generic, you might add soft/hard qualifiers to
distinguish the variant.

Thanks

Hi Ying,
 

It seems all the TxQs are sharing the same interrupt for Tx
completions. Will it be better to have separate interrupt per
num_tx_rings_p_up (8) queues? E.g. for a 16 core system, with 16 * 8
Tx queues, to have 16 interrupts for Tx completions of those 128 Tx
queues?

Actually not all TxQs share same interrupt vector.
In commit 76532d0c we assigned an interrupt vector for each TX ring.
When the number of Queues is higher than number of interrupt vectors, there are queues that share interrupts
And actually reaching the assignment you specified.

Also I'm looking at mlx4_en_select_queue(), it is using
__skb_tx_hash(). Use something to achieve XPS may bring better
performances.

We are considering this change. 

From: Eric Dumazet <edumazet@google.com>

After commit e22979d96a55d (mlx4_en: Moving to Interrupts for TX
completions) we no longer need to orphan skbs in mlx4_en_xmit()
since skb wont stay a long time in TX ring before their release.

Orphaning skbs in ndo_start_xmit() should be avoided as much as
possible, since it breaks TCP Small Queue or other flow control
mechanisms (per socket limits)

Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Yevgeny Petrilin <redacted>
Cc: Or Gerlitz <redacted>
---
 drivers/net/ethernet/mellanox/mlx4/en_tx.c |    4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_tx.c b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
index 10bba09..c10e3a6 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_tx.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_tx.c

@@ -712,10 +712,6 @@ netdev_tx_t mlx4_en_xmit(struct sk_buff *skb, struct net_device *dev)
 	if (bounce)
 		tx_desc = mlx4_en_bounce_to_desc(priv, ring, index, desc_size);
 
-	/* Run destructor before passing skb to HW */
-	if (likely(!skb_shared(skb)))
-		skb_orphan(skb);
-
 	if (ring->bf_enabled && desc_size <= MAX_BF && !bounce && !vlan_tag) {
 		*(__be32 *) (&tx_desc->ctrl.vlan_tag) |= cpu_to_be32(ring->doorbell_qpn);
 		op_own |= htonl((bf_index & 0xffff) << 8);

Acked-by: Yevgeny Petrilin <redacted>

From: Yevgeny Petrilin <redacted>
Date: Sun, 30 Sep 2012 06:49:34 +0000

Acked-by: Yevgeny Petrilin <redacted>

Stop top posting.

Please learn how to quote emails like is canonically done here on this
mailing list if you want to participate properly.

Thanks.

From: Eric Dumazet <redacted>
Date: Fri, 28 Sep 2012 19:53:26 +0200

From: Eric Dumazet <edumazet@google.com>

After commit e22979d96a55d (mlx4_en: Moving to Interrupts for TX
completions) we no longer need to orphan skbs in mlx4_en_xmit()
since skb wont stay a long time in TX ring before their release.

Orphaning skbs in ndo_start_xmit() should be avoided as much as
possible, since it breaks TCP Small Queue or other flow control
mechanisms (per socket limits)

Signed-off-by: Eric Dumazet <edumazet@google.com>

Applied.