| Message ID | 1239905060.3203.28.camel@achroite |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Ben Hutchings <bhutchings@solarflare.com> Date: Thu, 16 Apr 2009 19:04:20 +0100 > This loop over fragments in napi_fraginfo_skb() was "interesting". > > Signed-off-by: Ben Hutchings <bhutchings@solarflare.com> > --- > This is not tested, as only cxgb3 will currently pass in multiple > fragments at the same time. skb_shinfo(skb)->nr_frags would already be > 0 but it makes no sense to rely on that. I hope I'm not missing some > subtlety... I think the code still isn't right after your changes. The intent seems to be to append frags from 'info' into the SKB, so that it works even if the SKB already has some frags. And being that cxgb3 is pretty well tested with GRO I'm suspicious even moreso :-) Herbert? > diff --git a/net/core/dev.c b/net/core/dev.c > index ea8eb22..15ecc51 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -2539,9 +2539,9 @@ struct sk_buff *napi_fraginfo_skb(struct napi_struct *napi, > } > > BUG_ON(info->nr_frags > MAX_SKB_FRAGS); > - frag = &info->frags[info->nr_frags - 1]; > + frag = info->frags; > > - for (i = skb_shinfo(skb)->nr_frags; i < info->nr_frags; i++) { > + for (i = 0; i < info->nr_frags; i++) { > skb_fill_page_desc(skb, i, frag->page, frag->page_offset, > frag->size); > frag++; > > -- > Ben Hutchings, Senior Software Engineer, Solarflare Communications > Not speaking for my employer; that's the marketing department's job. > They asked us to note that Solarflare product names are trademarked. > -- 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 Fri, Apr 17, 2009 at 01:27:59AM -0700, David Miller wrote: > > I think the code still isn't right after your changes. > > The intent seems to be to append frags from 'info' into the SKB, > so that it works even if the SKB already has some frags. > > And being that cxgb3 is pretty well tested with GRO I'm suspicious > even moreso :-) > > Herbert? Ah, I'd replied via private email to Ben. This whole loop no longer exists in net-next. As for 2.6.30, the existing loop should be correct. Cheers,
On Fri, 2009-04-17 at 01:27 -0700, David Miller wrote: > From: Ben Hutchings <bhutchings@solarflare.com> > Date: Thu, 16 Apr 2009 19:04:20 +0100 > > > This loop over fragments in napi_fraginfo_skb() was "interesting". > > > > Signed-off-by: Ben Hutchings <bhutchings@solarflare.com> > > --- > > This is not tested, as only cxgb3 will currently pass in multiple > > fragments at the same time. skb_shinfo(skb)->nr_frags would already be > > 0 but it makes no sense to rely on that. I hope I'm not missing some > > subtlety... > > I think the code still isn't right after your changes. > > The intent seems to be to append frags from 'info' into the SKB, > so that it works even if the SKB already has some frags. [...] This function is setting up a 'new' skb from the fragments it's given. To avoid allocation overhead it recycles skbs via napi->skb if they are merged (or otherwise discarded). There should be no data attached to the recycled skb. Ben.
On Fri, 2009-04-17 at 17:14 +0800, Herbert Xu wrote: > On Fri, Apr 17, 2009 at 01:27:59AM -0700, David Miller wrote: > > > > I think the code still isn't right after your changes. > > > > The intent seems to be to append frags from 'info' into the SKB, > > so that it works even if the SKB already has some frags. > > > > And being that cxgb3 is pretty well tested with GRO I'm suspicious > > even moreso :-) > > > > Herbert? > > Ah, I'd replied via private email to Ben. This whole loop no > longer exists in net-next. As for 2.6.30, the existing loop > should be correct. The loop is iterating forward, not backward, so why initialise frag to point to the last fragment? Ben.
On Fri, Apr 17, 2009 at 03:03:30PM +0100, Ben Hutchings wrote: > > The loop is iterating forward, not backward, so why initialise frag to > point to the last fragment? Ah yes, that's completely bogus. I suppose it only works because the current users only supply one frag under normal circumstanses. Yes we should integrate your patch for 2.6.30. For net-next the loop no longer exists so it should be fine. Thanks,
From: Herbert Xu <herbert@gondor.apana.org.au> Date: Fri, 17 Apr 2009 22:48:12 +0800 > On Fri, Apr 17, 2009 at 03:03:30PM +0100, Ben Hutchings wrote: >> >> The loop is iterating forward, not backward, so why initialise frag to >> point to the last fragment? > > Ah yes, that's completely bogus. I suppose it only works because > the current users only supply one frag under normal circumstanses. > > Yes we should integrate your patch for 2.6.30. For net-next the > loop no longer exists so it should be fine. I've applied Ben's patch. -- 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/net/core/dev.c b/net/core/dev.c index ea8eb22..15ecc51 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -2539,9 +2539,9 @@ struct sk_buff *napi_fraginfo_skb(struct napi_struct *napi, } BUG_ON(info->nr_frags > MAX_SKB_FRAGS); - frag = &info->frags[info->nr_frags - 1]; + frag = info->frags; - for (i = skb_shinfo(skb)->nr_frags; i < info->nr_frags; i++) { + for (i = 0; i < info->nr_frags; i++) { skb_fill_page_desc(skb, i, frag->page, frag->page_offset, frag->size); frag++;
This loop over fragments in napi_fraginfo_skb() was "interesting". Signed-off-by: Ben Hutchings <bhutchings@solarflare.com> --- This is not tested, as only cxgb3 will currently pass in multiple fragments at the same time. skb_shinfo(skb)->nr_frags would already be 0 but it makes no sense to rely on that. I hope I'm not missing some subtlety... Ben. net/core/dev.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)