Message ID | 20210630110404.21209-1-mohsin.kazmi14@gmail.com (mailing list archive) |
---|---|
State | Superseded, archived |
Delegated to: | Thomas Monjalon |
Headers | show |
Series | [v2] net: prepare the outer ipv4 hdr for checksum | expand |
Context | Check | Description |
---|---|---|
ci/iol-abi-testing | success | Testing PASS |
ci/intel-Testing | success | Testing PASS |
ci/Intel-compilation | success | Compilation OK |
ci/iol-intel-Performance | fail | Performance Testing issues |
ci/iol-mellanox-Functional | fail | Functional Testing issues |
ci/iol-testing | fail | Testing issues |
ci/iol-intel-Functional | success | Functional Testing PASS |
ci/github-robot | success | github build: passed |
ci/checkpatch | warning | coding style issues |
Hi Mohsin, Hope you are fine! Please see my comments below. On Wed, Jun 30, 2021 at 01:04:04PM +0200, Mohsin Kazmi wrote: > Re: [PATCH v2] net: prepare the outer ipv4 hdr for checksum I suggest to highlight that it this is the Intel-specific tx-prepare function in the commit title. What about: net: fix Intel-specific Tx preparation for outer checksums > Preparation the headers for the hardware offload > misses the outer ipv4 checksum offload. > It results in bad checksum computed by hardware NIC. > > This patch fixes the issue by setting the outer ipv4 > checksum field to 0. > > Fixes: 4fb7e803eb1a ("ethdev: add Tx preparation") > Cc: stable@dpdk.org > > Signed-off-by: Mohsin Kazmi <mohsin.kazmi14@gmail.com> > Acked-by: Qi Zhang <qi.z.zhang@intel.com> > --- > > v2: > * Update the commit message with Fixes. > --- > lib/net/rte_net.h | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/lib/net/rte_net.h b/lib/net/rte_net.h > index 434435ffa2..e47365099e 100644 > --- a/lib/net/rte_net.h > +++ b/lib/net/rte_net.h > @@ -128,8 +128,18 @@ rte_net_intel_cksum_flags_prepare(struct rte_mbuf *m, uint64_t ol_flags) > if (!(ol_flags & (PKT_TX_IP_CKSUM | PKT_TX_L4_MASK | PKT_TX_TCP_SEG))) > return 0; I think this test should be updated too with PKT_TX_OUTER_IP_CKSUM. > > - if (ol_flags & (PKT_TX_OUTER_IPV4 | PKT_TX_OUTER_IPV6)) > + if (ol_flags & (PKT_TX_OUTER_IPV4 | PKT_TX_OUTER_IPV6)) { > inner_l3_offset += m->outer_l2_len + m->outer_l3_len; > + /* > + * prepare outer ipv4 header checksum by setting it to 0, > + * in order to be computed by hardware NICs. > + */ > + if (ol_flags & PKT_TX_OUTER_IP_CKSUM) { > + ipv4_hdr = rte_pktmbuf_mtod_offset(m, > + struct rte_ipv4_hdr *, m->outer_l2_len); > + ipv4_hdr->hdr_checksum = 0; > + } > + } What about outer L4 checksum? Does it requires the same than inner? > > /* > * Check if headers are fragmented. > -- > 2.17.1 >
Hi Olivier, Thanks for the review. Please find the comments inline below: On Wed, Jun 30, 2021 at 3:09 PM Olivier Matz <olivier.matz@6wind.com> wrote: > Hi Mohsin, > > Hope you are fine! > Please see my comments below. > > On Wed, Jun 30, 2021 at 01:04:04PM +0200, Mohsin Kazmi wrote: > > Re: [PATCH v2] net: prepare the outer ipv4 hdr for checksum > > I suggest to highlight that it this is the Intel-specific tx-prepare > function in the commit title. What about: > net: fix Intel-specific Tx preparation for outer checksums > > I'll update the commit title as suggested. > Preparation the headers for the hardware offload > > misses the outer ipv4 checksum offload. > > It results in bad checksum computed by hardware NIC. > > > > This patch fixes the issue by setting the outer ipv4 > > checksum field to 0. > > > > Fixes: 4fb7e803eb1a ("ethdev: add Tx preparation") > > Cc: stable@dpdk.org > > > > Signed-off-by: Mohsin Kazmi <mohsin.kazmi14@gmail.com> > > Acked-by: Qi Zhang <qi.z.zhang@intel.com> > > --- > > > > v2: > > * Update the commit message with Fixes. > > --- > > lib/net/rte_net.h | 12 +++++++++++- > > 1 file changed, 11 insertions(+), 1 deletion(-) > > > > diff --git a/lib/net/rte_net.h b/lib/net/rte_net.h > > index 434435ffa2..e47365099e 100644 > > --- a/lib/net/rte_net.h > > +++ b/lib/net/rte_net.h > > @@ -128,8 +128,18 @@ rte_net_intel_cksum_flags_prepare(struct rte_mbuf > *m, uint64_t ol_flags) > > if (!(ol_flags & (PKT_TX_IP_CKSUM | PKT_TX_L4_MASK | > PKT_TX_TCP_SEG))) > > return 0; > > I think this test should be updated too with PKT_TX_OUTER_IP_CKSUM. > Thanks. Yes, I'll update it too. > > > > > - if (ol_flags & (PKT_TX_OUTER_IPV4 | PKT_TX_OUTER_IPV6)) > > + if (ol_flags & (PKT_TX_OUTER_IPV4 | PKT_TX_OUTER_IPV6)) { > > inner_l3_offset += m->outer_l2_len + m->outer_l3_len; > > + /* > > + * prepare outer ipv4 header checksum by setting it to 0, > > + * in order to be computed by hardware NICs. > > + */ > > + if (ol_flags & PKT_TX_OUTER_IP_CKSUM) { > > + ipv4_hdr = rte_pktmbuf_mtod_offset(m, > > + struct rte_ipv4_hdr *, > m->outer_l2_len); > > + ipv4_hdr->hdr_checksum = 0; > > + } > > + } > > What about outer L4 checksum? Does it requires the same than inner? > I am using XL710 for my testing with i40e dpdk driver. AFAIK, It doesn't support outer l4 checksum. I am not sure if other Intel NICs support it. > > > > /* > > * Check if headers are fragmented. > > -- > > 2.17.1 > > >
07/07/2021 11:14, Mohsin Kazmi: > On Wed, Jun 30, 2021 at 3:09 PM Olivier Matz <olivier.matz@6wind.com> wrote: > > > + if (ol_flags & (PKT_TX_OUTER_IPV4 | PKT_TX_OUTER_IPV6)) { > > > inner_l3_offset += m->outer_l2_len + m->outer_l3_len; > > > + /* > > > + * prepare outer ipv4 header checksum by setting it to 0, > > > + * in order to be computed by hardware NICs. > > > + */ > > > + if (ol_flags & PKT_TX_OUTER_IP_CKSUM) { > > > + ipv4_hdr = rte_pktmbuf_mtod_offset(m, > > > + struct rte_ipv4_hdr *, > > m->outer_l2_len); > > > + ipv4_hdr->hdr_checksum = 0; > > > + } > > > + } > > > > What about outer L4 checksum? Does it requires the same than inner? > > > I am using XL710 for my testing with i40e dpdk driver. AFAIK, It doesn't > support outer l4 checksum. I am not sure if other Intel NICs support it. This function is used by a lot of drivers. Try git grep rte_net_intel_cksum_prepare I think we need more reviews on the v3. Given it is far from being a new bug, I suggest to wait the next release in order to have more feedbacks.
Hi Thomas, Thanks for the review. I did the git grep rte_net_intel_cksum_prepare and git grep PKT_TX_OUTER_UDP_CKSUM. Following are the two drivers that use the function to prepare headers for checksum which also uses the outer_udp_checksum offload within drivers. 1) Hisilicon hns3 2) Wangxun txgbe 1) has implemented its own version of functions to prepare for outer header checksum. It may benefit/impact from the change. The function "rte_net_intel_cksum_prepare" is intel specific and intel cards do not support outer l4 checksum offload. DPDK may provide a generic version of the same function which can be used in different drivers. -br Mohsin On Thu, Jul 22, 2021 at 8:53 PM Thomas Monjalon <thomas@monjalon.net> wrote: > 07/07/2021 11:14, Mohsin Kazmi: > > On Wed, Jun 30, 2021 at 3:09 PM Olivier Matz <olivier.matz@6wind.com> > wrote: > > > > + if (ol_flags & (PKT_TX_OUTER_IPV4 | PKT_TX_OUTER_IPV6)) { > > > > inner_l3_offset += m->outer_l2_len + m->outer_l3_len; > > > > + /* > > > > + * prepare outer ipv4 header checksum by setting it to > 0, > > > > + * in order to be computed by hardware NICs. > > > > + */ > > > > + if (ol_flags & PKT_TX_OUTER_IP_CKSUM) { > > > > + ipv4_hdr = rte_pktmbuf_mtod_offset(m, > > > > + struct rte_ipv4_hdr *, > > > m->outer_l2_len); > > > > + ipv4_hdr->hdr_checksum = 0; > > > > + } > > > > + } > > > > > > What about outer L4 checksum? Does it requires the same than inner? > > > > > I am using XL710 for my testing with i40e dpdk driver. AFAIK, It doesn't > > support outer l4 checksum. I am not sure if other Intel NICs support it. > > This function is used by a lot of drivers. > Try git grep rte_net_intel_cksum_prepare > > I think we need more reviews on the v3. > Given it is far from being a new bug, I suggest to wait the next release > in order to have more feedbacks. > > >
diff --git a/lib/net/rte_net.h b/lib/net/rte_net.h index 434435ffa2..e47365099e 100644 --- a/lib/net/rte_net.h +++ b/lib/net/rte_net.h @@ -128,8 +128,18 @@ rte_net_intel_cksum_flags_prepare(struct rte_mbuf *m, uint64_t ol_flags) if (!(ol_flags & (PKT_TX_IP_CKSUM | PKT_TX_L4_MASK | PKT_TX_TCP_SEG))) return 0; - if (ol_flags & (PKT_TX_OUTER_IPV4 | PKT_TX_OUTER_IPV6)) + if (ol_flags & (PKT_TX_OUTER_IPV4 | PKT_TX_OUTER_IPV6)) { inner_l3_offset += m->outer_l2_len + m->outer_l3_len; + /* + * prepare outer ipv4 header checksum by setting it to 0, + * in order to be computed by hardware NICs. + */ + if (ol_flags & PKT_TX_OUTER_IP_CKSUM) { + ipv4_hdr = rte_pktmbuf_mtod_offset(m, + struct rte_ipv4_hdr *, m->outer_l2_len); + ipv4_hdr->hdr_checksum = 0; + } + } /* * Check if headers are fragmented.