| Message ID | 1318931232.16132.65.camel@zakaz.uk.xensource.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
Ian Campbell <Ian.Campbell@citrix.com> wrote on 10/18/2011 03:17:11 PM: > > I think the best thing might be to remove the additional ref taking from > > the setter function and audit the previous changes to ensure they > > conform. I'll do that right away and post a fixup patch ASAP. > > Sigh, only one out of the ten callers of (__)skb_frag_set_page expects > skb_frag_set_page to take a new reference. I think that's pretty > comprehensive evidence that the current behaviour is unexpected and > wrong. Looks good! Does it make sense to commit both of these patches? The reason being - my patch becomes a cleanup of set_skb_frag() in virtio_net driver. thanks, - KK -- 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 Tue, 2011-10-18 at 11:46 +0100, Krishna Kumar2 wrote: > Ian Campbell <Ian.Campbell@citrix.com> wrote on 10/18/2011 03:17:11 PM: > > > > I think the best thing might be to remove the additional ref taking > from > > > the setter function and audit the previous changes to ensure they > > > conform. I'll do that right away and post a fixup patch ASAP. > > > > Sigh, only one out of the ten callers of (__)skb_frag_set_page expects > > skb_frag_set_page to take a new reference. I think that's pretty > > comprehensive evidence that the current behaviour is unexpected and > > wrong. > > Looks good! Thanks. > Does it make sense to commit both of these patches? The > reason being - my patch becomes a cleanup of set_skb_frag() > in virtio_net driver. I think your patch remains a valid cleanup but I don't maintain that code. Ian. -- 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
Le mardi 18 octobre 2011 à 10:47 +0100, Ian Campbell a écrit : > On Tue, 2011-10-18 at 09:36 +0100, Ian Campbell wrote: > > I think the best thing might be to remove the additional ref taking from > > the setter function and audit the previous changes to ensure they > > conform. I'll do that right away and post a fixup patch ASAP. > > Sigh, only one out of the ten callers of (__)skb_frag_set_page expects > skb_frag_set_page to take a new reference. I think that's pretty > comprehensive evidence that the current behaviour is unexpected and > wrong. > > Sorry about this. > > Ian. > > 8<--------------------------------------------------------- > > From 42c26b7ca640bd5cb6f9c3bc76db96c92ed8ff82 Mon Sep 17 00:00:00 2001 > From: Ian Campbell <ian.campbell@citrix.com> > Date: Tue, 18 Oct 2011 09:59:37 +0100 > Subject: [PATCH] net: do not take an additional reference in skb_frag_set_page > > I audited all of the callers in the tree and only one of them (pktgen) expects > it to do so. Taking this reference is pretty obviously confusing and error > prone. > > In particular I looked at the following commits which switched callers of > (__)skb_frag_set_page to the skb paged fragment api: > > 6a930b9f163d7e6d9ef692e05616c4ede65038ec cxgb3: convert to SKB paged frag API. > 5dc3e196ea21e833128d51eb5b788a070fea1f28 myri10ge: convert to SKB paged frag API. > 0e0634d20dd670a89af19af2a686a6cce943ac14 vmxnet3: convert to SKB paged frag API. > 86ee8130a46769f73f8f423f99dbf782a09f9233 virtionet: convert to SKB paged frag API. > 4a22c4c919c201c2a7f4ee09e672435a3072d875 sfc: convert to SKB paged frag API. > 18324d690d6a5028e3c174fc1921447aedead2b8 cassini: convert to SKB paged frag API. > b061b39e3ae18ad75466258cf2116e18fa5bbd80 benet: convert to SKB paged frag API. > b7b6a688d217936459ff5cf1087b2361db952509 bnx2: convert to SKB paged frag API. > 804cf14ea5ceca46554d5801e2817bba8116b7e5 net: xfrm: convert to SKB frag APIs > ea2ab69379a941c6f8884e290fdd28c93936a778 net: convert core to skb paged frag APIs > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > --- > include/linux/skbuff.h | 1 - > net/core/pktgen.c | 1 + > 2 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h > index 64f8695..78741da 100644 > --- a/include/linux/skbuff.h > +++ b/include/linux/skbuff.h > @@ -1765,7 +1765,6 @@ static inline void *skb_frag_address_safe(const skb_frag_t *frag) > static inline void __skb_frag_set_page(skb_frag_t *frag, struct page *page) > { > frag->page = page; > - __skb_frag_ref(frag); > } > > /** > diff --git a/net/core/pktgen.c b/net/core/pktgen.c > index 796044a..c4effd4 100644 > --- a/net/core/pktgen.c > +++ b/net/core/pktgen.c > @@ -2603,6 +2603,7 @@ static void pktgen_finalize_skb(struct pktgen_dev *pkt_dev, struct sk_buff *skb, > break; > } > skb_frag_set_page(skb, i, pkt_dev->page); > + skb_frag_ref(skb, i); > skb_shinfo(skb)->frags[i].page_offset = 0; > /*last fragment, fill rest of data*/ > if (i == (frags - 1)) I am ok with this patch, since it makes sense to let driver do the page->_count change itself (it can use a batched add() in case a page is splitted in many frags) But I suggest using get_page(pkt_dev->page), this seems more obvious to me [ This was how I wrote the thing ;) ] -- 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
Ian Campbell <Ian.Campbell@citrix.com> wrote on 10/18/2011 04:20:55 PM: > > > Sigh, only one out of the ten callers of (__)skb_frag_set_page expects > > > skb_frag_set_page to take a new reference. I think that's pretty > > > comprehensive evidence that the current behaviour is unexpected and > > > wrong. > > > > Looks good! > > Thanks. Tested this patch for virtio_net - hang/panic is fixed. MemFree also doesn't reduce at end of test compared to the start. Tested-by: krkumar2@in.ibm.com - KK > > Does it make sense to commit both of these patches? The > > reason being - my patch becomes a cleanup of set_skb_frag() > > in virtio_net driver. > > I think your patch remains a valid cleanup but I don't maintain that > code. > > Ian. > > -- 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
Krishna could you please respin your original patch, fixing the subject and adjusting the commit message to indicate this is a cleanup. Assume I have applied Ian's patch, because that's what I'm about to do. Thanks! -- 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 64f8695..78741da 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -1765,7 +1765,6 @@ static inline void *skb_frag_address_safe(const skb_frag_t *frag) static inline void __skb_frag_set_page(skb_frag_t *frag, struct page *page) { frag->page = page; - __skb_frag_ref(frag); } /** diff --git a/net/core/pktgen.c b/net/core/pktgen.c index 796044a..c4effd4 100644 --- a/net/core/pktgen.c +++ b/net/core/pktgen.c @@ -2603,6 +2603,7 @@ static void pktgen_finalize_skb(struct pktgen_dev *pkt_dev, struct sk_buff *skb, break; } skb_frag_set_page(skb, i, pkt_dev->page); + skb_frag_ref(skb, i); skb_shinfo(skb)->frags[i].page_offset = 0; /*last fragment, fill rest of data*/ if (i == (frags - 1))