DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [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-dev] [PATCH] net: fix unneeded replacement of 0 by ffff for TCP checksum
  2020-07-10  6:55 [dpdk-dev] [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-dev] [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-dev] [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-dev] [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-dev] [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           ` 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-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-dev] [PATCH] net: fix unneeded replacement of 0 by ffff for TCP checksum
  2020-07-10 13:56           ` 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-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-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-dev] [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           ` 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).