From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jarek Poplawski Subject: Re: [PATCH net-next-2.6] net: pskb_expand_head() optimization Date: Sat, 11 Sep 2010 14:31:40 +0200 Message-ID: <20100911123140.GA1939@del.dom.local> References: <20100907091614.GA8245@ff.dom.local> <1283852248.2338.160.camel@edumazet-laptop> <20100910.125449.235704956.davem@davemloft.net> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: eric.dumazet@gmail.com, netdev@vger.kernel.org To: David Miller Return-path: Received: from mail-bw0-f46.google.com ([209.85.214.46]:64713 "EHLO mail-bw0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751796Ab0IKMcD (ORCPT ); Sat, 11 Sep 2010 08:32:03 -0400 Received: by bwz11 with SMTP id 11so3117113bwz.19 for ; Sat, 11 Sep 2010 05:32:00 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20100910.125449.235704956.davem@davemloft.net> Sender: netdev-owner@vger.kernel.org List-ID: On Fri, Sep 10, 2010 at 12:54:49PM -0700, David Miller wrote: > From: Eric Dumazet > Date: Tue, 07 Sep 2010 11:37:28 +0200 >=20 > > Le mardi 07 septembre 2010 ? 09:16 +0000, Jarek Poplawski a =E9crit= : > >> On 2010-09-07 07:02, Eric Dumazet wrote: > >=20 > >> >=20 > >> > I understand what you want to do, but problem is we need to perf= orm a > >> > CAS2 operation : atomically changes two values (dataref and frag= _list) > >>=20 > >> Alas I can't understand why do you think these clone and atomic te= sts > >> in skb_release_data() don't protect skb_shinfo(skb)->frag_list eno= ugh. > >>=20 > >=20 > > It was early in the morning, before a cup of tea. > >=20 > > David only had to set frag_list in the new shinfo, not the old. >=20 > Ok, how does this look? >=20 > diff --git a/net/core/skbuff.c b/net/core/skbuff.c > index 752c197..aaa9750 100644 > --- a/net/core/skbuff.c > +++ b/net/core/skbuff.c > @@ -327,6 +327,32 @@ static void skb_clone_fraglist(struct sk_buff *s= kb) > skb_get(list); > } > =20 > +static struct sk_buff *skb_copy_fraglist(struct sk_buff *parent, > + gfp_t gfp_mask) > +{ > + struct sk_buff *first_skb =3D NULL; > + struct sk_buff *prev_skb =3D NULL; > + struct sk_buff *skb; > + > + skb_walk_frags(parent, skb) { > + struct sk_buff *nskb =3D pskb_copy(skb, gfp_mask); > + > + if (!nskb) > + goto fail; > + if (!first_skb) > + first_skb =3D skb; Probably here and below: "=3D nskb" > + else > + prev_skb->next =3D skb; > + prev_skb =3D skb; > + } > + > + return first_skb; > + > +fail: With "if (first_skb)" here it would look better to me even if it currently doesn't matter. Otherwise seems OK, but I still would like to know the scenario demanding this change. Jarek P. > + skb_drop_list(&first_skb); > + return NULL; > +} > + > static void skb_release_data(struct sk_buff *skb) > { > if (!skb->cloned || > @@ -775,11 +801,12 @@ EXPORT_SYMBOL(pskb_copy); > int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail, > gfp_t gfp_mask) > { > - int i; > - u8 *data; > int size =3D nhead + (skb_end_pointer(skb) - skb->head) + ntail; > - long off; > + struct skb_shared_info *new_shinfo; > bool fastpath; > + u8 *data; > + long off; > + int i; > =20 > BUG_ON(nhead < 0); > =20 > @@ -797,8 +824,8 @@ int pskb_expand_head(struct sk_buff *skb, int nhe= ad, int ntail, > */ > memcpy(data + nhead, skb->head, skb_tail_pointer(skb) - skb->head); > =20 > - memcpy((struct skb_shared_info *)(data + size), > - skb_shinfo(skb), > + new_shinfo =3D (struct skb_shared_info *)(data + size); > + memcpy(new_shinfo, skb_shinfo(skb), > offsetof(struct skb_shared_info, frags[skb_shinfo(skb)->nr_f= rags])); > =20 > /* Check if we can avoid taking references on fragments if we own > @@ -815,14 +842,20 @@ int pskb_expand_head(struct sk_buff *skb, int n= head, int ntail, > if (fastpath) { > kfree(skb->head); > } else { > + if (skb_has_frag_list(skb)) { > + struct sk_buff *new_list; > + > + new_list =3D skb_copy_fraglist(skb, gfp_mask); > + if (!new_list) > + goto free_data; > + new_shinfo->frag_list =3D new_list; > + } > for (i =3D 0; i < skb_shinfo(skb)->nr_frags; i++) > get_page(skb_shinfo(skb)->frags[i].page); > =20 > - if (skb_has_frag_list(skb)) > - skb_clone_fraglist(skb); > - > skb_release_data(skb); > } > + > off =3D (data + nhead) - skb->head; > =20 > skb->head =3D data; > @@ -848,6 +881,8 @@ int pskb_expand_head(struct sk_buff *skb, int nhe= ad, int ntail, > atomic_set(&skb_shinfo(skb)->dataref, 1); > return 0; > =20 > +free_data: > + kfree(data); > nodata: > return -ENOMEM; > }