Message ID | 1422574164-1860-1-git-send-email-pshelar@nicira.com |
---|---|
State | Rejected, archived |
Delegated to: | David Miller |
Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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(+)