* Is it a possible bug in dev_gro_receive()? @ 2010-07-30 1:54 Xin Xiaohui 2010-08-02 10:29 ` Jarek Poplawski 0 siblings, 1 reply; 9+ messages in thread From: Xin Xiaohui @ 2010-07-30 1:54 UTC (permalink / raw) To: netdev, herbert, davem; +Cc: Xin Xiaohui I looked into the code dev_gro_receive(), found the code here: if the frags[0] is pulled to 0, then the page will be released, and memmove() frags left. Is that right? I'm not sure if memmove do right or not, but frags[0].size is never set after memove at least. what I think a simple way is not to do anything if we found frags[0].size == 0. The patch is as followed. Or am I missing something here? --- net/core/dev.c | 7 ------- 1 files changed, 0 insertions(+), 7 deletions(-) diff --git a/net/core/dev.c b/net/core/dev.c index 264137f..28cdbbf 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -2730,13 +2730,6 @@ pull: skb_shinfo(skb)->frags[0].page_offset += grow; skb_shinfo(skb)->frags[0].size -= grow; - - if (unlikely(!skb_shinfo(skb)->frags[0].size)) { - put_page(skb_shinfo(skb)->frags[0].page); - memmove(skb_shinfo(skb)->frags, - skb_shinfo(skb)->frags + 1, - --skb_shinfo(skb)->nr_frags); - } } ok: -- 1.5.4.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: Is it a possible bug in dev_gro_receive()? 2010-07-30 1:54 Is it a possible bug in dev_gro_receive()? Xin Xiaohui @ 2010-08-02 10:29 ` Jarek Poplawski 2010-08-02 11:04 ` Herbert Xu 2010-08-03 2:33 ` Xin, Xiaohui 0 siblings, 2 replies; 9+ messages in thread From: Jarek Poplawski @ 2010-08-02 10:29 UTC (permalink / raw) To: Xin Xiaohui; +Cc: netdev, herbert, davem Xin Xiaohui wrote: > I looked into the code dev_gro_receive(), found the code here: > if the frags[0] is pulled to 0, then the page will be released, > and memmove() frags left. > Is that right? I'm not sure if memmove do right or not, but > frags[0].size is never set after memove at least. what I think > a simple way is not to do anything if we found frags[0].size == 0. > The patch is as followed. > > Or am I missing something here? I think, you're right, but fixing memmove looks nicer to me: - --skb_shinfo(skb)->nr_frags); + --skb_shinfo(skb)->nr_frags * sizeof(skb_frag_t)); Jarek P. > > --- > net/core/dev.c | 7 ------- > 1 files changed, 0 insertions(+), 7 deletions(-) > > diff --git a/net/core/dev.c b/net/core/dev.c > index 264137f..28cdbbf 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -2730,13 +2730,6 @@ pull: > > skb_shinfo(skb)->frags[0].page_offset += grow; > skb_shinfo(skb)->frags[0].size -= grow; > - > - if (unlikely(!skb_shinfo(skb)->frags[0].size)) { > - put_page(skb_shinfo(skb)->frags[0].page); > - memmove(skb_shinfo(skb)->frags, > - skb_shinfo(skb)->frags + 1, > - --skb_shinfo(skb)->nr_frags); > - } > } > > ok: ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Is it a possible bug in dev_gro_receive()? 2010-08-02 10:29 ` Jarek Poplawski @ 2010-08-02 11:04 ` Herbert Xu 2010-08-03 2:33 ` Xin, Xiaohui 1 sibling, 0 replies; 9+ messages in thread From: Herbert Xu @ 2010-08-02 11:04 UTC (permalink / raw) To: Jarek Poplawski; +Cc: Xin Xiaohui, netdev, davem On Mon, Aug 02, 2010 at 10:29:06AM +0000, Jarek Poplawski wrote: > Xin Xiaohui wrote: > > I looked into the code dev_gro_receive(), found the code here: > > if the frags[0] is pulled to 0, then the page will be released, > > and memmove() frags left. > > Is that right? I'm not sure if memmove do right or not, but > > frags[0].size is never set after memove at least. what I think > > a simple way is not to do anything if we found frags[0].size == 0. > > The patch is as followed. > > > > Or am I missing something here? > > I think, you're right, but fixing memmove looks nicer to me: > > - --skb_shinfo(skb)->nr_frags); > + --skb_shinfo(skb)->nr_frags * sizeof(skb_frag_t)); I agree with the diagnosis and your proposed fix. Thanks for catching this Xiaohui! Cheers, -- Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: Is it a possible bug in dev_gro_receive()? 2010-08-02 10:29 ` Jarek Poplawski 2010-08-02 11:04 ` Herbert Xu @ 2010-08-03 2:33 ` Xin, Xiaohui 2010-08-03 6:45 ` Jarek Poplawski 1 sibling, 1 reply; 9+ messages in thread From: Xin, Xiaohui @ 2010-08-03 2:33 UTC (permalink / raw) To: Jarek Poplawski Cc: netdev@vger.kernel.org, herbert@gondor.apana.org.au, davem@davemloft.net >-----Original Message----- >From: Jarek Poplawski [mailto:jarkao2@gmail.com] >Sent: Monday, August 02, 2010 6:29 PM >To: Xin, Xiaohui >Cc: netdev@vger.kernel.org; herbert@gondor.apana.org.au; davem@davemloft.net >Subject: Re: Is it a possible bug in dev_gro_receive()? > >Xin Xiaohui wrote: >> I looked into the code dev_gro_receive(), found the code here: >> if the frags[0] is pulled to 0, then the page will be released, >> and memmove() frags left. >> Is that right? I'm not sure if memmove do right or not, but >> frags[0].size is never set after memove at least. what I think >> a simple way is not to do anything if we found frags[0].size == 0. >> The patch is as followed. >> >> Or am I missing something here? > >I think, you're right, but fixing memmove looks nicer to me: > > - --skb_shinfo(skb)->nr_frags); > + --skb_shinfo(skb)->nr_frags * sizeof(skb_frag_t)); > >Jarek P. Is there a little hurt of performance to do memmove() if skb_shinfo(skb)->nr_frags is large? We're now working on the zero-copy patches based on napi_gro_frags() interface, and in this case, we have found a lot of skbs which frags[0] is pulled to 0. And after the memmove is fixed, each frags[x].size is needed to modify too. So I think don't do anything is better. Or is there any side effect with a null page in the stack? Thanks Xiaohui > >> >> --- >> net/core/dev.c | 7 ------- >> 1 files changed, 0 insertions(+), 7 deletions(-) >> >> diff --git a/net/core/dev.c b/net/core/dev.c >> index 264137f..28cdbbf 100644 >> --- a/net/core/dev.c >> +++ b/net/core/dev.c >> @@ -2730,13 +2730,6 @@ pull: >> >> skb_shinfo(skb)->frags[0].page_offset += grow; >> skb_shinfo(skb)->frags[0].size -= grow; >> - >> - if (unlikely(!skb_shinfo(skb)->frags[0].size)) { >> - put_page(skb_shinfo(skb)->frags[0].page); >> - memmove(skb_shinfo(skb)->frags, >> - skb_shinfo(skb)->frags + 1, >> - --skb_shinfo(skb)->nr_frags); >> - } >> } >> >> ok: > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Is it a possible bug in dev_gro_receive()? 2010-08-03 2:33 ` Xin, Xiaohui @ 2010-08-03 6:45 ` Jarek Poplawski 2010-08-10 8:11 ` Xin, Xiaohui 0 siblings, 1 reply; 9+ messages in thread From: Jarek Poplawski @ 2010-08-03 6:45 UTC (permalink / raw) To: Xin, Xiaohui Cc: netdev@vger.kernel.org, herbert@gondor.apana.org.au, davem@davemloft.net On Tue, Aug 03, 2010 at 10:33:24AM +0800, Xin, Xiaohui wrote: > >-----Original Message----- > >From: Jarek Poplawski [mailto:jarkao2@gmail.com] > >Sent: Monday, August 02, 2010 6:29 PM > >To: Xin, Xiaohui > >Cc: netdev@vger.kernel.org; herbert@gondor.apana.org.au; davem@davemloft.net > >Subject: Re: Is it a possible bug in dev_gro_receive()? > > > >Xin Xiaohui wrote: > >> I looked into the code dev_gro_receive(), found the code here: > >> if the frags[0] is pulled to 0, then the page will be released, > >> and memmove() frags left. > >> Is that right? I'm not sure if memmove do right or not, but > >> frags[0].size is never set after memove at least. what I think > >> a simple way is not to do anything if we found frags[0].size == 0. > >> The patch is as followed. > >> > >> Or am I missing something here? > > > >I think, you're right, but fixing memmove looks nicer to me: > > > > - --skb_shinfo(skb)->nr_frags); > > + --skb_shinfo(skb)->nr_frags * sizeof(skb_frag_t)); > > > >Jarek P. > > Is there a little hurt of performance to do memmove() if skb_shinfo(skb)->nr_frags is large? > We're now working on the zero-copy patches based on napi_gro_frags() interface, and in > this case, we have found a lot of skbs which frags[0] is pulled to 0. And after the memmove is > fixed, each frags[x].size is needed to modify too. > So I think don't do anything is better. Or is there any side effect with a null page in the stack? Even if it's better, generally you should separate fixes from optimizations. On the other hand, it was expected to be "unlikely" by design, so you should probably explain more why it has to be changed here too. Thanks, Jarek P. > > Thanks > Xiaohui > > > >> > >> --- > >> net/core/dev.c | 7 ------- > >> 1 files changed, 0 insertions(+), 7 deletions(-) > >> > >> diff --git a/net/core/dev.c b/net/core/dev.c > >> index 264137f..28cdbbf 100644 > >> --- a/net/core/dev.c > >> +++ b/net/core/dev.c > >> @@ -2730,13 +2730,6 @@ pull: > >> > >> skb_shinfo(skb)->frags[0].page_offset += grow; > >> skb_shinfo(skb)->frags[0].size -= grow; > >> - > >> - if (unlikely(!skb_shinfo(skb)->frags[0].size)) { > >> - put_page(skb_shinfo(skb)->frags[0].page); > >> - memmove(skb_shinfo(skb)->frags, > >> - skb_shinfo(skb)->frags + 1, > >> - --skb_shinfo(skb)->nr_frags); > >> - } > >> } > >> > >> ok: > > > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: Is it a possible bug in dev_gro_receive()? 2010-08-03 6:45 ` Jarek Poplawski @ 2010-08-10 8:11 ` Xin, Xiaohui 2010-08-10 8:34 ` Jarek Poplawski 0 siblings, 1 reply; 9+ messages in thread From: Xin, Xiaohui @ 2010-08-10 8:11 UTC (permalink / raw) To: Jarek Poplawski Cc: netdev@vger.kernel.org, herbert@gondor.apana.org.au, davem@davemloft.net Jarek, Seems community agree with your patch more. So may you send out your patch then? Thanks! Some of my related patches still need this fix. Thanks Xiaohui >-----Original Message----- >From: Jarek Poplawski [mailto:jarkao2@gmail.com] >Sent: Tuesday, August 03, 2010 2:45 PM >To: Xin, Xiaohui >Cc: netdev@vger.kernel.org; herbert@gondor.apana.org.au; davem@davemloft.net >Subject: Re: Is it a possible bug in dev_gro_receive()? > >On Tue, Aug 03, 2010 at 10:33:24AM +0800, Xin, Xiaohui wrote: >> >-----Original Message----- >> >From: Jarek Poplawski [mailto:jarkao2@gmail.com] >> >Sent: Monday, August 02, 2010 6:29 PM >> >To: Xin, Xiaohui >> >Cc: netdev@vger.kernel.org; herbert@gondor.apana.org.au; davem@davemloft.net >> >Subject: Re: Is it a possible bug in dev_gro_receive()? >> > >> >Xin Xiaohui wrote: >> >> I looked into the code dev_gro_receive(), found the code here: >> >> if the frags[0] is pulled to 0, then the page will be released, >> >> and memmove() frags left. >> >> Is that right? I'm not sure if memmove do right or not, but >> >> frags[0].size is never set after memove at least. what I think >> >> a simple way is not to do anything if we found frags[0].size == 0. >> >> The patch is as followed. >> >> >> >> Or am I missing something here? >> > >> >I think, you're right, but fixing memmove looks nicer to me: >> > >> > - --skb_shinfo(skb)->nr_frags); >> > + --skb_shinfo(skb)->nr_frags * sizeof(skb_frag_t)); >> > >> >Jarek P. >> >> Is there a little hurt of performance to do memmove() if skb_shinfo(skb)->nr_frags is >large? >> We're now working on the zero-copy patches based on napi_gro_frags() interface, and in >> this case, we have found a lot of skbs which frags[0] is pulled to 0. And after the memmove >is >> fixed, each frags[x].size is needed to modify too. >> So I think don't do anything is better. Or is there any side effect with a null page in the >stack? > >Even if it's better, generally you should separate fixes from >optimizations. On the other hand, it was expected to be "unlikely" by >design, so you should probably explain more why it has to be changed >here too. > >Thanks, >Jarek P. > >> >> Thanks >> Xiaohui >> > >> >> >> >> --- >> >> net/core/dev.c | 7 ------- >> >> 1 files changed, 0 insertions(+), 7 deletions(-) >> >> >> >> diff --git a/net/core/dev.c b/net/core/dev.c >> >> index 264137f..28cdbbf 100644 >> >> --- a/net/core/dev.c >> >> +++ b/net/core/dev.c >> >> @@ -2730,13 +2730,6 @@ pull: >> >> >> >> skb_shinfo(skb)->frags[0].page_offset += grow; >> >> skb_shinfo(skb)->frags[0].size -= grow; >> >> - >> >> - if (unlikely(!skb_shinfo(skb)->frags[0].size)) { >> >> - put_page(skb_shinfo(skb)->frags[0].page); >> >> - memmove(skb_shinfo(skb)->frags, >> >> - skb_shinfo(skb)->frags + 1, >> >> - --skb_shinfo(skb)->nr_frags); >> >> - } >> >> } >> >> >> >> ok: >> > >> > >> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Is it a possible bug in dev_gro_receive()? 2010-08-10 8:11 ` Xin, Xiaohui @ 2010-08-10 8:34 ` Jarek Poplawski 2010-08-11 12:02 ` [PATCH] net: Fix a memmove bug in dev_gro_receive() Jarek Poplawski 0 siblings, 1 reply; 9+ messages in thread From: Jarek Poplawski @ 2010-08-10 8:34 UTC (permalink / raw) To: Xin, Xiaohui Cc: netdev@vger.kernel.org, herbert@gondor.apana.org.au, davem@davemloft.net On Tue, Aug 10, 2010 at 04:11:54PM +0800, Xin, Xiaohui wrote: > Jarek, > Seems community agree with your patch more. > So may you send out your patch then? Thanks! > Some of my related patches still need this fix. Hmm... But there was no my patch. Only a tiny, cosmetical suggestion to your patch. I'd be glad if you add some credit or my "Acked-by", of course. But if you really have a big problem, e.g. you don't like my suggestion, please confirm. Thanks, Jarek P. > > Thanks > Xiaohui > > >-----Original Message----- > >From: Jarek Poplawski [mailto:jarkao2@gmail.com] > >Sent: Tuesday, August 03, 2010 2:45 PM > >To: Xin, Xiaohui > >Cc: netdev@vger.kernel.org; herbert@gondor.apana.org.au; davem@davemloft.net > >Subject: Re: Is it a possible bug in dev_gro_receive()? > > > >On Tue, Aug 03, 2010 at 10:33:24AM +0800, Xin, Xiaohui wrote: > >> >-----Original Message----- > >> >From: Jarek Poplawski [mailto:jarkao2@gmail.com] > >> >Sent: Monday, August 02, 2010 6:29 PM > >> >To: Xin, Xiaohui > >> >Cc: netdev@vger.kernel.org; herbert@gondor.apana.org.au; davem@davemloft.net > >> >Subject: Re: Is it a possible bug in dev_gro_receive()? > >> > > >> >Xin Xiaohui wrote: > >> >> I looked into the code dev_gro_receive(), found the code here: > >> >> if the frags[0] is pulled to 0, then the page will be released, > >> >> and memmove() frags left. > >> >> Is that right? I'm not sure if memmove do right or not, but > >> >> frags[0].size is never set after memove at least. what I think > >> >> a simple way is not to do anything if we found frags[0].size == 0. > >> >> The patch is as followed. > >> >> > >> >> Or am I missing something here? > >> > > >> >I think, you're right, but fixing memmove looks nicer to me: > >> > > >> > - --skb_shinfo(skb)->nr_frags); > >> > + --skb_shinfo(skb)->nr_frags * sizeof(skb_frag_t)); > >> > > >> >Jarek P. > >> > >> Is there a little hurt of performance to do memmove() if skb_shinfo(skb)->nr_frags is > >large? > >> We're now working on the zero-copy patches based on napi_gro_frags() interface, and in > >> this case, we have found a lot of skbs which frags[0] is pulled to 0. And after the memmove > >is > >> fixed, each frags[x].size is needed to modify too. > >> So I think don't do anything is better. Or is there any side effect with a null page in the > >stack? > > > >Even if it's better, generally you should separate fixes from > >optimizations. On the other hand, it was expected to be "unlikely" by > >design, so you should probably explain more why it has to be changed > >here too. > > > >Thanks, > >Jarek P. > > > >> > >> Thanks > >> Xiaohui > >> > > >> >> > >> >> --- > >> >> net/core/dev.c | 7 ------- > >> >> 1 files changed, 0 insertions(+), 7 deletions(-) > >> >> > >> >> diff --git a/net/core/dev.c b/net/core/dev.c > >> >> index 264137f..28cdbbf 100644 > >> >> --- a/net/core/dev.c > >> >> +++ b/net/core/dev.c > >> >> @@ -2730,13 +2730,6 @@ pull: > >> >> > >> >> skb_shinfo(skb)->frags[0].page_offset += grow; > >> >> skb_shinfo(skb)->frags[0].size -= grow; > >> >> - > >> >> - if (unlikely(!skb_shinfo(skb)->frags[0].size)) { > >> >> - put_page(skb_shinfo(skb)->frags[0].page); > >> >> - memmove(skb_shinfo(skb)->frags, > >> >> - skb_shinfo(skb)->frags + 1, > >> >> - --skb_shinfo(skb)->nr_frags); > >> >> - } > >> >> } > >> >> > >> >> ok: > >> > > >> > > >> ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] net: Fix a memmove bug in dev_gro_receive() 2010-08-10 8:34 ` Jarek Poplawski @ 2010-08-11 12:02 ` Jarek Poplawski 2010-08-18 0:37 ` David Miller 0 siblings, 1 reply; 9+ messages in thread From: Jarek Poplawski @ 2010-08-11 12:02 UTC (permalink / raw) To: Xin, Xiaohui Cc: netdev@vger.kernel.org, herbert@gondor.apana.org.au, davem@davemloft.net [was: Re: Is it a possible bug in dev_gro_receive()?] On Tue, Aug 10, 2010 at 08:34:26AM +0000, Jarek Poplawski wrote: > On Tue, Aug 10, 2010 at 04:11:54PM +0800, Xin, Xiaohui wrote: > > Jarek, > > Seems community agree with your patch more. > > So may you send out your patch then? Thanks! > > Some of my related patches still need this fix. > > Hmm... But there was no my patch. Only a tiny, cosmetical suggestion > to your patch. I'd be glad if you add some credit or my "Acked-by", > of course. But if you really have a big problem, e.g. you don't like > my suggestion, please confirm. Hmm#2... OK, it's probably something with my English, but since it seems to take too long, here it is. Xiaohui, I hope you'll send your "Signed-off-by" at least. Thanks, Jarek P. PS: I know, there is a bit too long line... ---------------------------> >Xin Xiaohui wrote: > I looked into the code dev_gro_receive(), found the code here: > if the frags[0] is pulled to 0, then the page will be released, > and memmove() frags left. > Is that right? I'm not sure if memmove do right or not, but > frags[0].size is never set after memove at least. what I think > a simple way is not to do anything if we found frags[0].size == 0. > The patch is as followed. ... This version of the patch fixes the bug directly in memmove. Reported-by: "Xin, Xiaohui" <xiaohui.xin@intel.com> Signed-off-by: Jarek Poplawski <jarkao2@gmail.com> --- diff --git a/net/core/dev.c b/net/core/dev.c index 1ae6543..3721fbb 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -3143,7 +3143,7 @@ pull: put_page(skb_shinfo(skb)->frags[0].page); memmove(skb_shinfo(skb)->frags, skb_shinfo(skb)->frags + 1, - --skb_shinfo(skb)->nr_frags); + --skb_shinfo(skb)->nr_frags * sizeof(skb_frag_t)); } } ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] net: Fix a memmove bug in dev_gro_receive() 2010-08-11 12:02 ` [PATCH] net: Fix a memmove bug in dev_gro_receive() Jarek Poplawski @ 2010-08-18 0:37 ` David Miller 0 siblings, 0 replies; 9+ messages in thread From: David Miller @ 2010-08-18 0:37 UTC (permalink / raw) To: jarkao2; +Cc: xiaohui.xin, netdev, herbert From: Jarek Poplawski <jarkao2@gmail.com> Date: Wed, 11 Aug 2010 12:02:10 +0000 >>Xin Xiaohui wrote: >> I looked into the code dev_gro_receive(), found the code here: >> if the frags[0] is pulled to 0, then the page will be released, >> and memmove() frags left. >> Is that right? I'm not sure if memmove do right or not, but >> frags[0].size is never set after memove at least. what I think >> a simple way is not to do anything if we found frags[0].size == 0. >> The patch is as followed. > ... > > This version of the patch fixes the bug directly in memmove. > > Reported-by: "Xin, Xiaohui" <xiaohui.xin@intel.com> > Signed-off-by: Jarek Poplawski <jarkao2@gmail.com> Applied thanks a lot Jarek. ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2010-08-18 0:37 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-07-30 1:54 Is it a possible bug in dev_gro_receive()? Xin Xiaohui 2010-08-02 10:29 ` Jarek Poplawski 2010-08-02 11:04 ` Herbert Xu 2010-08-03 2:33 ` Xin, Xiaohui 2010-08-03 6:45 ` Jarek Poplawski 2010-08-10 8:11 ` Xin, Xiaohui 2010-08-10 8:34 ` Jarek Poplawski 2010-08-11 12:02 ` [PATCH] net: Fix a memmove bug in dev_gro_receive() Jarek Poplawski 2010-08-18 0:37 ` David Miller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).