[PATCH] net: Fix GRO for multiple page fragments

[PATCH] net: Fix GRO for multiple page fragments

[Ben Hutchings]

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(-)

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.

Re: [PATCH] net: Fix GRO for multiple page fragments

[David Miller]

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.
> 

Re: [PATCH] net: Fix GRO for multiple page fragments

[Herbert Xu]

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,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] net: Fix GRO for multiple page fragments

[Ben Hutchings]

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.

-- 
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.

Re: [PATCH] net: Fix GRO for multiple page fragments

[Herbert Xu]

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,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] net: Fix GRO for multiple page fragments

[David Miller]

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.

Re: [PATCH] net: Fix GRO for multiple page fragments

[Ben Hutchings]

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, 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.