* [dpdk-stable] [PATCH] net: fix unneeded replacement of 0 by ffff for TCP checksum @ 2020-07-10 6:55 Hongzhi Guo 2020-07-10 12:41 ` Olivier Matz 0 siblings, 1 reply; 10+ messages in thread From: Hongzhi Guo @ 2020-07-10 6:55 UTC (permalink / raw) To: dev Cc: stable, stephen, mb, thomas, olivier.matz, konstantin.ananyev, ferruh.yigit, nicolas.chautru, zhoujingbin, chenchanghu, jerry.lilijun, haifeng.lin, guohongzhi1 Per RFC768: If the computed checksum is zero, it is transmitted as all ones. An all zero transmitted checksum value means that the transmitter generated no checksum. RFC793 for TCP has no such special treatment for the checksum of zero. Fixes: 6006818cfb26 ("net: new checksum functions") Cc: stable@dpdk.org Signed-off-by: Hongzhi Guo <guohongzhi1@huawei.com> --- v2: * Fixed commit tile * Fixed the API comment --- --- lib/librte_net/rte_ip.h | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/lib/librte_net/rte_ip.h b/lib/librte_net/rte_ip.h index 292f63fd7..d03c77120 100644 --- a/lib/librte_net/rte_ip.h +++ b/lib/librte_net/rte_ip.h @@ -325,7 +325,7 @@ rte_ipv4_phdr_cksum(const struct rte_ipv4_hdr *ipv4_hdr, uint64_t ol_flags) * The pointer to the beginning of the L4 header. * @return * The complemented checksum to set in the IP packet - * or 0 on error + * or 0 if the IP length is invalid in the header. */ static inline uint16_t rte_ipv4_udptcp_cksum(const struct rte_ipv4_hdr *ipv4_hdr, const void *l4_hdr) @@ -344,7 +344,13 @@ rte_ipv4_udptcp_cksum(const struct rte_ipv4_hdr *ipv4_hdr, const void *l4_hdr) cksum = ((cksum & 0xffff0000) >> 16) + (cksum & 0xffff); cksum = (~cksum) & 0xffff; - if (cksum == 0) + /* + *Per RFC768: + *If the computed checksum is zero for udp, + *it is transmitted as all ones. + *(the equivalent in one's complement arithmetic). + */ + if (cksum == 0 && ipv4_hdr->next_proto_id == IPPROTO_UDP) cksum = 0xffff; return (uint16_t)cksum; @@ -438,7 +444,13 @@ rte_ipv6_udptcp_cksum(const struct rte_ipv6_hdr *ipv6_hdr, const void *l4_hdr) cksum = ((cksum & 0xffff0000) >> 16) + (cksum & 0xffff); cksum = (~cksum) & 0xffff; - if (cksum == 0) + /* + *Per RFC768: + *If the computed checksum is zero for udp, + *it is transmitted as all ones. + *(the equivalent in one's complement arithmetic). + */ + if (cksum == 0 && ipv6_hdr->proto == IPPROTO_UDP) cksum = 0xffff; return (uint16_t)cksum; -- 2.21.0.windows.1 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [dpdk-stable] [PATCH] net: fix unneeded replacement of 0 by ffff for TCP checksum 2020-07-10 6:55 [dpdk-stable] [PATCH] net: fix unneeded replacement of 0 by ffff for TCP checksum Hongzhi Guo @ 2020-07-10 12:41 ` Olivier Matz 2020-07-10 13:10 ` Morten Brørup 0 siblings, 1 reply; 10+ messages in thread From: Olivier Matz @ 2020-07-10 12:41 UTC (permalink / raw) To: Hongzhi Guo Cc: dev, stable, stephen, mb, thomas, konstantin.ananyev, ferruh.yigit, nicolas.chautru, zhoujingbin, chenchanghu, jerry.lilijun, haifeng.lin On Fri, Jul 10, 2020 at 02:55:51PM +0800, Hongzhi Guo wrote: > Per RFC768: > If the computed checksum is zero, it is transmitted as all ones. > An all zero transmitted checksum value means that the transmitter > generated no checksum. > > RFC793 for TCP has no such special treatment for the checksum of zero. > > Fixes: 6006818cfb26 ("net: new checksum functions") > Cc: stable@dpdk.org > > Signed-off-by: Hongzhi Guo <guohongzhi1@huawei.com> > --- > v2: > * Fixed commit tile > * Fixed the API comment > --- > --- > lib/librte_net/rte_ip.h | 18 +++++++++++++++--- > 1 file changed, 15 insertions(+), 3 deletions(-) > > diff --git a/lib/librte_net/rte_ip.h b/lib/librte_net/rte_ip.h > index 292f63fd7..d03c77120 100644 > --- a/lib/librte_net/rte_ip.h > +++ b/lib/librte_net/rte_ip.h > @@ -325,7 +325,7 @@ rte_ipv4_phdr_cksum(const struct rte_ipv4_hdr *ipv4_hdr, uint64_t ol_flags) > * The pointer to the beginning of the L4 header. > * @return > * The complemented checksum to set in the IP packet > - * or 0 on error > + * or 0 if the IP length is invalid in the header. > */ > static inline uint16_t > rte_ipv4_udptcp_cksum(const struct rte_ipv4_hdr *ipv4_hdr, const void *l4_hdr) > @@ -344,7 +344,13 @@ rte_ipv4_udptcp_cksum(const struct rte_ipv4_hdr *ipv4_hdr, const void *l4_hdr) > > cksum = ((cksum & 0xffff0000) >> 16) + (cksum & 0xffff); > cksum = (~cksum) & 0xffff; > - if (cksum == 0) > + /* > + *Per RFC768: > + *If the computed checksum is zero for udp, > + *it is transmitted as all ones. > + *(the equivalent in one's complement arithmetic). > + */ There should be a space after the '*', and maybe it could be on less lines. Thomas, maybe you can do it when applying? > + if (cksum == 0 && ipv4_hdr->next_proto_id == IPPROTO_UDP) > cksum = 0xffff; > > return (uint16_t)cksum; > @@ -438,7 +444,13 @@ rte_ipv6_udptcp_cksum(const struct rte_ipv6_hdr *ipv6_hdr, const void *l4_hdr) > > cksum = ((cksum & 0xffff0000) >> 16) + (cksum & 0xffff); > cksum = (~cksum) & 0xffff; > - if (cksum == 0) > + /* > + *Per RFC768: > + *If the computed checksum is zero for udp, > + *it is transmitted as all ones. > + *(the equivalent in one's complement arithmetic). > + */ Same here > + if (cksum == 0 && ipv6_hdr->proto == IPPROTO_UDP) > cksum = 0xffff; > > return (uint16_t)cksum; > -- > 2.21.0.windows.1 > > Acked-by: Olivier Matz <olivier.matz@6wind.com> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [dpdk-stable] [PATCH] net: fix unneeded replacement of 0 by ffff for TCP checksum 2020-07-10 12:41 ` Olivier Matz @ 2020-07-10 13:10 ` Morten Brørup 2020-07-10 13:16 ` Olivier Matz 0 siblings, 1 reply; 10+ messages in thread From: Morten Brørup @ 2020-07-10 13:10 UTC (permalink / raw) To: Olivier Matz, Hongzhi Guo Cc: dev, stable, stephen, thomas, konstantin.ananyev, ferruh.yigit, nicolas.chautru, zhoujingbin, chenchanghu, jerry.lilijun, haifeng.lin > From: Olivier Matz [mailto:olivier.matz@6wind.com] > Sent: Friday, July 10, 2020 2:41 PM > > On Fri, Jul 10, 2020 at 02:55:51PM +0800, Hongzhi Guo wrote: > > Per RFC768: > > If the computed checksum is zero, it is transmitted as all ones. > > An all zero transmitted checksum value means that the transmitter > > generated no checksum. > > > > RFC793 for TCP has no such special treatment for the checksum of > zero. > > > > Fixes: 6006818cfb26 ("net: new checksum functions") > > Cc: stable@dpdk.org > > > > Signed-off-by: Hongzhi Guo <guohongzhi1@huawei.com> > > --- > > v2: > > * Fixed commit tile > > * Fixed the API comment > > --- > > --- > > lib/librte_net/rte_ip.h | 18 +++++++++++++++--- > > 1 file changed, 15 insertions(+), 3 deletions(-) > > > > diff --git a/lib/librte_net/rte_ip.h b/lib/librte_net/rte_ip.h > > index 292f63fd7..d03c77120 100644 > > --- a/lib/librte_net/rte_ip.h > > +++ b/lib/librte_net/rte_ip.h > > @@ -325,7 +325,7 @@ rte_ipv4_phdr_cksum(const struct rte_ipv4_hdr > *ipv4_hdr, uint64_t ol_flags) > > * The pointer to the beginning of the L4 header. > > * @return > > * The complemented checksum to set in the IP packet > > - * or 0 on error > > + * or 0 if the IP length is invalid in the header. > > */ > > static inline uint16_t > > rte_ipv4_udptcp_cksum(const struct rte_ipv4_hdr *ipv4_hdr, const > void *l4_hdr) 0 is a valid return value, so I suggest omitting it from the return value description: * @return - * The complemented checksum to set in the IP packet - * or 0 on error + * The complemented checksum to set in the IP packet. The comparison "if (l3_len < sizeof(struct rte_ipv4_hdr))" is only there to protect against invalid input; it prevents l4_len from becoming negative. For the same reason, unlikely() should be added to this comparison. Otherwise, Acked-by: Morten Brørup <mb@smartsharesystems.com> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [dpdk-stable] [PATCH] net: fix unneeded replacement of 0 by ffff for TCP checksum 2020-07-10 13:10 ` Morten Brørup @ 2020-07-10 13:16 ` Olivier Matz 2020-07-10 13:29 ` Morten Brørup 0 siblings, 1 reply; 10+ messages in thread From: Olivier Matz @ 2020-07-10 13:16 UTC (permalink / raw) To: Morten Brørup Cc: Hongzhi Guo, dev, stable, stephen, thomas, konstantin.ananyev, ferruh.yigit, nicolas.chautru, zhoujingbin, chenchanghu, jerry.lilijun, haifeng.lin On Fri, Jul 10, 2020 at 03:10:34PM +0200, Morten Brørup wrote: > > From: Olivier Matz [mailto:olivier.matz@6wind.com] > > Sent: Friday, July 10, 2020 2:41 PM > > > > On Fri, Jul 10, 2020 at 02:55:51PM +0800, Hongzhi Guo wrote: > > > Per RFC768: > > > If the computed checksum is zero, it is transmitted as all ones. > > > An all zero transmitted checksum value means that the transmitter > > > generated no checksum. > > > > > > RFC793 for TCP has no such special treatment for the checksum of > > zero. > > > > > > Fixes: 6006818cfb26 ("net: new checksum functions") > > > Cc: stable@dpdk.org > > > > > > Signed-off-by: Hongzhi Guo <guohongzhi1@huawei.com> > > > --- > > > v2: > > > * Fixed commit tile > > > * Fixed the API comment > > > --- > > > --- > > > lib/librte_net/rte_ip.h | 18 +++++++++++++++--- > > > 1 file changed, 15 insertions(+), 3 deletions(-) > > > > > > diff --git a/lib/librte_net/rte_ip.h b/lib/librte_net/rte_ip.h > > > index 292f63fd7..d03c77120 100644 > > > --- a/lib/librte_net/rte_ip.h > > > +++ b/lib/librte_net/rte_ip.h > > > @@ -325,7 +325,7 @@ rte_ipv4_phdr_cksum(const struct rte_ipv4_hdr > > *ipv4_hdr, uint64_t ol_flags) > > > * The pointer to the beginning of the L4 header. > > > * @return > > > * The complemented checksum to set in the IP packet > > > - * or 0 on error > > > + * or 0 if the IP length is invalid in the header. > > > */ > > > static inline uint16_t > > > rte_ipv4_udptcp_cksum(const struct rte_ipv4_hdr *ipv4_hdr, const > > void *l4_hdr) > > 0 is a valid return value, so I suggest omitting it from the return value description: > > * @return > - * The complemented checksum to set in the IP packet > - * or 0 on error > + * The complemented checksum to set in the IP packet. > > The comparison "if (l3_len < sizeof(struct rte_ipv4_hdr))" is only there to protect against invalid input; it prevents l4_len from becoming negative. I don't get why "0 if the IP length is invalid in the header" should be removed from the comment: 0 is both a valid return value and the value returned on invalid packet. > For the same reason, unlikely() should be added to this comparison. Maybe yes, but that's another story I think. > Otherwise, > > Acked-by: Morten Brørup <mb@smartsharesystems.com> > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [dpdk-stable] [PATCH] net: fix unneeded replacement of 0 by ffff for TCP checksum 2020-07-10 13:16 ` Olivier Matz @ 2020-07-10 13:29 ` Morten Brørup 2020-07-10 13:41 ` Olivier Matz 0 siblings, 1 reply; 10+ messages in thread From: Morten Brørup @ 2020-07-10 13:29 UTC (permalink / raw) To: Olivier Matz Cc: Hongzhi Guo, dev, stable, stephen, thomas, konstantin.ananyev, ferruh.yigit, nicolas.chautru, zhoujingbin, chenchanghu, jerry.lilijun, haifeng.lin > From: Olivier Matz [mailto:olivier.matz@6wind.com] > Sent: Friday, July 10, 2020 3:16 PM > > On Fri, Jul 10, 2020 at 03:10:34PM +0200, Morten Brørup wrote: > > > From: Olivier Matz [mailto:olivier.matz@6wind.com] > > > Sent: Friday, July 10, 2020 2:41 PM > > > > > > On Fri, Jul 10, 2020 at 02:55:51PM +0800, Hongzhi Guo wrote: > > > > Per RFC768: > > > > If the computed checksum is zero, it is transmitted as all ones. > > > > An all zero transmitted checksum value means that the transmitter > > > > generated no checksum. > > > > > > > > RFC793 for TCP has no such special treatment for the checksum of > > > zero. > > > > > > > > Fixes: 6006818cfb26 ("net: new checksum functions") > > > > Cc: stable@dpdk.org > > > > > > > > Signed-off-by: Hongzhi Guo <guohongzhi1@huawei.com> > > > > --- > > > > v2: > > > > * Fixed commit tile > > > > * Fixed the API comment > > > > --- > > > > --- > > > > lib/librte_net/rte_ip.h | 18 +++++++++++++++--- > > > > 1 file changed, 15 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/lib/librte_net/rte_ip.h b/lib/librte_net/rte_ip.h > > > > index 292f63fd7..d03c77120 100644 > > > > --- a/lib/librte_net/rte_ip.h > > > > +++ b/lib/librte_net/rte_ip.h > > > > @@ -325,7 +325,7 @@ rte_ipv4_phdr_cksum(const struct rte_ipv4_hdr > > > *ipv4_hdr, uint64_t ol_flags) > > > > * The pointer to the beginning of the L4 header. > > > > * @return > > > > * The complemented checksum to set in the IP packet > > > > - * or 0 on error > > > > + * or 0 if the IP length is invalid in the header. > > > > */ > > > > static inline uint16_t > > > > rte_ipv4_udptcp_cksum(const struct rte_ipv4_hdr *ipv4_hdr, const > > > void *l4_hdr) > > > > 0 is a valid return value, so I suggest omitting it from the return > value description: > > > > * @return > > - * The complemented checksum to set in the IP packet > > - * or 0 on error > > + * The complemented checksum to set in the IP packet. > > > > The comparison "if (l3_len < sizeof(struct rte_ipv4_hdr))" is only > there to protect against invalid input; it prevents l4_len from > becoming negative. > > I don't get why "0 if the IP length is invalid in the header" should > be removed from the comment: 0 is both a valid return value and > the value returned on invalid packet. To avoid confusion. We do not want people to add error handling for a return value of 0. 0 is not a special value or an error, so it does not deserve explicit mentioning. If we want to mention the return value for garbage input, we should not use the wording "or 0", because this suggests that 0 is not a normal return value. > > > For the same reason, unlikely() should be added to this comparison. > > Maybe yes, but that's another story I think. Agree. I was just mentioning it so it can be done when modifying the function anyway. > > > Otherwise, > > > > Acked-by: Morten Brørup <mb@smartsharesystems.com> > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [dpdk-stable] [PATCH] net: fix unneeded replacement of 0 by ffff for TCP checksum 2020-07-10 13:29 ` Morten Brørup @ 2020-07-10 13:41 ` Olivier Matz 2020-07-10 13:56 ` [dpdk-stable] [dpdk-dev] " Morten Brørup 0 siblings, 1 reply; 10+ messages in thread From: Olivier Matz @ 2020-07-10 13:41 UTC (permalink / raw) To: Morten Brørup Cc: Hongzhi Guo, dev, stable, stephen, thomas, konstantin.ananyev, ferruh.yigit, nicolas.chautru, zhoujingbin, chenchanghu, jerry.lilijun, haifeng.lin On Fri, Jul 10, 2020 at 03:29:36PM +0200, Morten Brørup wrote: > > From: Olivier Matz [mailto:olivier.matz@6wind.com] > > Sent: Friday, July 10, 2020 3:16 PM > > > > On Fri, Jul 10, 2020 at 03:10:34PM +0200, Morten Brørup wrote: > > > > From: Olivier Matz [mailto:olivier.matz@6wind.com] > > > > Sent: Friday, July 10, 2020 2:41 PM > > > > > > > > On Fri, Jul 10, 2020 at 02:55:51PM +0800, Hongzhi Guo wrote: > > > > > Per RFC768: > > > > > If the computed checksum is zero, it is transmitted as all ones. > > > > > An all zero transmitted checksum value means that the transmitter > > > > > generated no checksum. > > > > > > > > > > RFC793 for TCP has no such special treatment for the checksum of > > > > zero. > > > > > > > > > > Fixes: 6006818cfb26 ("net: new checksum functions") > > > > > Cc: stable@dpdk.org > > > > > > > > > > Signed-off-by: Hongzhi Guo <guohongzhi1@huawei.com> > > > > > --- > > > > > v2: > > > > > * Fixed commit tile > > > > > * Fixed the API comment > > > > > --- > > > > > --- > > > > > lib/librte_net/rte_ip.h | 18 +++++++++++++++--- > > > > > 1 file changed, 15 insertions(+), 3 deletions(-) > > > > > > > > > > diff --git a/lib/librte_net/rte_ip.h b/lib/librte_net/rte_ip.h > > > > > index 292f63fd7..d03c77120 100644 > > > > > --- a/lib/librte_net/rte_ip.h > > > > > +++ b/lib/librte_net/rte_ip.h > > > > > @@ -325,7 +325,7 @@ rte_ipv4_phdr_cksum(const struct rte_ipv4_hdr > > > > *ipv4_hdr, uint64_t ol_flags) > > > > > * The pointer to the beginning of the L4 header. > > > > > * @return > > > > > * The complemented checksum to set in the IP packet > > > > > - * or 0 on error > > > > > + * or 0 if the IP length is invalid in the header. > > > > > */ > > > > > static inline uint16_t > > > > > rte_ipv4_udptcp_cksum(const struct rte_ipv4_hdr *ipv4_hdr, const > > > > void *l4_hdr) > > > > > > 0 is a valid return value, so I suggest omitting it from the return > > value description: > > > > > > * @return > > > - * The complemented checksum to set in the IP packet > > > - * or 0 on error > > > + * The complemented checksum to set in the IP packet. > > > > > > The comparison "if (l3_len < sizeof(struct rte_ipv4_hdr))" is only > > there to protect against invalid input; it prevents l4_len from > > becoming negative. > > > > I don't get why "0 if the IP length is invalid in the header" should > > be removed from the comment: 0 is both a valid return value and > > the value returned on invalid packet. > > To avoid confusion. We do not want people to add error handling for a return value of 0. > > 0 is not a special value or an error, so it does not deserve explicit mentioning. > > If we want to mention the return value for garbage input, we should not use the wording "or 0", because this suggests that 0 is not a normal return value. Ok, got it. So maybe this? The complemented checksum to set in the IP packet. If the IP length is invalid in the header, it returns 0. > > > > > > For the same reason, unlikely() should be added to this comparison. > > > > Maybe yes, but that's another story I think. > > Agree. I was just mentioning it so it can be done when modifying the function anyway. > > > > > > Otherwise, > > > > > > Acked-by: Morten Brørup <mb@smartsharesystems.com> > > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [dpdk-stable] [dpdk-dev] [PATCH] net: fix unneeded replacement of 0 by ffff for TCP checksum 2020-07-10 13:41 ` Olivier Matz @ 2020-07-10 13:56 ` Morten Brørup 2020-07-10 14:40 ` Olivier Matz 0 siblings, 1 reply; 10+ messages in thread From: Morten Brørup @ 2020-07-10 13:56 UTC (permalink / raw) To: Olivier Matz Cc: Hongzhi Guo, dev, stable, stephen, thomas, konstantin.ananyev, ferruh.yigit, nicolas.chautru, zhoujingbin, chenchanghu, jerry.lilijun, haifeng.lin > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier Matz > Sent: Friday, July 10, 2020 3:41 PM > > On Fri, Jul 10, 2020 at 03:29:36PM +0200, Morten Brørup wrote: > > > From: Olivier Matz [mailto:olivier.matz@6wind.com] > > > Sent: Friday, July 10, 2020 3:16 PM > > > > > > On Fri, Jul 10, 2020 at 03:10:34PM +0200, Morten Brørup wrote: > > > > > From: Olivier Matz [mailto:olivier.matz@6wind.com] > > > > > Sent: Friday, July 10, 2020 2:41 PM > > > > > > > > > > On Fri, Jul 10, 2020 at 02:55:51PM +0800, Hongzhi Guo wrote: > > > > > > Per RFC768: > > > > > > If the computed checksum is zero, it is transmitted as all > ones. > > > > > > An all zero transmitted checksum value means that the > transmitter > > > > > > generated no checksum. > > > > > > > > > > > > RFC793 for TCP has no such special treatment for the checksum > of > > > > > zero. > > > > > > > > > > > > Fixes: 6006818cfb26 ("net: new checksum functions") > > > > > > Cc: stable@dpdk.org > > > > > > > > > > > > Signed-off-by: Hongzhi Guo <guohongzhi1@huawei.com> > > > > > > --- > > > > > > v2: > > > > > > * Fixed commit tile > > > > > > * Fixed the API comment > > > > > > --- > > > > > > --- > > > > > > lib/librte_net/rte_ip.h | 18 +++++++++++++++--- > > > > > > 1 file changed, 15 insertions(+), 3 deletions(-) > > > > > > > > > > > > diff --git a/lib/librte_net/rte_ip.h > b/lib/librte_net/rte_ip.h > > > > > > index 292f63fd7..d03c77120 100644 > > > > > > --- a/lib/librte_net/rte_ip.h > > > > > > +++ b/lib/librte_net/rte_ip.h > > > > > > @@ -325,7 +325,7 @@ rte_ipv4_phdr_cksum(const struct > rte_ipv4_hdr > > > > > *ipv4_hdr, uint64_t ol_flags) > > > > > > * The pointer to the beginning of the L4 header. > > > > > > * @return > > > > > > * The complemented checksum to set in the IP packet > > > > > > - * or 0 on error > > > > > > + * or 0 if the IP length is invalid in the header. > > > > > > */ > > > > > > static inline uint16_t > > > > > > rte_ipv4_udptcp_cksum(const struct rte_ipv4_hdr *ipv4_hdr, > const > > > > > void *l4_hdr) > > > > > > > > 0 is a valid return value, so I suggest omitting it from the > return > > > value description: > > > > > > > > * @return > > > > - * The complemented checksum to set in the IP packet > > > > - * or 0 on error > > > > + * The complemented checksum to set in the IP packet. > > > > > > > > The comparison "if (l3_len < sizeof(struct rte_ipv4_hdr))" is > only > > > there to protect against invalid input; it prevents l4_len from > > > becoming negative. > > > > > > I don't get why "0 if the IP length is invalid in the header" > should > > > be removed from the comment: 0 is both a valid return value and > > > the value returned on invalid packet. > > > > To avoid confusion. We do not want people to add error handling for a > return value of 0. > > > > 0 is not a special value or an error, so it does not deserve explicit > mentioning. > > > > If we want to mention the return value for garbage input, we should > not use the wording "or 0", because this suggests that 0 is not a > normal return value. > > Ok, got it. > > So maybe this? > > The complemented checksum to set in the IP packet. If > the IP length is invalid in the header, it returns 0. > It still mentions 0 as a special value, increasing the risk of the defensive user adding "error handling" for a return value of 0. How about this? The complemented checksum to set in the IP packet. If the IP length is invalid in the header, the return value is undefined. > > > > > > > > > > For the same reason, unlikely() should be added to this > comparison. > > > > > > Maybe yes, but that's another story I think. > > > > Agree. I was just mentioning it so it can be done when modifying the > function anyway. > > > > > > > > > Otherwise, > > > > > > > > Acked-by: Morten Brørup <mb@smartsharesystems.com> > > > > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [dpdk-stable] [dpdk-dev] [PATCH] net: fix unneeded replacement of 0 by ffff for TCP checksum 2020-07-10 13:56 ` [dpdk-stable] [dpdk-dev] " Morten Brørup @ 2020-07-10 14:40 ` Olivier Matz 2020-07-10 14:52 ` Olivier Matz 2020-07-10 21:03 ` Thomas Monjalon 0 siblings, 2 replies; 10+ messages in thread From: Olivier Matz @ 2020-07-10 14:40 UTC (permalink / raw) To: Morten Brørup Cc: Hongzhi Guo, dev, stable, stephen, thomas, konstantin.ananyev, ferruh.yigit, nicolas.chautru, zhoujingbin, chenchanghu, jerry.lilijun, haifeng.lin On Fri, Jul 10, 2020 at 03:56:11PM +0200, Morten Brørup wrote: > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier Matz > > Sent: Friday, July 10, 2020 3:41 PM > > > > On Fri, Jul 10, 2020 at 03:29:36PM +0200, Morten Brørup wrote: > > > > From: Olivier Matz [mailto:olivier.matz@6wind.com] > > > > Sent: Friday, July 10, 2020 3:16 PM > > > > > > > > On Fri, Jul 10, 2020 at 03:10:34PM +0200, Morten Brørup wrote: > > > > > > From: Olivier Matz [mailto:olivier.matz@6wind.com] > > > > > > Sent: Friday, July 10, 2020 2:41 PM > > > > > > > > > > > > On Fri, Jul 10, 2020 at 02:55:51PM +0800, Hongzhi Guo wrote: > > > > > > > Per RFC768: > > > > > > > If the computed checksum is zero, it is transmitted as all > > ones. > > > > > > > An all zero transmitted checksum value means that the > > transmitter > > > > > > > generated no checksum. > > > > > > > > > > > > > > RFC793 for TCP has no such special treatment for the checksum > > of > > > > > > zero. > > > > > > > > > > > > > > Fixes: 6006818cfb26 ("net: new checksum functions") > > > > > > > Cc: stable@dpdk.org > > > > > > > > > > > > > > Signed-off-by: Hongzhi Guo <guohongzhi1@huawei.com> > > > > > > > --- > > > > > > > v2: > > > > > > > * Fixed commit tile > > > > > > > * Fixed the API comment > > > > > > > --- > > > > > > > --- > > > > > > > lib/librte_net/rte_ip.h | 18 +++++++++++++++--- > > > > > > > 1 file changed, 15 insertions(+), 3 deletions(-) > > > > > > > > > > > > > > diff --git a/lib/librte_net/rte_ip.h > > b/lib/librte_net/rte_ip.h > > > > > > > index 292f63fd7..d03c77120 100644 > > > > > > > --- a/lib/librte_net/rte_ip.h > > > > > > > +++ b/lib/librte_net/rte_ip.h > > > > > > > @@ -325,7 +325,7 @@ rte_ipv4_phdr_cksum(const struct > > rte_ipv4_hdr > > > > > > *ipv4_hdr, uint64_t ol_flags) > > > > > > > * The pointer to the beginning of the L4 header. > > > > > > > * @return > > > > > > > * The complemented checksum to set in the IP packet > > > > > > > - * or 0 on error > > > > > > > + * or 0 if the IP length is invalid in the header. > > > > > > > */ > > > > > > > static inline uint16_t > > > > > > > rte_ipv4_udptcp_cksum(const struct rte_ipv4_hdr *ipv4_hdr, > > const > > > > > > void *l4_hdr) > > > > > > > > > > 0 is a valid return value, so I suggest omitting it from the > > return > > > > value description: > > > > > > > > > > * @return > > > > > - * The complemented checksum to set in the IP packet > > > > > - * or 0 on error > > > > > + * The complemented checksum to set in the IP packet. > > > > > > > > > > The comparison "if (l3_len < sizeof(struct rte_ipv4_hdr))" is > > only > > > > there to protect against invalid input; it prevents l4_len from > > > > becoming negative. > > > > > > > > I don't get why "0 if the IP length is invalid in the header" > > should > > > > be removed from the comment: 0 is both a valid return value and > > > > the value returned on invalid packet. > > > > > > To avoid confusion. We do not want people to add error handling for a > > return value of 0. > > > > > > 0 is not a special value or an error, so it does not deserve explicit > > mentioning. > > > > > > If we want to mention the return value for garbage input, we should > > not use the wording "or 0", because this suggests that 0 is not a > > normal return value. > > > > Ok, got it. > > > > So maybe this? > > > > The complemented checksum to set in the IP packet. If > > the IP length is invalid in the header, it returns 0. > > > It still mentions 0 as a special value, increasing the risk of the defensive user adding "error handling" for a return value of 0. > > How about this? > > The complemented checksum to set in the IP packet. If > the IP length is invalid in the header, the return value > is undefined. After reading again your arguments, I think I prefer your first proposal, which was also Hongzhi's initial submission: - * The complemented checksum to set in the IP packet - * or 0 on error + * The complemented checksum to set in the IP packet. Thomas, do you want to to resubmit with this change? > > > > > > > > > > > > > > For the same reason, unlikely() should be added to this > > comparison. > > > > > > > > Maybe yes, but that's another story I think. > > > > > > Agree. I was just mentioning it so it can be done when modifying the > > function anyway. > > > > > > > > > > > > Otherwise, > > > > > > > > > > Acked-by: Morten Brørup <mb@smartsharesystems.com> > > > > > > > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [dpdk-stable] [dpdk-dev] [PATCH] net: fix unneeded replacement of 0 by ffff for TCP checksum 2020-07-10 14:40 ` Olivier Matz @ 2020-07-10 14:52 ` Olivier Matz 2020-07-10 21:03 ` Thomas Monjalon 1 sibling, 0 replies; 10+ messages in thread From: Olivier Matz @ 2020-07-10 14:52 UTC (permalink / raw) To: Morten Brørup Cc: Hongzhi Guo, dev, stable, stephen, thomas, konstantin.ananyev, ferruh.yigit, nicolas.chautru, zhoujingbin, chenchanghu, jerry.lilijun, haifeng.lin On Fri, Jul 10, 2020 at 04:40:59PM +0200, Olivier Matz wrote: > On Fri, Jul 10, 2020 at 03:56:11PM +0200, Morten Brørup wrote: > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier Matz > > > Sent: Friday, July 10, 2020 3:41 PM > > > > > > On Fri, Jul 10, 2020 at 03:29:36PM +0200, Morten Brørup wrote: > > > > > From: Olivier Matz [mailto:olivier.matz@6wind.com] > > > > > Sent: Friday, July 10, 2020 3:16 PM > > > > > > > > > > On Fri, Jul 10, 2020 at 03:10:34PM +0200, Morten Brørup wrote: > > > > > > > From: Olivier Matz [mailto:olivier.matz@6wind.com] > > > > > > > Sent: Friday, July 10, 2020 2:41 PM > > > > > > > > > > > > > > On Fri, Jul 10, 2020 at 02:55:51PM +0800, Hongzhi Guo wrote: > > > > > > > > Per RFC768: > > > > > > > > If the computed checksum is zero, it is transmitted as all > > > ones. > > > > > > > > An all zero transmitted checksum value means that the > > > transmitter > > > > > > > > generated no checksum. > > > > > > > > > > > > > > > > RFC793 for TCP has no such special treatment for the checksum > > > of > > > > > > > zero. > > > > > > > > > > > > > > > > Fixes: 6006818cfb26 ("net: new checksum functions") > > > > > > > > Cc: stable@dpdk.org > > > > > > > > > > > > > > > > Signed-off-by: Hongzhi Guo <guohongzhi1@huawei.com> > > > > > > > > --- > > > > > > > > v2: > > > > > > > > * Fixed commit tile > > > > > > > > * Fixed the API comment > > > > > > > > --- > > > > > > > > --- > > > > > > > > lib/librte_net/rte_ip.h | 18 +++++++++++++++--- > > > > > > > > 1 file changed, 15 insertions(+), 3 deletions(-) > > > > > > > > > > > > > > > > diff --git a/lib/librte_net/rte_ip.h > > > b/lib/librte_net/rte_ip.h > > > > > > > > index 292f63fd7..d03c77120 100644 > > > > > > > > --- a/lib/librte_net/rte_ip.h > > > > > > > > +++ b/lib/librte_net/rte_ip.h > > > > > > > > @@ -325,7 +325,7 @@ rte_ipv4_phdr_cksum(const struct > > > rte_ipv4_hdr > > > > > > > *ipv4_hdr, uint64_t ol_flags) > > > > > > > > * The pointer to the beginning of the L4 header. > > > > > > > > * @return > > > > > > > > * The complemented checksum to set in the IP packet > > > > > > > > - * or 0 on error > > > > > > > > + * or 0 if the IP length is invalid in the header. > > > > > > > > */ > > > > > > > > static inline uint16_t > > > > > > > > rte_ipv4_udptcp_cksum(const struct rte_ipv4_hdr *ipv4_hdr, > > > const > > > > > > > void *l4_hdr) > > > > > > > > > > > > 0 is a valid return value, so I suggest omitting it from the > > > return > > > > > value description: > > > > > > > > > > > > * @return > > > > > > - * The complemented checksum to set in the IP packet > > > > > > - * or 0 on error > > > > > > + * The complemented checksum to set in the IP packet. > > > > > > > > > > > > The comparison "if (l3_len < sizeof(struct rte_ipv4_hdr))" is > > > only > > > > > there to protect against invalid input; it prevents l4_len from > > > > > becoming negative. > > > > > > > > > > I don't get why "0 if the IP length is invalid in the header" > > > should > > > > > be removed from the comment: 0 is both a valid return value and > > > > > the value returned on invalid packet. > > > > > > > > To avoid confusion. We do not want people to add error handling for a > > > return value of 0. > > > > > > > > 0 is not a special value or an error, so it does not deserve explicit > > > mentioning. > > > > > > > > If we want to mention the return value for garbage input, we should > > > not use the wording "or 0", because this suggests that 0 is not a > > > normal return value. > > > > > > Ok, got it. > > > > > > So maybe this? > > > > > > The complemented checksum to set in the IP packet. If > > > the IP length is invalid in the header, it returns 0. > > > > > It still mentions 0 as a special value, increasing the risk of the defensive user adding "error handling" for a return value of 0. > > > > How about this? > > > > The complemented checksum to set in the IP packet. If > > the IP length is invalid in the header, the return value > > is undefined. > > After reading again your arguments, I think I prefer your first > proposal, which was also Hongzhi's initial submission: > > - * The complemented checksum to set in the IP packet > - * or 0 on error > + * The complemented checksum to set in the IP packet. > > Thomas, do you want to to resubmit with this change? Sorry, I meant "do you want me to resubmit?" > > > > > > > > > > > > > > > > > > > For the same reason, unlikely() should be added to this > > > comparison. > > > > > > > > > > Maybe yes, but that's another story I think. > > > > > > > > Agree. I was just mentioning it so it can be done when modifying the > > > function anyway. > > > > > > > > > > > > > > > Otherwise, > > > > > > > > > > > > Acked-by: Morten Brørup <mb@smartsharesystems.com> > > > > > > > > > > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [dpdk-stable] [dpdk-dev] [PATCH] net: fix unneeded replacement of 0 by ffff for TCP checksum 2020-07-10 14:40 ` Olivier Matz 2020-07-10 14:52 ` Olivier Matz @ 2020-07-10 21:03 ` Thomas Monjalon 1 sibling, 0 replies; 10+ messages in thread From: Thomas Monjalon @ 2020-07-10 21:03 UTC (permalink / raw) To: Hongzhi Guo Cc: Morten Brørup, dev, stable, stephen, konstantin.ananyev, ferruh.yigit, nicolas.chautru, zhoujingbin, chenchanghu, jerry.lilijun, haifeng.lin, Olivier Matz 10/07/2020 16:40, Olivier Matz: > > > > > > > On Fri, Jul 10, 2020 at 02:55:51PM +0800, Hongzhi Guo wrote: > > > > > > > > Per RFC768: > > > > > > > > If the computed checksum is zero, it is transmitted as all > > > ones. > > > > > > > > An all zero transmitted checksum value means that the > > > transmitter > > > > > > > > generated no checksum. > > > > > > > > > > > > > > > > RFC793 for TCP has no such special treatment for the checksum > > > of > > > > > > > zero. > > > > > > > > > > > > > > > > Fixes: 6006818cfb26 ("net: new checksum functions") > > > > > > > > Cc: stable@dpdk.org > > > > > > > > > > > > > > > > Signed-off-by: Hongzhi Guo <guohongzhi1@huawei.com> [...] > After reading again your arguments, I think I prefer your first > proposal, which was also Hongzhi's initial submission: > > - * The complemented checksum to set in the IP packet > - * or 0 on error > + * The complemented checksum to set in the IP packet. > > Thomas, do you want to to resubmit with this change? Applied with all changes in comments. ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-07-10 21:03 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-07-10 6:55 [dpdk-stable] [PATCH] net: fix unneeded replacement of 0 by ffff for TCP checksum Hongzhi Guo 2020-07-10 12:41 ` Olivier Matz 2020-07-10 13:10 ` Morten Brørup 2020-07-10 13:16 ` Olivier Matz 2020-07-10 13:29 ` Morten Brørup 2020-07-10 13:41 ` Olivier Matz 2020-07-10 13:56 ` [dpdk-stable] [dpdk-dev] " Morten Brørup 2020-07-10 14:40 ` Olivier Matz 2020-07-10 14:52 ` Olivier Matz 2020-07-10 21:03 ` Thomas Monjalon
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).