| Message ID | 20150515143506.GA5019@via.ecp.fr |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
On Fri, 2015-05-15 at 16:35 +0200, Sabrina Dubroca wrote: > Hi Eric, > > 2015-05-15, 05:48:07 -0700, Eric Dumazet wrote: > > From: Eric Dumazet <edumazet@google.com> > > > > First one in __skb_checksum_validate_complete() fixes the following > > (and other callers) > > > > make C=2 CF=-D__CHECK_ENDIAN__ net/ipv4/tcp_ipv4.o > > CHECK net/ipv4/tcp_ipv4.c > > include/linux/skbuff.h:3052:24: warning: incorrect type in return expression (different base types) > > include/linux/skbuff.h:3052:24: expected restricted __sum16 > > include/linux/skbuff.h:3052:24: got int > > > > Second is fixing gso_make_checksum() : > > > > CHECK net/ipv4/gre_offload.c > > include/linux/skbuff.h:3360:14: warning: incorrect type in assignment (different base types) > > include/linux/skbuff.h:3360:14: expected unsigned short [unsigned] [usertype] csum > > include/linux/skbuff.h:3360:14: got restricted __sum16 > > include/linux/skbuff.h:3365:16: warning: incorrect type in return expression (different base types) > > include/linux/skbuff.h:3365:16: expected restricted __sum16 > > include/linux/skbuff.h:3365:16: got unsigned short [unsigned] [usertype] csum > > > > > > Fixes: 5a21232983aa7 ("net: Support for csum_bad in skbuff") > > Fixes: 7e2b10c1e52ca ("net: Support for multiple checksums with gso") > > Signed-off-by: Eric Dumazet <edumazet@google.com> > > CC: Tom Herbert <tom@herbertland.com> > > --- > > That's very similar to what I submitted in February: > https://patchwork.ozlabs.org/patch/437332/ Well, given you did not address David concerns, and Tom apparently does not know how to use sparse, I finally gave up. -- 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
2015-05-15, 07:52:37 -0700, Eric Dumazet wrote: > On Fri, 2015-05-15 at 16:35 +0200, Sabrina Dubroca wrote: > > Hi Eric, > > > > 2015-05-15, 05:48:07 -0700, Eric Dumazet wrote: > > > From: Eric Dumazet <edumazet@google.com> > > > > > > First one in __skb_checksum_validate_complete() fixes the following > > > (and other callers) > > > > > > make C=2 CF=-D__CHECK_ENDIAN__ net/ipv4/tcp_ipv4.o > > > CHECK net/ipv4/tcp_ipv4.c > > > include/linux/skbuff.h:3052:24: warning: incorrect type in return expression (different base types) > > > include/linux/skbuff.h:3052:24: expected restricted __sum16 > > > include/linux/skbuff.h:3052:24: got int > > > > > > Second is fixing gso_make_checksum() : > > > > > > CHECK net/ipv4/gre_offload.c > > > include/linux/skbuff.h:3360:14: warning: incorrect type in assignment (different base types) > > > include/linux/skbuff.h:3360:14: expected unsigned short [unsigned] [usertype] csum > > > include/linux/skbuff.h:3360:14: got restricted __sum16 > > > include/linux/skbuff.h:3365:16: warning: incorrect type in return expression (different base types) > > > include/linux/skbuff.h:3365:16: expected restricted __sum16 > > > include/linux/skbuff.h:3365:16: got unsigned short [unsigned] [usertype] csum > > > > > > > > > Fixes: 5a21232983aa7 ("net: Support for csum_bad in skbuff") > > > Fixes: 7e2b10c1e52ca ("net: Support for multiple checksums with gso") > > > Signed-off-by: Eric Dumazet <edumazet@google.com> > > > CC: Tom Herbert <tom@herbertland.com> > > > --- > > > > That's very similar to what I submitted in February: > > https://patchwork.ozlabs.org/patch/437332/ > > Well, given you did not address David concerns, and Tom apparently does > not know how to use sparse, I finally gave up. Sorry :( But I don't see how doing the same thing for __skb_checksum_validate_complete will address David's concerns. I really have no problem with your submission, I just thought that it might not satisfy David. -- 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, May 15, 2015 at 10:35 AM, Sabrina Dubroca <sd@queasysnail.net> wrote: > Hi Eric, > > 2015-05-15, 05:48:07 -0700, Eric Dumazet wrote: >> From: Eric Dumazet <edumazet@google.com> >> >> First one in __skb_checksum_validate_complete() fixes the following >> (and other callers) >> >> make C=2 CF=-D__CHECK_ENDIAN__ net/ipv4/tcp_ipv4.o >> CHECK net/ipv4/tcp_ipv4.c >> include/linux/skbuff.h:3052:24: warning: incorrect type in return expression (different base types) >> include/linux/skbuff.h:3052:24: expected restricted __sum16 >> include/linux/skbuff.h:3052:24: got int >> >> Second is fixing gso_make_checksum() : >> >> CHECK net/ipv4/gre_offload.c >> include/linux/skbuff.h:3360:14: warning: incorrect type in assignment (different base types) >> include/linux/skbuff.h:3360:14: expected unsigned short [unsigned] [usertype] csum >> include/linux/skbuff.h:3360:14: got restricted __sum16 >> include/linux/skbuff.h:3365:16: warning: incorrect type in return expression (different base types) >> include/linux/skbuff.h:3365:16: expected restricted __sum16 >> include/linux/skbuff.h:3365:16: got unsigned short [unsigned] [usertype] csum >> >> >> Fixes: 5a21232983aa7 ("net: Support for csum_bad in skbuff") >> Fixes: 7e2b10c1e52ca ("net: Support for multiple checksums with gso") >> Signed-off-by: Eric Dumazet <edumazet@google.com> >> CC: Tom Herbert <tom@herbertland.com> >> --- > > That's very similar to what I submitted in February: > https://patchwork.ozlabs.org/patch/437332/ > > I have a patch to make __skb_checksum_validate_complete return a bool, > since that's all the callers care about (it also needs modifications > at some call sites, not included here). > > After that, maybe we should also reverse the logic so that "validate" > functions return true when the csum is valid. > > > > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h > index f83aa6568cbf..23dc1ccb28f0 100644 > --- a/include/linux/skbuff.h > +++ b/include/linux/skbuff.h > @@ -3034,24 +3034,23 @@ static inline void skb_checksum_complete_unset(struct sk_buff *skb) > /* Validate (init) checksum based on checksum complete. > * > * Return values: > - * 0: checksum is validated or try to in skb_checksum_complete. In the latter > - * case the ip_summed will not be CHECKSUM_UNNECESSARY and the pseudo > - * checksum is stored in skb->csum for use in __skb_checksum_complete > - * non-zero: value of invalid checksum > - * > + * false: checksum is validated or try to in skb_checksum_complete. In the > + * latter case the ip_summed will not be CHECKSUM_UNNECESSARY and the > + * pseudo checksum is stored in skb->csum for use in __skb_checksum_complete > + * true: invalid checksum > */ > -static inline __sum16 __skb_checksum_validate_complete(struct sk_buff *skb, > - bool complete, > - __wsum psum) > +static inline bool __skb_checksum_validate_complete(struct sk_buff *skb, > + bool complete, > + __wsum psum) > { > if (skb->ip_summed == CHECKSUM_COMPLETE) { > if (!csum_fold(csum_add(psum, skb->csum))) { > skb->csum_valid = 1; > - return 0; > + return false; > } > } else if (skb->csum_bad) { > /* ip_summed == CHECKSUM_NONE in this case */ > - return 1; > + return true; > } > > skb->csum = psum; > @@ -3061,10 +3060,10 @@ static inline __sum16 __skb_checksum_validate_complete(struct sk_buff *skb, > > csum = __skb_checksum_complete(skb); > skb->csum_valid = !csum; > - return csum; > + return !!csum; > } > > - return 0; > + return false; Hi Sabrina, We returned the checksum value since that may be of interest to the caller at some point. > } > > static inline __wsum null_compute_pseudo(struct sk_buff *skb, int proto) > @@ -3079,13 +3078,13 @@ static inline __wsum null_compute_pseudo(struct sk_buff *skb, int proto) > * pseudo header. > * > * Return values: > - * 0: checksum is validated or try to in skb_checksum_complete > - * non-zero: value of invalid checksum > + * false: checksum is validated or try to in skb_checksum_complete > + * true: invalid checksum > */ > #define __skb_checksum_validate(skb, proto, complete, \ > zero_okay, check, compute_pseudo) \ > ({ \ > - __sum16 __ret = 0; \ > + bool __ret = false; \ > skb->csum_valid = 0; \ > if (__skb_checksum_validate_needed(skb, zero_okay, check)) \ > __ret = __skb_checksum_validate_complete(skb, \ > > > > > -- > Sabrina -- 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
Hi Tom, 2015-05-15, 11:27:25 -0400, Tom Herbert wrote: > Hi Sabrina, > > We returned the checksum value since that may be of interest to the > caller at some point. Hmm, I thought "we may need that some day" was not a valid argument? Thanks,
On Fri, 2015-05-15 at 17:23 +0200, Sabrina Dubroca wrote: > Sorry :( No big deal, we are dealing with false positives here. > > But I don't see how doing the same thing for > __skb_checksum_validate_complete will address David's concerns. > > I really have no problem with your submission, I just thought that it > might not satisfy David. The real problem/bug is with the flow label thing. This needs a real fix. -- 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
From: Sabrina Dubroca <sd@queasysnail.net> Date: Fri, 15 May 2015 17:54:41 +0200 > Hi Tom, > > 2015-05-15, 11:27:25 -0400, Tom Herbert wrote: >> Hi Sabrina, >> >> We returned the checksum value since that may be of interest to the >> caller at some point. > > Hmm, I thought "we may need that some day" was not a valid argument? I think Tom's time frame for "some day" is really small and he should be accomodated. -- 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
2015-05-15, 12:19:42 -0400, David Miller wrote: > From: Sabrina Dubroca <sd@queasysnail.net> > Date: Fri, 15 May 2015 17:54:41 +0200 > > > Hi Tom, > > > > 2015-05-15, 11:27:25 -0400, Tom Herbert wrote: > >> Hi Sabrina, > >> > >> We returned the checksum value since that may be of interest to the > >> caller at some point. > > > > Hmm, I thought "we may need that some day" was not a valid argument? > > I think Tom's time frame for "some day" is really small and he > should be accomodated. Okay. Sorry again.
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index f83aa6568cbf..23dc1ccb28f0 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -3034,24 +3034,23 @@ static inline void skb_checksum_complete_unset(struct sk_buff *skb) /* Validate (init) checksum based on checksum complete. * * Return values: - * 0: checksum is validated or try to in skb_checksum_complete. In the latter - * case the ip_summed will not be CHECKSUM_UNNECESSARY and the pseudo - * checksum is stored in skb->csum for use in __skb_checksum_complete - * non-zero: value of invalid checksum - * + * false: checksum is validated or try to in skb_checksum_complete. In the + * latter case the ip_summed will not be CHECKSUM_UNNECESSARY and the + * pseudo checksum is stored in skb->csum for use in __skb_checksum_complete + * true: invalid checksum */ -static inline __sum16 __skb_checksum_validate_complete(struct sk_buff *skb, - bool complete, - __wsum psum) +static inline bool __skb_checksum_validate_complete(struct sk_buff *skb, + bool complete, + __wsum psum) { if (skb->ip_summed == CHECKSUM_COMPLETE) { if (!csum_fold(csum_add(psum, skb->csum))) { skb->csum_valid = 1; - return 0; + return false; } } else if (skb->csum_bad) { /* ip_summed == CHECKSUM_NONE in this case */ - return 1; + return true; } skb->csum = psum; @@ -3061,10 +3060,10 @@ static inline __sum16 __skb_checksum_validate_complete(struct sk_buff *skb, csum = __skb_checksum_complete(skb); skb->csum_valid = !csum; - return csum; + return !!csum; } - return 0; + return false; } static inline __wsum null_compute_pseudo(struct sk_buff *skb, int proto) @@ -3079,13 +3078,13 @@ static inline __wsum null_compute_pseudo(struct sk_buff *skb, int proto) * pseudo header. * * Return values: - * 0: checksum is validated or try to in skb_checksum_complete - * non-zero: value of invalid checksum + * false: checksum is validated or try to in skb_checksum_complete + * true: invalid checksum */ #define __skb_checksum_validate(skb, proto, complete, \ zero_okay, check, compute_pseudo) \ ({ \ - __sum16 __ret = 0; \ + bool __ret = false; \ skb->csum_valid = 0; \ if (__skb_checksum_validate_needed(skb, zero_okay, check)) \ __ret = __skb_checksum_validate_complete(skb, \