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

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

[Eric Dumazet]

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 <yevgenyp@mellanox.co.il>
Cc: Or Gerlitz <ogerlitz@mellanox.com>
Cc: Ying Cai <ycai@google.com>
---
 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;
 }
 

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

[Or Gerlitz]

On Mon, Sep 17, 2012 at 10:29 AM, Eric Dumazet <eric.dumazet@gmail.com> 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.

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

[Yevgeny Petrilin]

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

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

[Eric Dumazet]

On Wed, 2012-09-19 at 07:58 +0000, Yevgeny Petrilin 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,
> 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

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

[Ying Cai]

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 <eric.dumazet@gmail.com> wrote:
>
> On Wed, 2012-09-19 at 07:58 +0000, Yevgeny Petrilin 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,
> > 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
>
>

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

[Yevgeny Petrilin]

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. 

[PATCH net-next] mlx4: dont orphan skbs in mlx4_en_xmit()

[Eric Dumazet]

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 <yevgenyp@mellanox.com>
Cc: Or Gerlitz <ogerlitz@mellanox.com>
---
 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);

RE: [PATCH net-next] mlx4: dont orphan skbs in mlx4_en_xmit()

[Yevgeny Petrilin]

Acked-by: Yevgeny Petrilin <yevgenyp@mellanox.com>

> -----Original Message-----
> From: Eric Dumazet [mailto:eric.dumazet@gmail.com]
> Sent: Friday, September 28, 2012 7:54 PM
> To: Yevgeny Petrilin
> Cc: David Miller; netdev; Or Gerlitz
> Subject: [PATCH net-next] mlx4: dont orphan skbs in mlx4_en_xmit()
> 
> 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 <yevgenyp@mellanox.com>
> Cc: Or Gerlitz <ogerlitz@mellanox.com>
> ---
>  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);
> 

Re: [PATCH net-next] mlx4: dont orphan skbs in mlx4_en_xmit()

[David Miller]

From: Yevgeny Petrilin <yevgenyp@mellanox.com>
Date: Sun, 30 Sep 2012 06:49:34 +0000

> Acked-by: Yevgeny Petrilin <yevgenyp@mellanox.com>

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.

Re: [PATCH net-next] mlx4: dont orphan skbs in mlx4_en_xmit()

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
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.