[PATCH net-next-2.6] net: eth_type_trans() should inline skb_pull()

[PATCH net-next-2.6] net: eth_type_trans() should inline skb_pull()

[Eric Dumazet]

840.000 pps instead of 800.000 pps on my 'old' machine, using RPS

Before patch, profile of CPU 0 (handling tg3 interrupts)

             2167.00 13.9% __alloc_skb            vmlinux
             1908.00 12.3% eth_type_trans         vmlinux
             1125.00  7.2% __kmalloc_track_caller vmlinux
              981.00  6.3% __netdev_alloc_skb     vmlinux
              925.00  5.9% _raw_spin_lock         vmlinux
              786.00  5.1% kmem_cache_alloc       vmlinux
              757.00  4.9% skb_pull               vmlinux
              698.00  4.5% tg3_read32             vmlinux
              637.00  4.1% __slab_alloc           vmlinux
              620.00  4.0% tg3_poll_work          vmlinux
              576.00  3.7% get_rps_cpu            vmlinux
              448.00  2.9% bnx2_interrupt         vmlinux

After (no more skb_pull, and eth_type_trans() not more expensive)
Predominant cost is memory allocator...

             1625.00 12.4% eth_type_trans         vmlinux
             1468.00 11.2% __alloc_skb            vmlinux
             1004.00  7.6% __kmalloc_track_caller vmlinux
              893.00  6.8% _raw_spin_lock         vmlinux
              738.00  5.6% __netdev_alloc_skb     vmlinux
              665.00  5.1% tg3_read32             vmlinux
              656.00  5.0% kmem_cache_alloc       vmlinux
              655.00  5.0% __slab_alloc           vmlinux
              509.00  3.9% bnx2_interrupt         vmlinux
              483.00  3.7% tg3_poll_work          vmlinux
              455.00  3.5% _raw_spin_lock_irqsave vmlinux
              330.00  2.5% get_rps_cpu            vmlinux
              286.00  2.2% nommu_map_page         vmlinux
              277.00  2.1% enqueue_to_backlog     vmlinux
              235.00  1.8% inet_gro_receive       vmlinux
              232.00  1.8% __copy_to_user_ll      vmlinux
              181.00  1.4% dev_gro_receive        vmlinux
              165.00  1.3% skb_gro_reset_offset   vmlinux

(bnx2_interrupt is called, because irq 16 is shared on this machine on two nics...)

Thanks !

[PATCH net-next-2.6] net: eth_type_trans() should inline skb_pull()

With RPS, this patch can give a 5 % boost in performance.

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
index 0c0d272..763524b 100644
--- a/net/ethernet/eth.c
+++ b/net/ethernet/eth.c
@@ -162,7 +162,8 @@ __be16 eth_type_trans(struct sk_buff *skb, struct net_device *dev)
 
 	skb->dev = dev;
 	skb_reset_mac_header(skb);
-	skb_pull(skb, ETH_HLEN);
+	if (likely(skb->len >= ETH_HLEN))
+		__skb_pull(skb, ETH_HLEN);
 	eth = eth_hdr(skb);
 
 	if (unlikely(is_multicast_ether_addr(eth->h_dest))) {

Re: [PATCH net-next-2.6] net: eth_type_trans() should inline skb_pull()

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Sat, 01 May 2010 08:42:25 +0200

> [PATCH net-next-2.6] net: eth_type_trans() should inline skb_pull()
> 
> With RPS, this patch can give a 5 % boost in performance.
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>

Awesome, but let's do this in a way that allows us to easily annotate
where inlining makes sense in other places, not just here.

Something like this, ok?

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 82f5116..746a652 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -1128,6 +1128,11 @@ static inline unsigned char *__skb_pull(struct sk_buff *skb, unsigned int len)
 	return skb->data += len;
 }
 
+static inline unsigned char *skb_pull_inline(struct sk_buff *skb, unsigned int len)
+{
+	return unlikely(len > skb->len) ? NULL : __skb_pull(skb, len);
+}
+
 extern unsigned char *__pskb_pull_tail(struct sk_buff *skb, int delta);
 
 static inline unsigned char *__pskb_pull(struct sk_buff *skb, unsigned int len)
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 4218ff4..8b9c109 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -1051,7 +1051,7 @@ EXPORT_SYMBOL(skb_push);
  */
 unsigned char *skb_pull(struct sk_buff *skb, unsigned int len)
 {
-	return unlikely(len > skb->len) ? NULL : __skb_pull(skb, len);
+	return skb_pull_inline(skb, len);
 }
 EXPORT_SYMBOL(skb_pull);
 
diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
index 0c0d272..61ec032 100644
--- a/net/ethernet/eth.c
+++ b/net/ethernet/eth.c
@@ -162,7 +162,7 @@ __be16 eth_type_trans(struct sk_buff *skb, struct net_device *dev)
 
 	skb->dev = dev;
 	skb_reset_mac_header(skb);
-	skb_pull(skb, ETH_HLEN);
+	skb_pull_inline(skb, ETH_HLEN);
 	eth = eth_hdr(skb);
 
 	if (unlikely(is_multicast_ether_addr(eth->h_dest))) {

Re: [PATCH net-next-2.6] net: eth_type_trans() should inline skb_pull()

[Eric Dumazet]

Le samedi 01 mai 2010 à 18:15 -0700, David Miller a écrit :
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Sat, 01 May 2010 08:42:25 +0200
> 
> > [PATCH net-next-2.6] net: eth_type_trans() should inline skb_pull()
> > 
> > With RPS, this patch can give a 5 % boost in performance.
> > 
> > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> 
> Awesome, but let's do this in a way that allows us to easily annotate
> where inlining makes sense in other places, not just here.
> 
> Something like this, ok?
> 
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 82f5116..746a652 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -1128,6 +1128,11 @@ static inline unsigned char *__skb_pull(struct sk_buff *skb, unsigned int len)
>  	return skb->data += len;
>  }
>  
> +static inline unsigned char *skb_pull_inline(struct sk_buff *skb, unsigned int len)
> +{
> +	return unlikely(len > skb->len) ? NULL : __skb_pull(skb, len);
> +}
> +
>  extern unsigned char *__pskb_pull_tail(struct sk_buff *skb, int delta);
>  
>  static inline unsigned char *__pskb_pull(struct sk_buff *skb, unsigned int len)
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 4218ff4..8b9c109 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -1051,7 +1051,7 @@ EXPORT_SYMBOL(skb_push);
>   */
>  unsigned char *skb_pull(struct sk_buff *skb, unsigned int len)
>  {
> -	return unlikely(len > skb->len) ? NULL : __skb_pull(skb, len);
> +	return skb_pull_inline(skb, len);
>  }
>  EXPORT_SYMBOL(skb_pull);
>  
> diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
> index 0c0d272..61ec032 100644
> --- a/net/ethernet/eth.c
> +++ b/net/ethernet/eth.c
> @@ -162,7 +162,7 @@ __be16 eth_type_trans(struct sk_buff *skb, struct net_device *dev)
>  
>  	skb->dev = dev;
>  	skb_reset_mac_header(skb);
> -	skb_pull(skb, ETH_HLEN);
> +	skb_pull_inline(skb, ETH_HLEN);
>  	eth = eth_hdr(skb);
>  
>  	if (unlikely(is_multicast_ether_addr(eth->h_dest))) {

Excellent !

Changli privately asked me why we were ignoring cases where skb->len <
ETH_HLEN.
I replied that minimum frame size was 46+12, then he asked me why we
were testing another time :

if (skb->len >= 2 && *(unsigned short *)rawp == 0xFFFF)
	return htons(ETH_P_802_3);


Could we assume all eth_type_trans() must call it with initial skb->len
>= (46 + 12) or not ?
(According to ethernet specs, all frames should have a minimum payload
of 46 bytes)

If not sure, maybe we should issue a WARN_ON_ONCE()

If yes, tests could be removed and we could gain two cycles ;)

Re: [PATCH net-next-2.6] net: eth_type_trans() should inline skb_pull()

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Sun, 02 May 2010 08:50:32 +0200

> Excellent !

Great, here is the commit message I will use:

--------------------
net: Inline skb_pull() in eth_type_trans().

In commit 6be8ac2f ("[NET]: uninline skb_pull, de-bloats a lot")
we uninlined skb_pull.

But in some critical paths it makes sense to inline this thing
and it helps performance significantly.

Create an skb_pull_inline() so that we can do this in a way that
serves also as annotation.

Based upon a patch by Eric Dumazet.

Signed-off-by: David S. Miller <davem@davemloft.net>
--------------------

> Could we assume all eth_type_trans() must call it with initial
> skb->len >= (46 + 12) or not ?  (According to ethernet specs, all
> frames should have a minimum payload of 46 bytes)
>
> If not sure, maybe we should issue a WARN_ON_ONCE()
> 
> If yes, tests could be removed and we could gain two cycles ;)

Isn't the minimum ETH_ZLEN?

But yes, regardless of whether the minimum ethernet frame is 58 or 60
bytes, we should require it's at least that big, and use that test
consistently throughout.

Anything smaller is a runt packet and should be tossed or marked as an
error in some other way by the hardware.  They should never make it to
eth_type_trans().