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