Patchwork net: Fix GRO for multiple page fragments

login
register
mail settings
Submitter Ben Hutchings
Date April 16, 2009, 6:04 p.m.
Message ID <1239905060.3203.28.camel@achroite>
Download mbox | patch
Permalink /patch/26081/
State Accepted
Delegated to: David Miller
Headers show

Comments

Ben Hutchings - April 16, 2009, 6:04 p.m.
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(-)
David Miller - April 17, 2009, 8:27 a.m.
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
Herbert Xu - April 17, 2009, 9:14 a.m.
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,
Ben Hutchings - April 17, 2009, 2:01 p.m.
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.
Ben Hutchings - April 17, 2009, 2:03 p.m.
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.
Herbert Xu - April 17, 2009, 2:48 p.m.
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,
David Miller - April 20, 2009, 9:24 a.m.
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

Patch

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++;