DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] net/tap: Allow all-zero checksum for UDP over IPv4
@ 2020-11-09 14:22 Michael Pfeiffer
  2020-11-10 14:46 ` Ferruh Yigit
  2020-11-10 15:59 ` Ferruh Yigit
  0 siblings, 2 replies; 12+ messages in thread
From: Michael Pfeiffer @ 2020-11-09 14:22 UTC (permalink / raw)
  To: Keith Wiles; +Cc: dev, Michael Pfeiffer

Unlike TCP, UDP checksums are optional and may be zero to indicate "not
set" [RFC 768] (except for IPv6, where this prohibited [RFC 8200]). Add
this special case to the checksum offload emulation in net/tap.

Signed-off-by: Michael Pfeiffer <michael.pfeiffer@tu-ilmenau.de>
---
 drivers/net/tap/rte_eth_tap.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index 2f8abb12c..e486b41c5 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -303,6 +303,7 @@ tap_verify_csum(struct rte_mbuf *mbuf)
 	uint16_t cksum = 0;
 	void *l3_hdr;
 	void *l4_hdr;
+	struct rte_udp_hdr *udp_hdr;
 
 	if (l2 == RTE_PTYPE_L2_ETHER_VLAN)
 		l2_len += 4;
@@ -349,10 +350,18 @@ tap_verify_csum(struct rte_mbuf *mbuf)
 		/* Don't verify checksum for multi-segment packets. */
 		if (mbuf->nb_segs > 1)
 			return;
-		if (l3 == RTE_PTYPE_L3_IPV4)
+		if (l3 == RTE_PTYPE_L3_IPV4) {
+			if (l4 == RTE_PTYPE_L4_UDP) {
+				udp_hdr = (struct rte_udp_hdr *)l4_hdr;
+				if (udp_hdr->dgram_cksum == 0) {
+					mbuf->ol_flags |= PKT_RX_L4_CKSUM_NONE;
+					return;
+				}
+			}
 			cksum = ~rte_ipv4_udptcp_cksum(l3_hdr, l4_hdr);
-		else if (l3 == RTE_PTYPE_L3_IPV6)
+		} else if (l3 == RTE_PTYPE_L3_IPV6) {
 			cksum = ~rte_ipv6_udptcp_cksum(l3_hdr, l4_hdr);
+		}
 		mbuf->ol_flags |= cksum ?
 			PKT_RX_L4_CKSUM_BAD :
 			PKT_RX_L4_CKSUM_GOOD;
-- 
2.29.2


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] [PATCH] net/tap: Allow all-zero checksum for UDP over IPv4
  2020-11-09 14:22 [dpdk-dev] [PATCH] net/tap: Allow all-zero checksum for UDP over IPv4 Michael Pfeiffer
@ 2020-11-10 14:46 ` Ferruh Yigit
  2020-11-10 15:56   ` Ferruh Yigit
  2020-11-10 16:01   ` Morten Brørup
  2020-11-10 15:59 ` Ferruh Yigit
  1 sibling, 2 replies; 12+ messages in thread
From: Ferruh Yigit @ 2020-11-10 14:46 UTC (permalink / raw)
  To: Michael Pfeiffer, Keith Wiles; +Cc: dev

On 11/9/2020 2:22 PM, Michael Pfeiffer wrote:
> Unlike TCP, UDP checksums are optional and may be zero to indicate "not
> set" [RFC 768] (except for IPv6, where this prohibited [RFC 8200]). Add
> this special case to the checksum offload emulation in net/tap.
> 
> Signed-off-by: Michael Pfeiffer <michael.pfeiffer@tu-ilmenau.de>
> ---
>   drivers/net/tap/rte_eth_tap.c | 13 +++++++++++--
>   1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> index 2f8abb12c..e486b41c5 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -303,6 +303,7 @@ tap_verify_csum(struct rte_mbuf *mbuf)
>   	uint16_t cksum = 0;
>   	void *l3_hdr;
>   	void *l4_hdr;
> +	struct rte_udp_hdr *udp_hdr;
>   
>   	if (l2 == RTE_PTYPE_L2_ETHER_VLAN)
>   		l2_len += 4;
> @@ -349,10 +350,18 @@ tap_verify_csum(struct rte_mbuf *mbuf)
>   		/* Don't verify checksum for multi-segment packets. */
>   		if (mbuf->nb_segs > 1)
>   			return;
> -		if (l3 == RTE_PTYPE_L3_IPV4)
> +		if (l3 == RTE_PTYPE_L3_IPV4) {
> +			if (l4 == RTE_PTYPE_L4_UDP) {
> +				udp_hdr = (struct rte_udp_hdr *)l4_hdr;
> +				if (udp_hdr->dgram_cksum == 0) {
> +					mbuf->ol_flags |= PKT_RX_L4_CKSUM_NONE;
> +					return;
> +				}
> +			}
>   			cksum = ~rte_ipv4_udptcp_cksum(l3_hdr, l4_hdr);
> -		else if (l3 == RTE_PTYPE_L3_IPV6)
> +		} else if (l3 == RTE_PTYPE_L3_IPV6) {
>   			cksum = ~rte_ipv6_udptcp_cksum(l3_hdr, l4_hdr);
> +		}
>   		mbuf->ol_flags |= cksum ?
>   			PKT_RX_L4_CKSUM_BAD :
>   			PKT_RX_L4_CKSUM_GOOD;
> 

While checking this I stuck with following part:

  cksum = ~rte_ipv6_udptcp_cksum(l3_hdr, l4_hdr);
  ...
   mbuf->ol_flags |= cksum ?
   	PKT_RX_L4_CKSUM_BAD :
  	PKT_RX_L4_CKSUM_GOOD;


Is this correct, or am I missing something, can intention be '!' here instead of 
'~' ?

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] [PATCH] net/tap: Allow all-zero checksum for UDP over IPv4
  2020-11-10 14:46 ` Ferruh Yigit
@ 2020-11-10 15:56   ` Ferruh Yigit
  2020-11-10 16:01   ` Morten Brørup
  1 sibling, 0 replies; 12+ messages in thread
From: Ferruh Yigit @ 2020-11-10 15:56 UTC (permalink / raw)
  To: Michael Pfeiffer, Keith Wiles; +Cc: dev

On 11/10/2020 2:46 PM, Ferruh Yigit wrote:
> On 11/9/2020 2:22 PM, Michael Pfeiffer wrote:
>> Unlike TCP, UDP checksums are optional and may be zero to indicate "not
>> set" [RFC 768] (except for IPv6, where this prohibited [RFC 8200]). Add
>> this special case to the checksum offload emulation in net/tap.
>>
>> Signed-off-by: Michael Pfeiffer <michael.pfeiffer@tu-ilmenau.de>
>> ---
>>   drivers/net/tap/rte_eth_tap.c | 13 +++++++++++--
>>   1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
>> index 2f8abb12c..e486b41c5 100644
>> --- a/drivers/net/tap/rte_eth_tap.c
>> +++ b/drivers/net/tap/rte_eth_tap.c
>> @@ -303,6 +303,7 @@ tap_verify_csum(struct rte_mbuf *mbuf)
>>       uint16_t cksum = 0;
>>       void *l3_hdr;
>>       void *l4_hdr;
>> +    struct rte_udp_hdr *udp_hdr;
>>       if (l2 == RTE_PTYPE_L2_ETHER_VLAN)
>>           l2_len += 4;
>> @@ -349,10 +350,18 @@ tap_verify_csum(struct rte_mbuf *mbuf)
>>           /* Don't verify checksum for multi-segment packets. */
>>           if (mbuf->nb_segs > 1)
>>               return;
>> -        if (l3 == RTE_PTYPE_L3_IPV4)
>> +        if (l3 == RTE_PTYPE_L3_IPV4) {
>> +            if (l4 == RTE_PTYPE_L4_UDP) {
>> +                udp_hdr = (struct rte_udp_hdr *)l4_hdr;
>> +                if (udp_hdr->dgram_cksum == 0) {
>> +                    mbuf->ol_flags |= PKT_RX_L4_CKSUM_NONE;
>> +                    return;
>> +                }
>> +            }
>>               cksum = ~rte_ipv4_udptcp_cksum(l3_hdr, l4_hdr);
>> -        else if (l3 == RTE_PTYPE_L3_IPV6)
>> +        } else if (l3 == RTE_PTYPE_L3_IPV6) {
>>               cksum = ~rte_ipv6_udptcp_cksum(l3_hdr, l4_hdr);
>> +        }
>>           mbuf->ol_flags |= cksum ?
>>               PKT_RX_L4_CKSUM_BAD :
>>               PKT_RX_L4_CKSUM_GOOD;
>>
> 
> While checking this I stuck with following part:
> 
>   cksum = ~rte_ipv6_udptcp_cksum(l3_hdr, l4_hdr);
>   ...
>    mbuf->ol_flags |= cksum ?
>        PKT_RX_L4_CKSUM_BAD :
>       PKT_RX_L4_CKSUM_GOOD;
> 
> 
> Is this correct, or am I missing something, can intention be '!' here instead of 
> '~' ?

This seems correct, this is related to what cksum functions return and how to 
verify cksum, I will proceed with the patch.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] [PATCH] net/tap: Allow all-zero checksum for UDP over IPv4
  2020-11-09 14:22 [dpdk-dev] [PATCH] net/tap: Allow all-zero checksum for UDP over IPv4 Michael Pfeiffer
  2020-11-10 14:46 ` Ferruh Yigit
@ 2020-11-10 15:59 ` Ferruh Yigit
  2020-11-11  7:23   ` Michael Pfeiffer
  1 sibling, 1 reply; 12+ messages in thread
From: Ferruh Yigit @ 2020-11-10 15:59 UTC (permalink / raw)
  To: Michael Pfeiffer, Keith Wiles; +Cc: dev

On 11/9/2020 2:22 PM, Michael Pfeiffer wrote:
> Unlike TCP, UDP checksums are optional and may be zero to indicate "not
> set" [RFC 768] (except for IPv6, where this prohibited [RFC 8200]). Add
> this special case to the checksum offload emulation in net/tap.
> 
> Signed-off-by: Michael Pfeiffer <michael.pfeiffer@tu-ilmenau.de>
> ---
>   drivers/net/tap/rte_eth_tap.c | 13 +++++++++++--
>   1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> index 2f8abb12c..e486b41c5 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -303,6 +303,7 @@ tap_verify_csum(struct rte_mbuf *mbuf)
>   	uint16_t cksum = 0;
>   	void *l3_hdr;
>   	void *l4_hdr;
> +	struct rte_udp_hdr *udp_hdr;
>   
>   	if (l2 == RTE_PTYPE_L2_ETHER_VLAN)
>   		l2_len += 4;
> @@ -349,10 +350,18 @@ tap_verify_csum(struct rte_mbuf *mbuf)
>   		/* Don't verify checksum for multi-segment packets. */
>   		if (mbuf->nb_segs > 1)
>   			return;
> -		if (l3 == RTE_PTYPE_L3_IPV4)
> +		if (l3 == RTE_PTYPE_L3_IPV4) {
> +			if (l4 == RTE_PTYPE_L4_UDP) {
> +				udp_hdr = (struct rte_udp_hdr *)l4_hdr;
> +				if (udp_hdr->dgram_cksum == 0) {

Overall patch looks good to me, but can you please add a comment on top of above 
check to describe why checksum can be zero, as done in the commit log.

> +					mbuf->ol_flags |= PKT_RX_L4_CKSUM_NONE;
> +					return;
> +				}
> +			}
>   			cksum = ~rte_ipv4_udptcp_cksum(l3_hdr, l4_hdr);
> -		else if (l3 == RTE_PTYPE_L3_IPV6)
> +		} else if (l3 == RTE_PTYPE_L3_IPV6) {
>   			cksum = ~rte_ipv6_udptcp_cksum(l3_hdr, l4_hdr);
> +		}
>   		mbuf->ol_flags |= cksum ?
>   			PKT_RX_L4_CKSUM_BAD :
>   			PKT_RX_L4_CKSUM_GOOD;
> 


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] [PATCH] net/tap: Allow all-zero checksum for UDP over IPv4
  2020-11-10 14:46 ` Ferruh Yigit
  2020-11-10 15:56   ` Ferruh Yigit
@ 2020-11-10 16:01   ` Morten Brørup
  2020-11-10 17:42     ` Ferruh Yigit
  1 sibling, 1 reply; 12+ messages in thread
From: Morten Brørup @ 2020-11-10 16:01 UTC (permalink / raw)
  To: Ferruh Yigit, Michael Pfeiffer, Keith Wiles; +Cc: dev

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ferruh Yigit
> Sent: Tuesday, November 10, 2020 3:47 PM
> 
> On 11/9/2020 2:22 PM, Michael Pfeiffer wrote:
> > Unlike TCP, UDP checksums are optional and may be zero to indicate "not
> > set" [RFC 768] (except for IPv6, where this prohibited [RFC 8200]). Add
> > this special case to the checksum offload emulation in net/tap.
> >
> > Signed-off-by: Michael Pfeiffer <michael.pfeiffer@tu-ilmenau.de>
> > ---
> >   drivers/net/tap/rte_eth_tap.c | 13 +++++++++++--
> >   1 file changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/tap/rte_eth_tap.c
> b/drivers/net/tap/rte_eth_tap.c
> > index 2f8abb12c..e486b41c5 100644
> > --- a/drivers/net/tap/rte_eth_tap.c
> > +++ b/drivers/net/tap/rte_eth_tap.c
> > @@ -303,6 +303,7 @@ tap_verify_csum(struct rte_mbuf *mbuf)
> >   	uint16_t cksum = 0;
> >   	void *l3_hdr;
> >   	void *l4_hdr;
> > +	struct rte_udp_hdr *udp_hdr;
> >
> >   	if (l2 == RTE_PTYPE_L2_ETHER_VLAN)
> >   		l2_len += 4;
> > @@ -349,10 +350,18 @@ tap_verify_csum(struct rte_mbuf *mbuf)
> >   		/* Don't verify checksum for multi-segment packets. */
> >   		if (mbuf->nb_segs > 1)
> >   			return;
> > -		if (l3 == RTE_PTYPE_L3_IPV4)
> > +		if (l3 == RTE_PTYPE_L3_IPV4) {
> > +			if (l4 == RTE_PTYPE_L4_UDP) {
> > +				udp_hdr = (struct rte_udp_hdr *)l4_hdr;
> > +				if (udp_hdr->dgram_cksum == 0) {
> > +					mbuf->ol_flags |= PKT_RX_L4_CKSUM_NONE;
> > +					return;
> > +				}
> > +			}
> >   			cksum = ~rte_ipv4_udptcp_cksum(l3_hdr, l4_hdr);
> > -		else if (l3 == RTE_PTYPE_L3_IPV6)
> > +		} else if (l3 == RTE_PTYPE_L3_IPV6) {
> >   			cksum = ~rte_ipv6_udptcp_cksum(l3_hdr, l4_hdr);
> > +		}
> >   		mbuf->ol_flags |= cksum ?
> >   			PKT_RX_L4_CKSUM_BAD :
> >   			PKT_RX_L4_CKSUM_GOOD;
> >
> 
> While checking this I stuck with following part:
> 
>   cksum = ~rte_ipv6_udptcp_cksum(l3_hdr, l4_hdr);
>   ...
>    mbuf->ol_flags |= cksum ?
>    	PKT_RX_L4_CKSUM_BAD :
>   	PKT_RX_L4_CKSUM_GOOD;
> 
> 
> Is this correct, or am I missing something, can intention be '!' here
> instead of
> '~' ?

It is correct. The packet's checksum is calculated by rte_ipv6_udptcp_cksum(), and it should be 0xFFFF. The '~' operation makes cksum 0 iff the calculated checksum is 0xFFFF.


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] [PATCH] net/tap: Allow all-zero checksum for UDP over IPv4
  2020-11-10 16:01   ` Morten Brørup
@ 2020-11-10 17:42     ` Ferruh Yigit
  2020-11-11  7:06       ` Morten Brørup
  0 siblings, 1 reply; 12+ messages in thread
From: Ferruh Yigit @ 2020-11-10 17:42 UTC (permalink / raw)
  To: Morten Brørup, Michael Pfeiffer, Keith Wiles; +Cc: dev

On 11/10/2020 4:01 PM, Morten Brørup wrote:
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ferruh Yigit
>> Sent: Tuesday, November 10, 2020 3:47 PM
>>
>> On 11/9/2020 2:22 PM, Michael Pfeiffer wrote:
>>> Unlike TCP, UDP checksums are optional and may be zero to indicate "not
>>> set" [RFC 768] (except for IPv6, where this prohibited [RFC 8200]). Add
>>> this special case to the checksum offload emulation in net/tap.
>>>
>>> Signed-off-by: Michael Pfeiffer <michael.pfeiffer@tu-ilmenau.de>
>>> ---
>>>    drivers/net/tap/rte_eth_tap.c | 13 +++++++++++--
>>>    1 file changed, 11 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/tap/rte_eth_tap.c
>> b/drivers/net/tap/rte_eth_tap.c
>>> index 2f8abb12c..e486b41c5 100644
>>> --- a/drivers/net/tap/rte_eth_tap.c
>>> +++ b/drivers/net/tap/rte_eth_tap.c
>>> @@ -303,6 +303,7 @@ tap_verify_csum(struct rte_mbuf *mbuf)
>>>    	uint16_t cksum = 0;
>>>    	void *l3_hdr;
>>>    	void *l4_hdr;
>>> +	struct rte_udp_hdr *udp_hdr;
>>>
>>>    	if (l2 == RTE_PTYPE_L2_ETHER_VLAN)
>>>    		l2_len += 4;
>>> @@ -349,10 +350,18 @@ tap_verify_csum(struct rte_mbuf *mbuf)
>>>    		/* Don't verify checksum for multi-segment packets. */
>>>    		if (mbuf->nb_segs > 1)
>>>    			return;
>>> -		if (l3 == RTE_PTYPE_L3_IPV4)
>>> +		if (l3 == RTE_PTYPE_L3_IPV4) {
>>> +			if (l4 == RTE_PTYPE_L4_UDP) {
>>> +				udp_hdr = (struct rte_udp_hdr *)l4_hdr;
>>> +				if (udp_hdr->dgram_cksum == 0) {
>>> +					mbuf->ol_flags |= PKT_RX_L4_CKSUM_NONE;
>>> +					return;
>>> +				}
>>> +			}
>>>    			cksum = ~rte_ipv4_udptcp_cksum(l3_hdr, l4_hdr);
>>> -		else if (l3 == RTE_PTYPE_L3_IPV6)
>>> +		} else if (l3 == RTE_PTYPE_L3_IPV6) {
>>>    			cksum = ~rte_ipv6_udptcp_cksum(l3_hdr, l4_hdr);
>>> +		}
>>>    		mbuf->ol_flags |= cksum ?
>>>    			PKT_RX_L4_CKSUM_BAD :
>>>    			PKT_RX_L4_CKSUM_GOOD;
>>>
>>
>> While checking this I stuck with following part:
>>
>>    cksum = ~rte_ipv6_udptcp_cksum(l3_hdr, l4_hdr);
>>    ...
>>     mbuf->ol_flags |= cksum ?
>>     	PKT_RX_L4_CKSUM_BAD :
>>    	PKT_RX_L4_CKSUM_GOOD;
>>
>>
>> Is this correct, or am I missing something, can intention be '!' here
>> instead of
>> '~' ?
> 
> It is correct. The packet's checksum is calculated by rte_ipv6_udptcp_cksum(), and it should be 0xFFFF. The '~' operation makes cksum 0 iff the calculated checksum is 0xFFFF.
> 

Yep, figure that out late,
as far as I understand when the checksum value is zero, 
'rte_ipv6_udptcp_cksum()' will return the checksum value and when checksum is 
correct in the packet, function will return 0xFFFF, this is based on checksum 
calculation, is this right?

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] [PATCH] net/tap: Allow all-zero checksum for UDP over IPv4
  2020-11-10 17:42     ` Ferruh Yigit
@ 2020-11-11  7:06       ` Morten Brørup
  0 siblings, 0 replies; 12+ messages in thread
From: Morten Brørup @ 2020-11-11  7:06 UTC (permalink / raw)
  To: Ferruh Yigit, Michael Pfeiffer, Keith Wiles; +Cc: dev

> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
> Sent: Tuesday, November 10, 2020 6:43 PM
> 
> On 11/10/2020 4:01 PM, Morten Brørup wrote:
> >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ferruh Yigit
> >> Sent: Tuesday, November 10, 2020 3:47 PM
> >>
> >> On 11/9/2020 2:22 PM, Michael Pfeiffer wrote:
> >>> Unlike TCP, UDP checksums are optional and may be zero to indicate
> "not
> >>> set" [RFC 768] (except for IPv6, where this prohibited [RFC 8200]).
> Add
> >>> this special case to the checksum offload emulation in net/tap.
> >>>
> >>> Signed-off-by: Michael Pfeiffer <michael.pfeiffer@tu-ilmenau.de>
> >>> ---
> >>>    drivers/net/tap/rte_eth_tap.c | 13 +++++++++++--
> >>>    1 file changed, 11 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/net/tap/rte_eth_tap.c
> >> b/drivers/net/tap/rte_eth_tap.c
> >>> index 2f8abb12c..e486b41c5 100644
> >>> --- a/drivers/net/tap/rte_eth_tap.c
> >>> +++ b/drivers/net/tap/rte_eth_tap.c
> >>> @@ -303,6 +303,7 @@ tap_verify_csum(struct rte_mbuf *mbuf)
> >>>    	uint16_t cksum = 0;
> >>>    	void *l3_hdr;
> >>>    	void *l4_hdr;
> >>> +	struct rte_udp_hdr *udp_hdr;
> >>>
> >>>    	if (l2 == RTE_PTYPE_L2_ETHER_VLAN)
> >>>    		l2_len += 4;
> >>> @@ -349,10 +350,18 @@ tap_verify_csum(struct rte_mbuf *mbuf)
> >>>    		/* Don't verify checksum for multi-segment packets.
> */
> >>>    		if (mbuf->nb_segs > 1)
> >>>    			return;
> >>> -		if (l3 == RTE_PTYPE_L3_IPV4)
> >>> +		if (l3 == RTE_PTYPE_L3_IPV4) {
> >>> +			if (l4 == RTE_PTYPE_L4_UDP) {
> >>> +				udp_hdr = (struct rte_udp_hdr *)l4_hdr;
> >>> +				if (udp_hdr->dgram_cksum == 0) {
> >>> +					mbuf->ol_flags |= PKT_RX_L4_CKSUM_NONE;
> >>> +					return;
> >>> +				}
> >>> +			}
> >>>    			cksum = ~rte_ipv4_udptcp_cksum(l3_hdr, l4_hdr);
> >>> -		else if (l3 == RTE_PTYPE_L3_IPV6)
> >>> +		} else if (l3 == RTE_PTYPE_L3_IPV6) {
> >>>    			cksum = ~rte_ipv6_udptcp_cksum(l3_hdr, l4_hdr);
> >>> +		}
> >>>    		mbuf->ol_flags |= cksum ?
> >>>    			PKT_RX_L4_CKSUM_BAD :
> >>>    			PKT_RX_L4_CKSUM_GOOD;
> >>>
> >>
> >> While checking this I stuck with following part:
> >>
> >>    cksum = ~rte_ipv6_udptcp_cksum(l3_hdr, l4_hdr);
> >>    ...
> >>     mbuf->ol_flags |= cksum ?
> >>     	PKT_RX_L4_CKSUM_BAD :
> >>    	PKT_RX_L4_CKSUM_GOOD;
> >>
> >>
> >> Is this correct, or am I missing something, can intention be '!'
> here
> >> instead of
> >> '~' ?
> >
> > It is correct. The packet's checksum is calculated by
> rte_ipv6_udptcp_cksum(), and it should be 0xFFFF. The '~' operation
> makes cksum 0 iff the calculated checksum is 0xFFFF.
> >
> 
> Yep, figure that out late,
> as far as I understand when the checksum value is zero,
> 'rte_ipv6_udptcp_cksum()' will return the checksum value and when
> checksum is
> correct in the packet, function will return 0xFFFF, this is based on
> checksum
> calculation, is this right?

Exactly.


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] [PATCH] net/tap: Allow all-zero checksum for UDP over IPv4
  2020-11-10 15:59 ` Ferruh Yigit
@ 2020-11-11  7:23   ` Michael Pfeiffer
  2020-11-11  9:31     ` Ferruh Yigit
  0 siblings, 1 reply; 12+ messages in thread
From: Michael Pfeiffer @ 2020-11-11  7:23 UTC (permalink / raw)
  To: Ferruh Yigit, Keith Wiles; +Cc: dev

Hi,

On Tue, 2020-11-10 at 15:59 +0000, Ferruh Yigit wrote:
> On 11/9/2020 2:22 PM, Michael Pfeiffer wrote:
> > Unlike TCP, UDP checksums are optional and may be zero to indicate "not
> > set" [RFC 768] (except for IPv6, where this prohibited [RFC 8200]). Add
> > this special case to the checksum offload emulation in net/tap.
> > 
> > Signed-off-by: Michael Pfeiffer <michael.pfeiffer@tu-ilmenau.de>
> > ---
> >   drivers/net/tap/rte_eth_tap.c | 13 +++++++++++--
> >   1 file changed, 11 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> > index 2f8abb12c..e486b41c5 100644
> > --- a/drivers/net/tap/rte_eth_tap.c
> > +++ b/drivers/net/tap/rte_eth_tap.c
> > @@ -303,6 +303,7 @@ tap_verify_csum(struct rte_mbuf *mbuf)
> >         uint16_t cksum = 0;
> >         void *l3_hdr;
> >         void *l4_hdr;
> > +       struct rte_udp_hdr *udp_hdr;
> >   
> >         if (l2 == RTE_PTYPE_L2_ETHER_VLAN)
> >                 l2_len += 4;
> > @@ -349,10 +350,18 @@ tap_verify_csum(struct rte_mbuf *mbuf)
> >                 /* Don't verify checksum for multi-segment packets. */
> >                 if (mbuf->nb_segs > 1)
> >                         return;
> > -               if (l3 == RTE_PTYPE_L3_IPV4)
> > +               if (l3 == RTE_PTYPE_L3_IPV4) {
> > +                       if (l4 == RTE_PTYPE_L4_UDP) {
> > +                               udp_hdr = (struct rte_udp_hdr *)l4_hdr;
> > +                               if (udp_hdr->dgram_cksum == 0) {
> 
> Overall patch looks good to me, but can you please add a comment on top of
> above 
> check to describe why checksum can be zero, as done in the commit log.

Sure, I will update the patch. I am also not completely sure whether
PKT_RX_L4_CKSUM_NONE is the right flag for this case (rather than _UNKNOWN).
From rte_core_mbuf.h:

 * - PKT_RX_L4_CKSUM_UNKNOWN: no information about the RX L4 checksum
 * - PKT_RX_L4_CKSUM_NONE: the L4 checksum is not correct in the packet
 *   data, but the integrity of the L4 data is verified.

The second part after the "but" is not really the case here. I don't know how
relevant the distinction is, as most application side code will probably only
do something like

if ((mbuf->ol_flags & PKT_RX_L4_CKSUM_MASK) == PKT_RX_L4_CKSUM_BAD)
	rte_pktmbuf_free(mbuf);

anyway. Do you have any opinions on that?

> > +                                       mbuf->ol_flags |=
> > PKT_RX_L4_CKSUM_NONE;
> > +                                       return;
> > +                               }
> > +                       }
> >                         cksum = ~rte_ipv4_udptcp_cksum(l3_hdr, l4_hdr);
> > -               else if (l3 == RTE_PTYPE_L3_IPV6)
> > +               } else if (l3 == RTE_PTYPE_L3_IPV6) {
> >                         cksum = ~rte_ipv6_udptcp_cksum(l3_hdr, l4_hdr);
> > +               }
> >                 mbuf->ol_flags |= cksum ?
> >                         PKT_RX_L4_CKSUM_BAD :
> >                         PKT_RX_L4_CKSUM_GOOD;
> > 
> 

Regards
Michael


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] [PATCH] net/tap: Allow all-zero checksum for UDP over IPv4
  2020-11-11  7:23   ` Michael Pfeiffer
@ 2020-11-11  9:31     ` Ferruh Yigit
  2020-11-13 13:02       ` Ferruh Yigit
  0 siblings, 1 reply; 12+ messages in thread
From: Ferruh Yigit @ 2020-11-11  9:31 UTC (permalink / raw)
  To: Michael Pfeiffer, Keith Wiles, Olivier Matz; +Cc: dev

On 11/11/2020 7:23 AM, Michael Pfeiffer wrote:
> Hi,
> 
> On Tue, 2020-11-10 at 15:59 +0000, Ferruh Yigit wrote:
>> On 11/9/2020 2:22 PM, Michael Pfeiffer wrote:
>>> Unlike TCP, UDP checksums are optional and may be zero to indicate "not
>>> set" [RFC 768] (except for IPv6, where this prohibited [RFC 8200]). Add
>>> this special case to the checksum offload emulation in net/tap.
>>>
>>> Signed-off-by: Michael Pfeiffer <michael.pfeiffer@tu-ilmenau.de>
>>> ---
>>>    drivers/net/tap/rte_eth_tap.c | 13 +++++++++++--
>>>    1 file changed, 11 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
>>> index 2f8abb12c..e486b41c5 100644
>>> --- a/drivers/net/tap/rte_eth_tap.c
>>> +++ b/drivers/net/tap/rte_eth_tap.c
>>> @@ -303,6 +303,7 @@ tap_verify_csum(struct rte_mbuf *mbuf)
>>>          uint16_t cksum = 0;
>>>          void *l3_hdr;
>>>          void *l4_hdr;
>>> +       struct rte_udp_hdr *udp_hdr;
>>>    
>>>          if (l2 == RTE_PTYPE_L2_ETHER_VLAN)
>>>                  l2_len += 4;
>>> @@ -349,10 +350,18 @@ tap_verify_csum(struct rte_mbuf *mbuf)
>>>                  /* Don't verify checksum for multi-segment packets. */
>>>                  if (mbuf->nb_segs > 1)
>>>                          return;
>>> -               if (l3 == RTE_PTYPE_L3_IPV4)
>>> +               if (l3 == RTE_PTYPE_L3_IPV4) {
>>> +                       if (l4 == RTE_PTYPE_L4_UDP) {
>>> +                               udp_hdr = (struct rte_udp_hdr *)l4_hdr;
>>> +                               if (udp_hdr->dgram_cksum == 0) {
>>
>> Overall patch looks good to me, but can you please add a comment on top of
>> above
>> check to describe why checksum can be zero, as done in the commit log.
> 
> Sure, I will update the patch. I am also not completely sure whether
> PKT_RX_L4_CKSUM_NONE is the right flag for this case (rather than _UNKNOWN).
>  From rte_core_mbuf.h:
> 
>   * - PKT_RX_L4_CKSUM_UNKNOWN: no information about the RX L4 checksum
>   * - PKT_RX_L4_CKSUM_NONE: the L4 checksum is not correct in the packet
>   *   data, but the integrity of the L4 data is verified.
> 
> The second part after the "but" is not really the case here. I don't know how
> relevant the distinction is, as most application side code will probably only
> do something like
> 
> if ((mbuf->ol_flags & PKT_RX_L4_CKSUM_MASK) == PKT_RX_L4_CKSUM_BAD)
> 	rte_pktmbuf_free(mbuf);
> 
> anyway. Do you have any opinions on that?
> 

I also checked for that and wasn't sure about it :) cc'ed Olivier too for comment.

I think it is NOT 'PKT_RX_L4_CKSUM_UNKNOWN', since we know that checksum value 
is 0x0000 which means it is not provided.

'PKT_RX_L4_CKSUM_NONE' suits better but not sure about the expectation on 
"integrity of the L4 data is verified" part, I assume that explanation is just 
to differentiate between 'CKSUM_BAD'.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] [PATCH] net/tap: Allow all-zero checksum for UDP over IPv4
  2020-11-11  9:31     ` Ferruh Yigit
@ 2020-11-13 13:02       ` Ferruh Yigit
  2020-11-13 14:03         ` [dpdk-dev] [PATCH v2] " Michael Pfeiffer
  0 siblings, 1 reply; 12+ messages in thread
From: Ferruh Yigit @ 2020-11-13 13:02 UTC (permalink / raw)
  To: Michael Pfeiffer, Keith Wiles, Olivier Matz; +Cc: dev

On 11/11/2020 9:31 AM, Ferruh Yigit wrote:
> On 11/11/2020 7:23 AM, Michael Pfeiffer wrote:
>> Hi,
>>
>> On Tue, 2020-11-10 at 15:59 +0000, Ferruh Yigit wrote:
>>> On 11/9/2020 2:22 PM, Michael Pfeiffer wrote:
>>>> Unlike TCP, UDP checksums are optional and may be zero to indicate "not
>>>> set" [RFC 768] (except for IPv6, where this prohibited [RFC 8200]). Add
>>>> this special case to the checksum offload emulation in net/tap.
>>>>
>>>> Signed-off-by: Michael Pfeiffer <michael.pfeiffer@tu-ilmenau.de>
>>>> ---
>>>>    drivers/net/tap/rte_eth_tap.c | 13 +++++++++++--
>>>>    1 file changed, 11 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
>>>> index 2f8abb12c..e486b41c5 100644
>>>> --- a/drivers/net/tap/rte_eth_tap.c
>>>> +++ b/drivers/net/tap/rte_eth_tap.c
>>>> @@ -303,6 +303,7 @@ tap_verify_csum(struct rte_mbuf *mbuf)
>>>>          uint16_t cksum = 0;
>>>>          void *l3_hdr;
>>>>          void *l4_hdr;
>>>> +       struct rte_udp_hdr *udp_hdr;
>>>>          if (l2 == RTE_PTYPE_L2_ETHER_VLAN)
>>>>                  l2_len += 4;
>>>> @@ -349,10 +350,18 @@ tap_verify_csum(struct rte_mbuf *mbuf)
>>>>                  /* Don't verify checksum for multi-segment packets. */
>>>>                  if (mbuf->nb_segs > 1)
>>>>                          return;
>>>> -               if (l3 == RTE_PTYPE_L3_IPV4)
>>>> +               if (l3 == RTE_PTYPE_L3_IPV4) {
>>>> +                       if (l4 == RTE_PTYPE_L4_UDP) {
>>>> +                               udp_hdr = (struct rte_udp_hdr *)l4_hdr;
>>>> +                               if (udp_hdr->dgram_cksum == 0) {
>>>
>>> Overall patch looks good to me, but can you please add a comment on top of
>>> above
>>> check to describe why checksum can be zero, as done in the commit log.
>>
>> Sure, I will update the patch. I am also not completely sure whether
>> PKT_RX_L4_CKSUM_NONE is the right flag for this case (rather than _UNKNOWN).
>>  From rte_core_mbuf.h:
>>
>>   * - PKT_RX_L4_CKSUM_UNKNOWN: no information about the RX L4 checksum
>>   * - PKT_RX_L4_CKSUM_NONE: the L4 checksum is not correct in the packet
>>   *   data, but the integrity of the L4 data is verified.
>>
>> The second part after the "but" is not really the case here. I don't know how
>> relevant the distinction is, as most application side code will probably only
>> do something like
>>
>> if ((mbuf->ol_flags & PKT_RX_L4_CKSUM_MASK) == PKT_RX_L4_CKSUM_BAD)
>>     rte_pktmbuf_free(mbuf);
>>
>> anyway. Do you have any opinions on that?
>>
> 
> I also checked for that and wasn't sure about it :) cc'ed Olivier too for comment.
> 
> I think it is NOT 'PKT_RX_L4_CKSUM_UNKNOWN', since we know that checksum value 
> is 0x0000 which means it is not provided.
> 
> 'PKT_RX_L4_CKSUM_NONE' suits better but not sure about the expectation on 
> "integrity of the L4 data is verified" part, I assume that explanation is just 
> to differentiate between 'CKSUM_BAD'.

I suggest to continue with 'PKT_RX_L4_CKSUM_NONE'.

Can it be possible to get the new version today, so we can include this to the -rc4?

Thanks

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [dpdk-dev] [PATCH v2] net/tap: Allow all-zero checksum for UDP over IPv4
  2020-11-13 13:02       ` Ferruh Yigit
@ 2020-11-13 14:03         ` Michael Pfeiffer
  2020-11-13 14:49           ` Ferruh Yigit
  0 siblings, 1 reply; 12+ messages in thread
From: Michael Pfeiffer @ 2020-11-13 14:03 UTC (permalink / raw)
  To: Ferruh Yigit; +Cc: Keith Wiles, dev, Michael Pfeiffer

Unlike TCP, UDP checksums are optional and may be zero to indicate "not
set" [RFC 768] (except for IPv6, where this prohibited [RFC 8200]). Add
this special case to the checksum offload emulation in net/tap.

Signed-off-by: Michael Pfeiffer <michael.pfeiffer@tu-ilmenau.de>
---

v2:
* Added comment.

 drivers/net/tap/rte_eth_tap.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index 2f8abb12c..2542de306 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -303,6 +303,7 @@ tap_verify_csum(struct rte_mbuf *mbuf)
 	uint16_t cksum = 0;
 	void *l3_hdr;
 	void *l4_hdr;
+	struct rte_udp_hdr *udp_hdr;
 
 	if (l2 == RTE_PTYPE_L2_ETHER_VLAN)
 		l2_len += 4;
@@ -349,10 +350,23 @@ tap_verify_csum(struct rte_mbuf *mbuf)
 		/* Don't verify checksum for multi-segment packets. */
 		if (mbuf->nb_segs > 1)
 			return;
-		if (l3 == RTE_PTYPE_L3_IPV4)
+		if (l3 == RTE_PTYPE_L3_IPV4) {
+			if (l4 == RTE_PTYPE_L4_UDP) {
+				udp_hdr = (struct rte_udp_hdr *)l4_hdr;
+				if (udp_hdr->dgram_cksum == 0) {
+					/*
+					 * For IPv4, a zero UDP checksum
+					 * indicates that the sender did not
+					 * generate one [RFC 768].
+					 */
+					mbuf->ol_flags |= PKT_RX_L4_CKSUM_NONE;
+					return;
+				}
+			}
 			cksum = ~rte_ipv4_udptcp_cksum(l3_hdr, l4_hdr);
-		else if (l3 == RTE_PTYPE_L3_IPV6)
+		} else if (l3 == RTE_PTYPE_L3_IPV6) {
 			cksum = ~rte_ipv6_udptcp_cksum(l3_hdr, l4_hdr);
+		}
 		mbuf->ol_flags |= cksum ?
 			PKT_RX_L4_CKSUM_BAD :
 			PKT_RX_L4_CKSUM_GOOD;
-- 
2.20.1


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] [PATCH v2] net/tap: Allow all-zero checksum for UDP over IPv4
  2020-11-13 14:03         ` [dpdk-dev] [PATCH v2] " Michael Pfeiffer
@ 2020-11-13 14:49           ` Ferruh Yigit
  0 siblings, 0 replies; 12+ messages in thread
From: Ferruh Yigit @ 2020-11-13 14:49 UTC (permalink / raw)
  To: Michael Pfeiffer; +Cc: Keith Wiles, dev

On 11/13/2020 2:03 PM, Michael Pfeiffer wrote:
> Unlike TCP, UDP checksums are optional and may be zero to indicate "not
> set" [RFC 768] (except for IPv6, where this prohibited [RFC 8200]). Add
> this special case to the checksum offload emulation in net/tap.
> 
> Signed-off-by: Michael Pfeiffer <michael.pfeiffer@tu-ilmenau.de>

Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

Applied to dpdk-next-net/main, thanks.


^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2020-11-13 14:49 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-09 14:22 [dpdk-dev] [PATCH] net/tap: Allow all-zero checksum for UDP over IPv4 Michael Pfeiffer
2020-11-10 14:46 ` Ferruh Yigit
2020-11-10 15:56   ` Ferruh Yigit
2020-11-10 16:01   ` Morten Brørup
2020-11-10 17:42     ` Ferruh Yigit
2020-11-11  7:06       ` Morten Brørup
2020-11-10 15:59 ` Ferruh Yigit
2020-11-11  7:23   ` Michael Pfeiffer
2020-11-11  9:31     ` Ferruh Yigit
2020-11-13 13:02       ` Ferruh Yigit
2020-11-13 14:03         ` [dpdk-dev] [PATCH v2] " Michael Pfeiffer
2020-11-13 14:49           ` Ferruh Yigit

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).