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