[PATCH] net: increase skb->users instead of skb_clone()

[PATCH] net: increase skb->users instead of skb_clone()

[Changli Gao]

In dev_queue_xmit_nit(), we have to clone skbs as we need to mangle skbs,
however, we don't need to clone skbs for all the packet_types.

Except for the first packet_type, we increase skb->users instead of
skb_clone().

Signed-off-by: Changli Gao <xiaosuo@gmail.com>
---
 net/core/dev.c |   30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)
diff --git a/net/core/dev.c b/net/core/dev.c
index bf5ced5..888cb74 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1496,6 +1496,14 @@ int dev_forward_skb(struct net_device *dev, struct sk_buff *skb)
 }
 EXPORT_SYMBOL_GPL(dev_forward_skb);
 
+static inline int deliver_skb(struct sk_buff *skb,
+			      struct packet_type *pt_prev,
+			      struct net_device *orig_dev)
+{
+	atomic_inc(&skb->users);
+	return pt_prev->func(skb, skb->dev, pt_prev, orig_dev);
+}
+
 /*
  *	Support routine. Sends outgoing frames to any network
  *	taps currently in use.
@@ -1504,6 +1512,8 @@ EXPORT_SYMBOL_GPL(dev_forward_skb);
 static void dev_queue_xmit_nit(struct sk_buff *skb, struct net_device *dev)
 {
 	struct packet_type *ptype;
+	struct sk_buff *skb2 = NULL;
+	struct packet_type *pt_prev = NULL;
 
 #ifdef CONFIG_NET_CLS_ACT
 	if (!(skb->tstamp.tv64 && (G_TC_FROM(skb->tc_verd) & AT_INGRESS)))
@@ -1520,7 +1530,13 @@ static void dev_queue_xmit_nit(struct sk_buff *skb, struct net_device *dev)
 		if ((ptype->dev == dev || !ptype->dev) &&
 		    (ptype->af_packet_priv == NULL ||
 		     (struct sock *)ptype->af_packet_priv != skb->sk)) {
-			struct sk_buff *skb2 = skb_clone(skb, GFP_ATOMIC);
+			if (pt_prev) {
+				deliver_skb(skb2, pt_prev, skb->dev);
+				pt_prev = ptype;
+				continue;
+			}
+
+			skb2 = skb_clone(skb, GFP_ATOMIC);
 			if (!skb2)
 				break;
 
@@ -1542,9 +1558,11 @@ static void dev_queue_xmit_nit(struct sk_buff *skb, struct net_device *dev)
 
 			skb2->transport_header = skb2->network_header;
 			skb2->pkt_type = PACKET_OUTGOING;
-			ptype->func(skb2, skb->dev, ptype, skb->dev);
+			pt_prev = ptype;
 		}
 	}
+	if (pt_prev)
+		pt_prev->func(skb2, skb->dev, pt_prev, skb->dev);
 	rcu_read_unlock();
 }
 
@@ -2788,14 +2806,6 @@ static void net_tx_action(struct softirq_action *h)
 	}
 }
 
-static inline int deliver_skb(struct sk_buff *skb,
-			      struct packet_type *pt_prev,
-			      struct net_device *orig_dev)
-{
-	atomic_inc(&skb->users);
-	return pt_prev->func(skb, skb->dev, pt_prev, orig_dev);
-}
-
 #if (defined(CONFIG_BRIDGE) || defined(CONFIG_BRIDGE_MODULE)) && \
     (defined(CONFIG_ATM_LANE) || defined(CONFIG_ATM_LANE_MODULE))
 /* This hook is defined here for ATM LANE */

Re: [PATCH] net: increase skb->users instead of skb_clone()

[Eric Dumazet]

Le jeudi 16 décembre 2010 à 13:57 +0800, Changli Gao a écrit :
> In dev_queue_xmit_nit(), we have to clone skbs as we need to mangle skbs,
> however, we don't need to clone skbs for all the packet_types.
> 
> Except for the first packet_type, we increase skb->users instead of
> skb_clone().
> 
> Signed-off-by: Changli Gao <xiaosuo@gmail.com>
> ---
>  net/core/dev.c |   30 ++++++++++++++++++++----------
>  1 file changed, 20 insertions(+), 10 deletions(-)
> diff --git a/net/core/dev.c b/net/core/dev.c
> index bf5ced5..888cb74 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -1496,6 +1496,14 @@ int dev_forward_skb(struct net_device *dev, struct sk_buff *skb)
>  }
>  EXPORT_SYMBOL_GPL(dev_forward_skb);
>  
> +static inline int deliver_skb(struct sk_buff *skb,
> +			      struct packet_type *pt_prev,
> +			      struct net_device *orig_dev)
> +{
> +	atomic_inc(&skb->users);
> +	return pt_prev->func(skb, skb->dev, pt_prev, orig_dev);
> +}
> +
>  /*
>   *	Support routine. Sends outgoing frames to any network
>   *	taps currently in use.
> @@ -1504,6 +1512,8 @@ EXPORT_SYMBOL_GPL(dev_forward_skb);
>  static void dev_queue_xmit_nit(struct sk_buff *skb, struct net_device *dev)
>  {
>  	struct packet_type *ptype;
> +	struct sk_buff *skb2 = NULL;
> +	struct packet_type *pt_prev = NULL;
>  
>  #ifdef CONFIG_NET_CLS_ACT
>  	if (!(skb->tstamp.tv64 && (G_TC_FROM(skb->tc_verd) & AT_INGRESS)))
> @@ -1520,7 +1530,13 @@ static void dev_queue_xmit_nit(struct sk_buff *skb, struct net_device *dev)
>  		if ((ptype->dev == dev || !ptype->dev) &&
>  		    (ptype->af_packet_priv == NULL ||
>  		     (struct sock *)ptype->af_packet_priv != skb->sk)) {
> -			struct sk_buff *skb2 = skb_clone(skb, GFP_ATOMIC);
> +			if (pt_prev) {
> +				deliver_skb(skb2, pt_prev, skb->dev);
> +				pt_prev = ptype;
> +				continue;
> +			}
> +
> +			skb2 = skb_clone(skb, GFP_ATOMIC);
>  			if (!skb2)
>  				break;
>  
> @@ -1542,9 +1558,11 @@ static void dev_queue_xmit_nit(struct sk_buff *skb, struct net_device *dev)
>  
>  			skb2->transport_header = skb2->network_header;
>  			skb2->pkt_type = PACKET_OUTGOING;
> -			ptype->func(skb2, skb->dev, ptype, skb->dev);
> +			pt_prev = ptype;
>  		}
>  	}
> +	if (pt_prev)
> +		pt_prev->func(skb2, skb->dev, pt_prev, skb->dev);
>  	rcu_read_unlock();
>  }
>  
> @@ -2788,14 +2806,6 @@ static void net_tx_action(struct softirq_action *h)
>  	}
>  }
>  
> -static inline int deliver_skb(struct sk_buff *skb,
> -			      struct packet_type *pt_prev,
> -			      struct net_device *orig_dev)
> -{
> -	atomic_inc(&skb->users);
> -	return pt_prev->func(skb, skb->dev, pt_prev, orig_dev);
> -}
> -
>  #if (defined(CONFIG_BRIDGE) || defined(CONFIG_BRIDGE_MODULE)) && \
>      (defined(CONFIG_ATM_LANE) || defined(CONFIG_ATM_LANE_MODULE))
>  /* This hook is defined here for ATM LANE */

You beat me, but I was thinking of a different way, adding a new
pt_prev->xmit_func(), handling all the details (no need for atomic ops
on skb users if packet is not delivered at all).

By the way, your patch is not 100% safe/OK, because af_packet rcv()
handler writes on skb (skb_pull() and all)

Re: [PATCH] net: increase skb->users instead of skb_clone()

[Changli Gao]

On Thu, Dec 16, 2010 at 3:18 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
> You beat me, but I was thinking of a different way, adding a new
> pt_prev->xmit_func(), handling all the details (no need for atomic ops
> on skb users if packet is not delivered at all).
>
> By the way, your patch is not 100% safe/OK, because af_packet rcv()
> handler writes on skb (skb_pull() and all)
>

But af_packet_rcv() restores skbs at last.

        if (skb_head != skb->data && skb_shared(skb)) {
                skb->data = skb_head;
                skb->len = skb_len;
        }

-- 
Regards,
Changli Gao(xiaosuo@gmail.com)

Re: [PATCH] net: increase skb->users instead of skb_clone()

[Eric Dumazet]

Le jeudi 16 décembre 2010 à 15:23 +0800, Changli Gao a écrit :
> On Thu, Dec 16, 2010 at 3:18 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> >
> > You beat me, but I was thinking of a different way, adding a new
> > pt_prev->xmit_func(), handling all the details (no need for atomic ops
> > on skb users if packet is not delivered at all).
> >
> > By the way, your patch is not 100% safe/OK, because af_packet rcv()
> > handler writes on skb (skb_pull() and all)
> >
> 
> But af_packet_rcv() restores skbs at last.
> 
>         if (skb_head != skb->data && skb_shared(skb)) {
>                 skb->data = skb_head;
>                 skb->len = skb_len;
>         }
> 

Thats right, your patch seems fine, thanks !

Acked-by: Eric Dumazet <eric.dumazet@gmail.com>

Re: [PATCH] net: increase skb->users instead of skb_clone()

[Junchang Wang]

On Thu, Dec 16, 2010 at 1:57 PM, Changli Gao <xiaosuo@gmail.com> wrote:
> In dev_queue_xmit_nit(), we have to clone skbs as we need to mangle skbs,
> however, we don't need to clone skbs for all the packet_types.
>
> Except for the first packet_type, we increase skb->users instead of
> skb_clone().

Hi Changli,
Take af_packet for example, I can't see benefit from this patch.

> +static inline int deliver_skb(struct sk_buff *skb,
> +                             struct packet_type *pt_prev,
> +                             struct net_device *orig_dev)
> +{
> +       atomic_inc(&skb->users);
> +       return pt_prev->func(skb, skb->dev, pt_prev, orig_dev);
> +}
The increment call will incur skb_shared() failure in packet_rcv.
In reality, packet_rcv has to clone this packet by itself.


Thanks

-- 
--Junchang

Re: [PATCH] net: increase skb->users instead of skb_clone()

[Changli Gao]

On Thu, Dec 16, 2010 at 10:05 PM, Junchang Wang <junchangwang@gmail.com> wrote:
>
> Hi Changli,
> Take af_packet for example, I can't see benefit from this patch.
>
>> +static inline int deliver_skb(struct sk_buff *skb,
>> +                             struct packet_type *pt_prev,
>> +                             struct net_device *orig_dev)
>> +{
>> +       atomic_inc(&skb->users);
>> +       return pt_prev->func(skb, skb->dev, pt_prev, orig_dev);
>> +}
> The increment call will incur skb_shared() failure in packet_rcv.
> In reality, packet_rcv has to clone this packet by itself.
>
>

This happens when run_filter returns non zero. For your case, only
small parts of packets match bpf filter.


-- 
Regards,
Changli Gao(xiaosuo@gmail.com)

Re: [PATCH] net: increase skb->users instead of skb_clone()

[Eric Dumazet]

Le jeudi 16 décembre 2010 à 22:05 +0800, Junchang Wang a écrit :
> On Thu, Dec 16, 2010 at 1:57 PM, Changli Gao <xiaosuo@gmail.com> wrote:
> > In dev_queue_xmit_nit(), we have to clone skbs as we need to mangle skbs,
> > however, we don't need to clone skbs for all the packet_types.
> >
> > Except for the first packet_type, we increase skb->users instead of
> > skb_clone().
> 
> Hi Changli,
> Take af_packet for example, I can't see benefit from this patch.
> 
> > +static inline int deliver_skb(struct sk_buff *skb,
> > +                             struct packet_type *pt_prev,
> > +                             struct net_device *orig_dev)
> > +{
> > +       atomic_inc(&skb->users);
> > +       return pt_prev->func(skb, skb->dev, pt_prev, orig_dev);
> > +}
> The increment call will incur skb_shared() failure in packet_rcv.
> In reality, packet_rcv has to clone this packet by itself.
> 

Yes, and no.

Consider the case you have one receiver.

Packet given after Changli patch wont be shared, so packet_rcv wont
clone it : Thats a win. Only one skb_clone() done instead of two.

Consider case with 2 receivers :

First time we call packet_rcv, packet is shared (because we call
deliver_skb(), so packet_rcv clones it. Normal situation, we really need
to clone it.

Second time, we give a non shared packet : Thats a win over previous
situation.

Re: [PATCH] net: increase skb->users instead of skb_clone()

[Junchang Wang]

On Thu, Dec 16, 2010 at 3:23 PM, Changli Gao <xiaosuo@gmail.com> wrote:

>> You beat me, but I was thinking of a different way, adding a new
>> pt_prev->xmit_func(), handling all the details (no need for atomic ops
>> on skb users if packet is not delivered at all).
>>
>> By the way, your patch is not 100% safe/OK, because af_packet rcv()
>> handler writes on skb (skb_pull() and all)
>>
>
> But af_packet_rcv() restores skbs at last.
>
>        if (skb_head != skb->data && skb_shared(skb)) {
>                skb->data = skb_head;
>                skb->len = skb_len;
>        }
>
If af packet_rcv invokes skb_clone, this skb is differ from the original one.
Eric's warning is right.


-- 
--Junchang

Re: [PATCH] net: increase skb->users instead of skb_clone()

[Eric Dumazet]

Le jeudi 16 décembre 2010 à 22:20 +0800, Junchang Wang a écrit :
> On Thu, Dec 16, 2010 at 3:23 PM, Changli Gao <xiaosuo@gmail.com> wrote:
> 
> >> You beat me, but I was thinking of a different way, adding a new
> >> pt_prev->xmit_func(), handling all the details (no need for atomic ops
> >> on skb users if packet is not delivered at all).
> >>
> >> By the way, your patch is not 100% safe/OK, because af_packet rcv()
> >> handler writes on skb (skb_pull() and all)
> >>
> >
> > But af_packet_rcv() restores skbs at last.
> >
> >        if (skb_head != skb->data && skb_shared(skb)) {
> >                skb->data = skb_head;
> >                skb->len = skb_len;
> >        }
> >
> If af packet_rcv invokes skb_clone, this skb is differ from the original one.
> Eric's warning is right.

It was a false alarm.

If packet_rcv() invokes skb_clone(), skb still points to original skb.
No worry.

Re: [PATCH] net: increase skb->users instead of skb_clone()

[Junchang Wang]

On Thu, Dec 16, 2010 at 10:18 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
> Yes, and no.
>
> Consider the case you have one receiver.
>
> Packet given after Changli patch wont be shared, so packet_rcv wont
> clone it : Thats a win. Only one skb_clone() done instead of two.
>
> Consider case with 2 receivers :
>
> First time we call packet_rcv, packet is shared (because we call
> deliver_skb(), so packet_rcv clones it. Normal situation, we really need
> to clone it.

Got it. Thanks.

>
> Second time, we give a non shared packet : Thats a win over previous
> situation.
>
But, if we have N receivers, we get only the last one win - the first N-1 will
call deliver_skb().


Thanks.
-- 
--Junchang

Re: [PATCH] net: increase skb->users instead of skb_clone()

[Junchang Wang]

On Thu, Dec 16, 2010 at 10:12 PM, Changli Gao <xiaosuo@gmail.com> wrote:

>
> This happens when run_filter returns non zero. For your case, only
> small parts of packets match bpf filter.
Hi Changli,
In most cases, I want user-space applications see everything. :)

Thanks.
-- 
--Junchang

Re: [PATCH] net: increase skb->users instead of skb_clone()

[Eric Dumazet]

Le jeudi 16 décembre 2010 à 22:31 +0800, Junchang Wang a écrit :
> On Thu, Dec 16, 2010 at 10:18 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> >
> > Yes, and no.
> >
> > Consider the case you have one receiver.
> >
> > Packet given after Changli patch wont be shared, so packet_rcv wont
> > clone it : Thats a win. Only one skb_clone() done instead of two.
> >
> > Consider case with 2 receivers :
> >
> > First time we call packet_rcv, packet is shared (because we call
> > deliver_skb(), so packet_rcv clones it. Normal situation, we really need
> > to clone it.
> 
> Got it. Thanks.
> 
> >
> > Second time, we give a non shared packet : Thats a win over previous
> > situation.
> >
> But, if we have N receivers, we get only the last one win - the first N-1 will
> call deliver_skb().
> 

Yes, but you want to, because each receiver has to make a private copy
of the skb.

The big win is that if packet if filtered out (not accepted by the
socket filter), you end with no extra skb_clone() at all.

Say you have 8 receivers, with a filter matching some hash/cpu, and only
one af_packet socket will take the message.

Before patch : 8 skb_clones()

After patch : one skb_clone()

If I undertood patch intent ;)

Re: [PATCH] net: increase skb->users instead of skb_clone()

[Changli Gao]

On Thu, Dec 16, 2010 at 10:41 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
> Yes, but you want to, because each receiver has to make a private copy
> of the skb.
>
> The big win is that if packet if filtered out (not accepted by the
> socket filter), you end with no extra skb_clone() at all.
>
> Say you have 8 receivers, with a filter matching some hash/cpu, and only
> one af_packet socket will take the message.
>
> Before patch : 8 skb_clones()
>
> After patch : one skb_clone()
>
> If I undertood patch intent ;)
>

Yes, you got it. :)

-- 
Regards,
Changli Gao(xiaosuo@gmail.com)

Re: [PATCH] net: increase skb->users instead of skb_clone()

[Junchang Wang]

On Thu, Dec 16, 2010 at 10:41 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> But, if we have N receivers, we get only the last one win - the first N-1 will
>> call deliver_skb().
>>
>
> Yes, but you want to, because each receiver has to make a private copy
> of the skb.
>
> The big win is that if packet if filtered out (not accepted by the
> socket filter), you end with no extra skb_clone() at all.
>
> Say you have 8 receivers, with a filter matching some hash/cpu, and only
> one af_packet socket will take the message.
>
> Before patch : 8 skb_clones()
>
> After patch : one skb_clone()
>
Now I understand. :)
The patch is fine. Thanks Changli and Eric.



-- 
--Junchang

Re: [PATCH] net: increase skb->users instead of skb_clone()

[David Miller]

From: Changli Gao <xiaosuo@gmail.com>
Date: Thu, 16 Dec 2010 13:57:25 +0800

> In dev_queue_xmit_nit(), we have to clone skbs as we need to mangle skbs,
> however, we don't need to clone skbs for all the packet_types.
> 
> Except for the first packet_type, we increase skb->users instead of
> skb_clone().
> 
> Signed-off-by: Changli Gao <xiaosuo@gmail.com>

Applied, thanks.