diff mbox series

[v3] net: fix Intel-specific Prepare the outer ipv4 hdr for checksum

Message ID 20210707094019.40945-1-mohsin.kazmi14@gmail.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series [v3] net: fix Intel-specific Prepare the outer ipv4 hdr for checksum |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/github-robot success github build: passed
ci/Intel-compilation success Compilation OK
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-testing success Testing PASS
ci/iol-abi-testing success Testing PASS
ci/intel-Testing success Testing PASS

Commit Message

Mohsin Kazmi July 7, 2021, 9:40 a.m. UTC 
  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>
---
v3:
   * Update the conditional test with PKT_TX_OUTER_IP_CKSUM.
   * Update the commit title with "Intel-specific".

v2:
   * Update the commit message with Fixes.

 lib/net/rte_net.h | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

Comments

Thomas Monjalon July 22, 2021, 7:56 p.m. UTC | #1 
+Cc more people for reviews.

07/07/2021 11:40, Mohsin Kazmi:
> 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.

nit: please write "IPv4" here and below.

> 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>
[...]
> @@ -125,11 +125,22 @@ rte_net_intel_cksum_flags_prepare(struct rte_mbuf *m, uint64_t ol_flags)
>  	 * Mainly it is required to avoid fragmented headers check if
>  	 * no offloads are requested.
>  	 */
> -	if (!(ol_flags & (PKT_TX_IP_CKSUM | PKT_TX_L4_MASK | PKT_TX_TCP_SEG)))
> +	if (!(ol_flags & (PKT_TX_IP_CKSUM | PKT_TX_L4_MASK | PKT_TX_TCP_SEG |
> +			  PKT_TX_OUTER_IP_CKSUM)))
>  		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;
> +		}
> +	}
Olivier Matz July 27, 2021, 12:52 p.m. UTC | #2 
On Thu, Jul 22, 2021 at 09:56:25PM +0200, Thomas Monjalon wrote:
> +Cc more people for reviews.
> 
> 07/07/2021 11:40, Mohsin Kazmi:
> > Preparation the headers for the hardware offload

"of" is missing after Preparation?

> > 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.
> 
> nit: please write "IPv4" here and below.
> 
> > 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>

Acked-by: Olivier Matz <olivier.matz@6wind.com>
Andrew Rybchenko July 28, 2021, 3:46 p.m. UTC | #3 
On 7/7/21 12:40 PM, Mohsin Kazmi wrote:
> 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>
> ---
> v3:
>     * Update the conditional test with PKT_TX_OUTER_IP_CKSUM.
>     * Update the commit title with "Intel-specific".
> 
> v2:
>     * Update the commit message with Fixes.
> 
>   lib/net/rte_net.h | 15 +++++++++++++--
>   1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/net/rte_net.h b/lib/net/rte_net.h
> index 434435ffa2..3f4c8c58b9 100644
> --- a/lib/net/rte_net.h
> +++ b/lib/net/rte_net.h
> @@ -125,11 +125,22 @@ rte_net_intel_cksum_flags_prepare(struct rte_mbuf *m, uint64_t ol_flags)
>   	 * Mainly it is required to avoid fragmented headers check if
>   	 * no offloads are requested.
>   	 */
> -	if (!(ol_flags & (PKT_TX_IP_CKSUM | PKT_TX_L4_MASK | PKT_TX_TCP_SEG)))
> +	if (!(ol_flags & (PKT_TX_IP_CKSUM | PKT_TX_L4_MASK | PKT_TX_TCP_SEG |
> +			  PKT_TX_OUTER_IP_CKSUM)))
>   		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;

Here we assume that the field is located in the first segment.
Unlikely but it still could be false. We must handle it properly.

> +		}
> +	}
>   
>   	/*
>   	 * Check if headers are fragmented.
>
Olivier Matz July 30, 2021, 11:11 a.m. UTC | #4 
On Wed, Jul 28, 2021 at 06:46:53PM +0300, Andrew Rybchenko wrote:
> On 7/7/21 12:40 PM, Mohsin Kazmi wrote:
> > 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>
> > ---
> > v3:
> >     * Update the conditional test with PKT_TX_OUTER_IP_CKSUM.
> >     * Update the commit title with "Intel-specific".
> > 
> > v2:
> >     * Update the commit message with Fixes.
> > 
> >   lib/net/rte_net.h | 15 +++++++++++++--
> >   1 file changed, 13 insertions(+), 2 deletions(-)
> > 
> > diff --git a/lib/net/rte_net.h b/lib/net/rte_net.h
> > index 434435ffa2..3f4c8c58b9 100644
> > --- a/lib/net/rte_net.h
> > +++ b/lib/net/rte_net.h
> > @@ -125,11 +125,22 @@ rte_net_intel_cksum_flags_prepare(struct rte_mbuf *m, uint64_t ol_flags)
> >   	 * Mainly it is required to avoid fragmented headers check if
> >   	 * no offloads are requested.
> >   	 */
> > -	if (!(ol_flags & (PKT_TX_IP_CKSUM | PKT_TX_L4_MASK | PKT_TX_TCP_SEG)))
> > +	if (!(ol_flags & (PKT_TX_IP_CKSUM | PKT_TX_L4_MASK | PKT_TX_TCP_SEG |
> > +			  PKT_TX_OUTER_IP_CKSUM)))
> >   		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;
> 
> Here we assume that the field is located in the first segment.
> Unlikely but it still could be false. We must handle it properly.

This is specified in the API comment, so I think it has to be checked
by the caller.

> > +		}
> > +	}
> >   	/*
> >   	 * Check if headers are fragmented.
> > 
>
Andrew Rybchenko July 31, 2021, 12:49 p.m. UTC | #5 
On 7/30/21 2:11 PM, Olivier Matz wrote:
> On Wed, Jul 28, 2021 at 06:46:53PM +0300, Andrew Rybchenko wrote:
>> On 7/7/21 12:40 PM, Mohsin Kazmi wrote:
>>> 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>
>>> ---
>>> v3:
>>>      * Update the conditional test with PKT_TX_OUTER_IP_CKSUM.
>>>      * Update the commit title with "Intel-specific".
>>>
>>> v2:
>>>      * Update the commit message with Fixes.
>>>
>>>    lib/net/rte_net.h | 15 +++++++++++++--
>>>    1 file changed, 13 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/lib/net/rte_net.h b/lib/net/rte_net.h
>>> index 434435ffa2..3f4c8c58b9 100644
>>> --- a/lib/net/rte_net.h
>>> +++ b/lib/net/rte_net.h
>>> @@ -125,11 +125,22 @@ rte_net_intel_cksum_flags_prepare(struct rte_mbuf *m, uint64_t ol_flags)
>>>    	 * Mainly it is required to avoid fragmented headers check if
>>>    	 * no offloads are requested.
>>>    	 */
>>> -	if (!(ol_flags & (PKT_TX_IP_CKSUM | PKT_TX_L4_MASK | PKT_TX_TCP_SEG)))
>>> +	if (!(ol_flags & (PKT_TX_IP_CKSUM | PKT_TX_L4_MASK | PKT_TX_TCP_SEG |
>>> +			  PKT_TX_OUTER_IP_CKSUM)))
>>>    		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;
>>
>> Here we assume that the field is located in the first segment.
>> Unlikely but it still could be false. We must handle it properly.
> 
> This is specified in the API comment, so I think it has to be checked
> by the caller.

If no, what's the point to spoil memory here if stricter check is
done few lines below.

>>> +		}
>>> +	}
>>>    	/*
>>>    	 * Check if headers are fragmented.
>>>
>>
Mohsin Kazmi Aug. 3, 2021, 12:49 p.m. UTC | #6 
On Sat, Jul 31, 2021 at 1:49 PM Andrew Rybchenko <
andrew.rybchenko@oktetlabs.ru> wrote:

> On 7/30/21 2:11 PM, Olivier Matz wrote:
> > On Wed, Jul 28, 2021 at 06:46:53PM +0300, Andrew Rybchenko wrote:
> >> On 7/7/21 12:40 PM, Mohsin Kazmi wrote:
> >>> 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>
> >>> ---
> >>> v3:
> >>>      * Update the conditional test with PKT_TX_OUTER_IP_CKSUM.
> >>>      * Update the commit title with "Intel-specific".
> >>>
> >>> v2:
> >>>      * Update the commit message with Fixes.
> >>>
> >>>    lib/net/rte_net.h | 15 +++++++++++++--
> >>>    1 file changed, 13 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/lib/net/rte_net.h b/lib/net/rte_net.h
> >>> index 434435ffa2..3f4c8c58b9 100644
> >>> --- a/lib/net/rte_net.h
> >>> +++ b/lib/net/rte_net.h
> >>> @@ -125,11 +125,22 @@ rte_net_intel_cksum_flags_prepare(struct
> rte_mbuf *m, uint64_t ol_flags)
> >>>      * Mainly it is required to avoid fragmented headers check if
> >>>      * no offloads are requested.
> >>>      */
> >>> -   if (!(ol_flags & (PKT_TX_IP_CKSUM | PKT_TX_L4_MASK |
> PKT_TX_TCP_SEG)))
> >>> +   if (!(ol_flags & (PKT_TX_IP_CKSUM | PKT_TX_L4_MASK |
> PKT_TX_TCP_SEG |
> >>> +                     PKT_TX_OUTER_IP_CKSUM)))
> >>>             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;
> >>
> >> Here we assume that the field is located in the first segment.
> >> Unlikely but it still could be false. We must handle it properly.
> >
> > This is specified in the API comment, so I think it has to be checked
> > by the caller.
>
> If no, what's the point to spoil memory here if stricter check is
> done few lines below.
>
We have two possibilities:
1) take the whole block of above code after the strict check: Then strict
check will use m->outer_l2_len + m->outer_l3_len directly without any
condition and we will be on the mercy of drivers to initialize these to 0
if outer headers are not use. Drivers usually don't set the fields which
they are not interested in because of performance reasons as
setting these values per packet will cost them additional cycles.
2) Taking just PKT_TX_OUTER_IP_CKSUM conditional check after the strict
fragmented check: In that case, each packet will hit an extra conditional
check without getting benefit from it, again with a performance penalty.

I am more inclined towards solution 1. But I also welcome other
suggestions/comments.

>
> >>> +           }
> >>> +   }
> >>>     /*
> >>>      * Check if headers are fragmented.
> >>>
> >>
>
>
Mohsin Kazmi Aug. 27, 2021, 1:44 p.m. UTC | #7 
Are we good with this patch with the current state? @Olivier: Any comments
on the above suggestions?

On Tue, Aug 3, 2021 at 1:49 PM Mohsin Kazmi <mohsin.kazmi14@gmail.com>
wrote:

>
>
> On Sat, Jul 31, 2021 at 1:49 PM Andrew Rybchenko <
> andrew.rybchenko@oktetlabs.ru> wrote:
>
>> On 7/30/21 2:11 PM, Olivier Matz wrote:
>> > On Wed, Jul 28, 2021 at 06:46:53PM +0300, Andrew Rybchenko wrote:
>> >> On 7/7/21 12:40 PM, Mohsin Kazmi wrote:
>> >>> 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>
>> >>> ---
>> >>> v3:
>> >>>      * Update the conditional test with PKT_TX_OUTER_IP_CKSUM.
>> >>>      * Update the commit title with "Intel-specific".
>> >>>
>> >>> v2:
>> >>>      * Update the commit message with Fixes.
>> >>>
>> >>>    lib/net/rte_net.h | 15 +++++++++++++--
>> >>>    1 file changed, 13 insertions(+), 2 deletions(-)
>> >>>
>> >>> diff --git a/lib/net/rte_net.h b/lib/net/rte_net.h
>> >>> index 434435ffa2..3f4c8c58b9 100644
>> >>> --- a/lib/net/rte_net.h
>> >>> +++ b/lib/net/rte_net.h
>> >>> @@ -125,11 +125,22 @@ rte_net_intel_cksum_flags_prepare(struct
>> rte_mbuf *m, uint64_t ol_flags)
>> >>>      * Mainly it is required to avoid fragmented headers check if
>> >>>      * no offloads are requested.
>> >>>      */
>> >>> -   if (!(ol_flags & (PKT_TX_IP_CKSUM | PKT_TX_L4_MASK |
>> PKT_TX_TCP_SEG)))
>> >>> +   if (!(ol_flags & (PKT_TX_IP_CKSUM | PKT_TX_L4_MASK |
>> PKT_TX_TCP_SEG |
>> >>> +                     PKT_TX_OUTER_IP_CKSUM)))
>> >>>             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;
>> >>
>> >> Here we assume that the field is located in the first segment.
>> >> Unlikely but it still could be false. We must handle it properly.
>> >
>> > This is specified in the API comment, so I think it has to be checked
>> > by the caller.
>>
>> If no, what's the point to spoil memory here if stricter check is
>> done few lines below.
>>
> We have two possibilities:
> 1) take the whole block of above code after the strict check: Then strict
> check will use m->outer_l2_len + m->outer_l3_len directly without any
> condition and we will be on the mercy of drivers to initialize these to 0
> if outer headers are not use. Drivers usually don't set the fields which
> they are not interested in because of performance reasons as
> setting these values per packet will cost them additional cycles.
> 2) Taking just PKT_TX_OUTER_IP_CKSUM conditional check after the strict
> fragmented check: In that case, each packet will hit an extra conditional
> check without getting benefit from it, again with a performance penalty.
>
> I am more inclined towards solution 1. But I also welcome other
> suggestions/comments.
>
>>
>> >>> +           }
>> >>> +   }
>> >>>     /*
>> >>>      * Check if headers are fragmented.
>> >>>
>> >>
>>
>>
diff mbox series

Patch

diff --git a/lib/net/rte_net.h b/lib/net/rte_net.h
index 434435ffa2..3f4c8c58b9 100644
--- a/lib/net/rte_net.h
+++ b/lib/net/rte_net.h
@@ -125,11 +125,22 @@  rte_net_intel_cksum_flags_prepare(struct rte_mbuf *m, uint64_t ol_flags)
 	 * Mainly it is required to avoid fragmented headers check if
 	 * no offloads are requested.
 	 */
-	if (!(ol_flags & (PKT_TX_IP_CKSUM | PKT_TX_L4_MASK | PKT_TX_TCP_SEG)))
+	if (!(ol_flags & (PKT_TX_IP_CKSUM | PKT_TX_L4_MASK | PKT_TX_TCP_SEG |
+			  PKT_TX_OUTER_IP_CKSUM)))
 		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.