[PATCH net-next v2 1/6] skbuff: Add skb_list_linearize()

[PATCH net-next v2 1/6] skbuff: Add skb_list_linearize()

[Pravin B Shelar]

similar to skb_linearize(), this API takes skb list as arg and
linearize it into one big skb. STT driver patch will use this.

Signed-off-by: Pravin B Shelar <pshelar@nicira.com>
---
Fixed according to comments from Eric.
---
 include/linux/skbuff.h |  2 ++
 net/core/skbuff.c      | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 85ab7d7..c9194c1 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -2532,6 +2532,8 @@ static inline int skb_linearize(struct sk_buff *skb)
 	return skb_is_nonlinear(skb) ? __skb_linearize(skb) : 0;
 }
 
+int skb_list_linearize(struct sk_buff *head, gfp_t gfp_mask);
+
 /**
  * skb_has_shared_frag - can any frag be overwritten
  * @skb: buffer to test
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 56db472..d6358a7 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -2329,6 +2329,40 @@ void skb_copy_and_csum_dev(const struct sk_buff *skb, u8 *to)
 }
 EXPORT_SYMBOL(skb_copy_and_csum_dev);
 
+int skb_list_linearize(struct sk_buff *head, gfp_t gfp_mask)
+{
+	struct sk_buff *skb;
+	int tlen = 0;
+	int err;
+
+	err = skb_linearize(head);
+	if (err)
+		return err;
+
+	skb = head->next;
+	while (skb) {
+		tlen += skb->len;
+		skb = skb->next;
+	}
+	err = pskb_expand_head(head, 0, tlen, gfp_mask);
+	if (err)
+		return err;
+
+	skb = head->next;
+	while (skb) {
+		err = skb_copy_bits(skb, 0, skb_tail_pointer(head), skb->len);
+		if (err)
+			return err;
+		head->tail += skb->len;
+		skb = skb->next;
+	}
+	kfree_skb_list(head->next);
+	head->next = NULL;
+	head->len += tlen;
+	return 0;
+}
+EXPORT_SYMBOL(skb_list_linearize);
+
 /**
  *	skb_dequeue - remove from the head of the queue
  *	@list: list to dequeue from
-- 
1.9.1

Re: [PATCH net-next v2 1/6] skbuff: Add skb_list_linearize()

[Alexander Duyck]

On 01/29/2015 03:29 PM, Pravin B Shelar wrote:
> similar to skb_linearize(), this API takes skb list as arg and
> linearize it into one big skb. STT driver patch will use this.
>
> Signed-off-by: Pravin B Shelar <pshelar@nicira.com>
> ---
> Fixed according to comments from Eric.
> ---
>  include/linux/skbuff.h |  2 ++
>  net/core/skbuff.c      | 34 ++++++++++++++++++++++++++++++++++
>  2 files changed, 36 insertions(+)
>
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 85ab7d7..c9194c1 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -2532,6 +2532,8 @@ static inline int skb_linearize(struct sk_buff *skb)
>  	return skb_is_nonlinear(skb) ? __skb_linearize(skb) : 0;
>  }
>  
> +int skb_list_linearize(struct sk_buff *head, gfp_t gfp_mask);
> +
>  /**
>   * skb_has_shared_frag - can any frag be overwritten
>   * @skb: buffer to test
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 56db472..d6358a7 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -2329,6 +2329,40 @@ void skb_copy_and_csum_dev(const struct sk_buff *skb, u8 *to)
>  }
>  EXPORT_SYMBOL(skb_copy_and_csum_dev);
>  
> +int skb_list_linearize(struct sk_buff *head, gfp_t gfp_mask)
> +{
> +	struct sk_buff *skb;
> +	int tlen = 0;
> +	int err;
> +
> +	err = skb_linearize(head);
> +	if (err)
> +		return err;

What is the point in linearizing the head if you are just going to
reallocated it pskb_expand_head anyway?  It seems like you should
probably just be calling __pskb_pull_tail after your call
pskb_expand_head below.

> +	skb = head->next;
> +	while (skb) {
> +		tlen += skb->len;
> +		skb = skb->next;
> +	}
> +	err = pskb_expand_head(head, 0, tlen, gfp_mask);
> +	if (err)
> +		return err;
> +
> +	skb = head->next;
> +	while (skb) {
> +		err = skb_copy_bits(skb, 0, skb_tail_pointer(head), skb->len);
> +		if (err)
> +			return err;
> +		head->tail += skb->len;
> +		skb = skb->next;
> +	}
> +	kfree_skb_list(head->next);
> +	head->next = NULL;
> +	head->len += tlen;
> +	return 0;
> +}
> +EXPORT_SYMBOL(skb_list_linearize);
> +
>  /**
>   *	skb_dequeue - remove from the head of the queue
>   *	@list: list to dequeue from

I'm not completely convinced this is even necessary.  Seems like you are
wasting cycles copying the frames around when you could probably just
pull the header of the first frame and then use page frages to fill in
the rest.

- Alex

Re: [PATCH net-next v2 1/6] skbuff: Add skb_list_linearize()

[Pravin Shelar]

On Thu, Jan 29, 2015 at 7:28 PM, Alexander Duyck
<alexander.duyck@gmail.com> wrote:
> On 01/29/2015 03:29 PM, Pravin B Shelar wrote:
>> similar to skb_linearize(), this API takes skb list as arg and
>> linearize it into one big skb. STT driver patch will use this.
>>
>> Signed-off-by: Pravin B Shelar <pshelar@nicira.com>
>> ---
>> Fixed according to comments from Eric.
>> ---
>>  include/linux/skbuff.h |  2 ++
>>  net/core/skbuff.c      | 34 ++++++++++++++++++++++++++++++++++
>>  2 files changed, 36 insertions(+)
>>
>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>> index 85ab7d7..c9194c1 100644
>> --- a/include/linux/skbuff.h
>> +++ b/include/linux/skbuff.h
>> @@ -2532,6 +2532,8 @@ static inline int skb_linearize(struct sk_buff *skb)
>>       return skb_is_nonlinear(skb) ? __skb_linearize(skb) : 0;
>>  }
>>
>> +int skb_list_linearize(struct sk_buff *head, gfp_t gfp_mask);
>> +
>>  /**
>>   * skb_has_shared_frag - can any frag be overwritten
>>   * @skb: buffer to test
>> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
>> index 56db472..d6358a7 100644
>> --- a/net/core/skbuff.c
>> +++ b/net/core/skbuff.c
>> @@ -2329,6 +2329,40 @@ void skb_copy_and_csum_dev(const struct sk_buff *skb, u8 *to)
>>  }
>>  EXPORT_SYMBOL(skb_copy_and_csum_dev);
>>
>> +int skb_list_linearize(struct sk_buff *head, gfp_t gfp_mask)
>> +{
>> +     struct sk_buff *skb;
>> +     int tlen = 0;
>> +     int err;
>> +
>> +     err = skb_linearize(head);
>> +     if (err)
>> +             return err;
>
> What is the point in linearizing the head if you are just going to
> reallocated it pskb_expand_head anyway?  It seems like you should
> probably just be calling __pskb_pull_tail after your call
> pskb_expand_head below.
>
Sure.
But when linearization is involved most effective way to optimize is
to avoid linearization :) anyways I can improve current implementation
according to your comment.

>> +     skb = head->next;
>> +     while (skb) {
>> +             tlen += skb->len;
>> +             skb = skb->next;
>> +     }
>> +     err = pskb_expand_head(head, 0, tlen, gfp_mask);
>> +     if (err)
>> +             return err;
>> +
>> +     skb = head->next;
>> +     while (skb) {
>> +             err = skb_copy_bits(skb, 0, skb_tail_pointer(head), skb->len);
>> +             if (err)
>> +                     return err;
>> +             head->tail += skb->len;
>> +             skb = skb->next;
>> +     }
>> +     kfree_skb_list(head->next);
>> +     head->next = NULL;
>> +     head->len += tlen;
>> +     return 0;
>> +}
>> +EXPORT_SYMBOL(skb_list_linearize);
>> +
>>  /**
>>   *   skb_dequeue - remove from the head of the queue
>>   *   @list: list to dequeue from
>
> I'm not completely convinced this is even necessary.  Seems like you are
> wasting cycles copying the frames around when you could probably just
> pull the header of the first frame and then use page frages to fill in
> the rest.
>

This is what is done when possible. But skb merging is not always
possible, for example when  MAX_SKB_FRAGS limit is reached. You can
have a look at STT implementation.

RE: [PATCH net-next v2 1/6] skbuff: Add skb_list_linearize()

[David Laight]

From: Pravin Shelar
...
> > I'm not completely convinced this is even necessary.  Seems like you are
> > wasting cycles copying the frames around when you could probably just
> > pull the header of the first frame and then use page frages to fill in
> > the rest.
> >
> 
> This is what is done when possible. But skb merging is not always
> possible, for example when  MAX_SKB_FRAGS limit is reached. You can
> have a look at STT implementation.

If MAX_SKB_FRAGS is reached why not just generate two frames?
Except in pathological cases with a log of small fragments
the linearize is likely to be more expensive than the processing.

	David

Re: [PATCH net-next v2 1/6] skbuff: Add skb_list_linearize()

[Sergei Shtylyov]

Hello.

On 01/30/2015 02:29 AM, Pravin B Shelar wrote:

> similar to skb_linearize(), this API takes skb list as arg and
> linearize it into one big skb. STT driver patch will use this.

> Signed-off-by: Pravin B Shelar <pshelar@nicira.com>

[...]

> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 56db472..d6358a7 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -2329,6 +2329,40 @@ void skb_copy_and_csum_dev(const struct sk_buff *skb, u8 *to)
>   }
>   EXPORT_SYMBOL(skb_copy_and_csum_dev);
>
> +int skb_list_linearize(struct sk_buff *head, gfp_t gfp_mask)
> +{
> +	struct sk_buff *skb;
> +	int tlen = 0;
> +	int err;
> +
> +	err = skb_linearize(head);
> +	if (err)
> +		return err;
> +
> +	skb = head->next;
> +	while (skb) {
> +		tlen += skb->len;
> +		skb = skb->next;
> +	}

	for (skb = head->next; skb; skb = skb->next)
		tlen += skb->len;

> +	err = pskb_expand_head(head, 0, tlen, gfp_mask);
> +	if (err)
> +		return err;
> +
> +	skb = head->next;
> +	while (skb) {
> +		err = skb_copy_bits(skb, 0, skb_tail_pointer(head), skb->len);
> +		if (err)
> +			return err;
> +		head->tail += skb->len;
> +		skb = skb->next;
> +	}

    Likewise, this can easily be converted into a *for* loop.

[...]

WBR, Sergei