| Message ID | 1387539943-25521-1-git-send-email-zwu.kernel@gmail.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
Hello. On 20-12-2013 15:45, Zhi Yong Wu wrote: > From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> > When the truesize of all fragments was calculated, it didn't take > the first small piece of data into account. > Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> > --- > net/core/skbuff.c | 10 +++++++--- > 1 files changed, 7 insertions(+), 3 deletions(-) > diff --git a/net/core/skbuff.c b/net/core/skbuff.c > index 2b6b863..0665bd6 100644 > --- a/net/core/skbuff.c > +++ b/net/core/skbuff.c > @@ -3027,9 +3027,12 @@ int skb_gro_receive(struct sk_buff **head, struct sk_buff *skb) > frag->page_offset += offset; > skb_frag_size_sub(frag, offset); > > - /* all fragments truesize : remove (head size + sk_buff) */ > + /** Why kernel-doc style comment here? Networking code uses the following comment style: /* bla * bla */ > + * all fragments truesize : > + * remove (head size + sk_buff + offset) > + */ > delta_truesize = skb->truesize - > - SKB_TRUESIZE(skb_end_offset(skb)); > + SKB_TRUESIZE(skb_end_offset(skb) + offset); > > skb->truesize -= skb->data_len; > skb->len -= skb->data_len; WBR, Sergei -- 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, Dec 20, 2013 at 9:52 PM, Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> wrote: > Hello. > > > On 20-12-2013 15:45, Zhi Yong Wu wrote: > >> From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> > > >> When the truesize of all fragments was calculated, it didn't take >> the first small piece of data into account. > > >> Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> >> --- >> net/core/skbuff.c | 10 +++++++--- >> 1 files changed, 7 insertions(+), 3 deletions(-) > > >> diff --git a/net/core/skbuff.c b/net/core/skbuff.c >> index 2b6b863..0665bd6 100644 >> --- a/net/core/skbuff.c >> +++ b/net/core/skbuff.c >> @@ -3027,9 +3027,12 @@ int skb_gro_receive(struct sk_buff **head, struct >> sk_buff *skb) >> frag->page_offset += offset; >> skb_frag_size_sub(frag, offset); >> >> - /* all fragments truesize : remove (head size + sk_buff) >> */ >> + /** > > > Why kernel-doc style comment here? Networking code uses the following > comment style: > > /* bla > * bla > */ No, If you check other comments in networking code, you will see why i did as that. /** * skb_segment - Perform protocol segmentation on skb. * @skb: buffer to segment * @features: features for the output path (see dev->features) * * This function performs segmentation on the given skb. It returns * a pointer to the first in a list of new skbs for the segments. * In case of error it returns ERR_PTR(err). */ struct sk_buff *skb_segment > > >> + * all fragments truesize : >> + * remove (head size + sk_buff + offset) >> + */ >> delta_truesize = skb->truesize - >> - SKB_TRUESIZE(skb_end_offset(skb)); >> + SKB_TRUESIZE(skb_end_offset(skb) + >> offset); >> >> skb->truesize -= skb->data_len; >> skb->len -= skb->data_len; > > > WBR, Sergei Sorry, what is WBR? >
Hello. On 20-12-2013 17:59, Zhi Yong Wu wrote: >>> From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> >>> When the truesize of all fragments was calculated, it didn't take >>> the first small piece of data into account. >>> Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> >>> --- >>> net/core/skbuff.c | 10 +++++++--- >>> 1 files changed, 7 insertions(+), 3 deletions(-) >>> diff --git a/net/core/skbuff.c b/net/core/skbuff.c >>> index 2b6b863..0665bd6 100644 >>> --- a/net/core/skbuff.c >>> +++ b/net/core/skbuff.c >>> @@ -3027,9 +3027,12 @@ int skb_gro_receive(struct sk_buff **head, struct >>> sk_buff *skb) >>> frag->page_offset += offset; >>> skb_frag_size_sub(frag, offset); >>> >>> - /* all fragments truesize : remove (head size + sk_buff) >>> */ >>> + /** >> Why kernel-doc style comment here? Networking code uses the following >> comment style: >> /* bla >> * bla >> */ > No, If you check other comments in networking code, I suggest you do this, and not only comments before functions but also inside them. I also suggest that you read chapter 8 of Documentation/CodingStyle. > you will see why i > did as that. I repeat, this is kernel-doc style comment, that is used to comment on a functions/structures, and then can be automatically processed to create kernel documentation. Your case does not fit for /** comment. > /** > * skb_segment - Perform protocol segmentation on skb. > * @skb: buffer to segment > * @features: features for the output path (see dev->features) > * > * This function performs segmentation on the given skb. It returns > * a pointer to the first in a list of new skbs for the segments. > * In case of error it returns ERR_PTR(err). > */ > struct sk_buff *skb_segment >>> + * all fragments truesize : >>> + * remove (head size + sk_buff + offset) >>> + */ >>> delta_truesize = skb->truesize - >>> - SKB_TRUESIZE(skb_end_offset(skb)); >>> + SKB_TRUESIZE(skb_end_offset(skb) + >>> offset); >>> >>> skb->truesize -= skb->data_len; >>> skb->len -= skb->data_len; >> >> >> WBR, Sergei > Sorry, what is WBR? With best regards. Nothing offensive. :-) WBR, Sergei -- 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, 2013-12-20 at 19:45 +0800, Zhi Yong Wu wrote: > From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> > > When the truesize of all fragments was calculated, it didn't take > the first small piece of data into account. > > Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> > --- > net/core/skbuff.c | 10 +++++++--- > 1 files changed, 7 insertions(+), 3 deletions(-) > > diff --git a/net/core/skbuff.c b/net/core/skbuff.c > index 2b6b863..0665bd6 100644 > --- a/net/core/skbuff.c > +++ b/net/core/skbuff.c > @@ -3027,9 +3027,12 @@ int skb_gro_receive(struct sk_buff **head, struct sk_buff *skb) > frag->page_offset += offset; > skb_frag_size_sub(frag, offset); > > - /* all fragments truesize : remove (head size + sk_buff) */ > + /** > + * all fragments truesize : > + * remove (head size + sk_buff + offset) > + */ > delta_truesize = skb->truesize - > - SKB_TRUESIZE(skb_end_offset(skb)); > + SKB_TRUESIZE(skb_end_offset(skb) + offset); > > skb->truesize -= skb->data_len; > skb->len -= skb->data_len; > @@ -3060,7 +3063,8 @@ int skb_gro_receive(struct sk_buff **head, struct sk_buff *skb) > memcpy(frag + 1, skbinfo->frags, sizeof(*frag) * skbinfo->nr_frags); > /* We dont need to clear skbinfo->nr_frags here */ > > - delta_truesize = skb->truesize - SKB_DATA_ALIGN(sizeof(struct sk_buff)); > + delta_truesize = skb->truesize - > + SKB_DATA_ALIGN(sizeof(struct sk_buff)) - offset; > NAPI_GRO_CB(skb)->free = NAPI_GRO_FREE_STOLEN_HEAD; > goto done; > } Nope, this patch is not needed. truesize is about tracking the memory we allocated, not the memory we really use. What exact issue do you have ? I suspect a bug in a driver. -- 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, Dec 20, 2013 at 10:21 PM, Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> wrote: > Hello. > > > On 20-12-2013 17:59, Zhi Yong Wu wrote: > >>>> From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> > > >>>> When the truesize of all fragments was calculated, it didn't take >>>> the first small piece of data into account. > > >>>> Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> >>>> --- >>>> net/core/skbuff.c | 10 +++++++--- >>>> 1 files changed, 7 insertions(+), 3 deletions(-) > > >>>> diff --git a/net/core/skbuff.c b/net/core/skbuff.c >>>> index 2b6b863..0665bd6 100644 >>>> --- a/net/core/skbuff.c >>>> +++ b/net/core/skbuff.c >>>> @@ -3027,9 +3027,12 @@ int skb_gro_receive(struct sk_buff **head, struct >>>> sk_buff *skb) >>>> frag->page_offset += offset; >>>> skb_frag_size_sub(frag, offset); >>>> >>>> - /* all fragments truesize : remove (head size + sk_buff) >>>> */ >>>> + /** > > >>> Why kernel-doc style comment here? Networking code uses the following >>> comment style: > > >>> /* bla >>> * bla >>> */ > > >> No, If you check other comments in networking code, > > > I suggest you do this, and not only comments before functions but also > inside them. I also suggest that you read chapter 8 of > Documentation/CodingStyle. > > >> you will see why i >> did as that. > > > I repeat, this is kernel-doc style comment, that is used to comment on a > functions/structures, and then can be automatically processed to create > kernel documentation. Your case does not fit for /** comment. Your explanation is very clear, i accept your advice, thanks. > > >> /** >> * skb_segment - Perform protocol segmentation on skb. >> * @skb: buffer to segment >> * @features: features for the output path (see dev->features) >> * >> * This function performs segmentation on the given skb. It returns >> * a pointer to the first in a list of new skbs for the segments. >> * In case of error it returns ERR_PTR(err). >> */ >> struct sk_buff *skb_segment > > >>>> + * all fragments truesize : >>>> + * remove (head size + sk_buff + offset) >>>> + */ >>>> delta_truesize = skb->truesize - >>>> - SKB_TRUESIZE(skb_end_offset(skb)); >>>> + SKB_TRUESIZE(skb_end_offset(skb) + >>>> offset); >>>> >>>> skb->truesize -= skb->data_len; >>>> skb->len -= skb->data_len; >>> >>> >>> >>> WBR, Sergei > > >> Sorry, what is WBR? > > > With best regards. Nothing offensive. :-) > > WBR, Sergei >
On Fri, Dec 20, 2013 at 10:28 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > On Fri, 2013-12-20 at 19:45 +0800, Zhi Yong Wu wrote: >> From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> >> >> When the truesize of all fragments was calculated, it didn't take >> the first small piece of data into account. >> >> Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> >> --- >> net/core/skbuff.c | 10 +++++++--- >> 1 files changed, 7 insertions(+), 3 deletions(-) >> >> diff --git a/net/core/skbuff.c b/net/core/skbuff.c >> index 2b6b863..0665bd6 100644 >> --- a/net/core/skbuff.c >> +++ b/net/core/skbuff.c >> @@ -3027,9 +3027,12 @@ int skb_gro_receive(struct sk_buff **head, struct sk_buff *skb) >> frag->page_offset += offset; >> skb_frag_size_sub(frag, offset); >> >> - /* all fragments truesize : remove (head size + sk_buff) */ >> + /** >> + * all fragments truesize : >> + * remove (head size + sk_buff + offset) >> + */ >> delta_truesize = skb->truesize - >> - SKB_TRUESIZE(skb_end_offset(skb)); >> + SKB_TRUESIZE(skb_end_offset(skb) + offset); >> >> skb->truesize -= skb->data_len; >> skb->len -= skb->data_len; >> @@ -3060,7 +3063,8 @@ int skb_gro_receive(struct sk_buff **head, struct sk_buff *skb) >> memcpy(frag + 1, skbinfo->frags, sizeof(*frag) * skbinfo->nr_frags); >> /* We dont need to clear skbinfo->nr_frags here */ >> >> - delta_truesize = skb->truesize - SKB_DATA_ALIGN(sizeof(struct sk_buff)); >> + delta_truesize = skb->truesize - >> + SKB_DATA_ALIGN(sizeof(struct sk_buff)) - offset; >> NAPI_GRO_CB(skb)->free = NAPI_GRO_FREE_STOLEN_HEAD; >> goto done; >> } > > > Nope, this patch is not needed. > > truesize is about tracking the memory we allocated, not the memory we > really use. wow, i forgot this, thanks for yor reminder. > > What exact issue do you have ? I suspect a bug in a driver. I didn't hit any issue, but found this when i read gro related code. a but in a driver? sorry, i don't know, look forward to seeing your fix. > > >
On Fri, 2013-12-20 at 22:37 +0800, Zhi Yong Wu wrote: > On Fri, Dec 20, 2013 at 10:28 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > > Nope, this patch is not needed. > > > > truesize is about tracking the memory we allocated, not the memory we > > really use. > wow, i forgot this, thanks for yor reminder. > > > > What exact issue do you have ? I suspect a bug in a driver. > I didn't hit any issue, but found this when i read gro related code. a > but in a driver? sorry, i don't know, look forward to seeing your fix. No, I have no particular issue with this code, I thought you had a problem on a running machine. If you found this by code review, then everything is fine, and no patch is needed at this point. 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/net/core/skbuff.c b/net/core/skbuff.c index 2b6b863..0665bd6 100644 --- a/net/core/skbuff.c +++ b/net/core/skbuff.c @@ -3027,9 +3027,12 @@ int skb_gro_receive(struct sk_buff **head, struct sk_buff *skb) frag->page_offset += offset; skb_frag_size_sub(frag, offset); - /* all fragments truesize : remove (head size + sk_buff) */ + /** + * all fragments truesize : + * remove (head size + sk_buff + offset) + */ delta_truesize = skb->truesize - - SKB_TRUESIZE(skb_end_offset(skb)); + SKB_TRUESIZE(skb_end_offset(skb) + offset); skb->truesize -= skb->data_len; skb->len -= skb->data_len; @@ -3060,7 +3063,8 @@ int skb_gro_receive(struct sk_buff **head, struct sk_buff *skb) memcpy(frag + 1, skbinfo->frags, sizeof(*frag) * skbinfo->nr_frags); /* We dont need to clear skbinfo->nr_frags here */ - delta_truesize = skb->truesize - SKB_DATA_ALIGN(sizeof(struct sk_buff)); + delta_truesize = skb->truesize - + SKB_DATA_ALIGN(sizeof(struct sk_buff)) - offset; NAPI_GRO_CB(skb)->free = NAPI_GRO_FREE_STOLEN_HEAD; goto done; }