[PATCH net-next] net/mlx4_en: optimizes get_fixed_ipv6_csum()

[PATCH net-next] net/mlx4_en: optimizes get_fixed_ipv6_csum()

[Eric Dumazet]

While trying to support CHECKSUM_COMPLETE for IPV6 fragments,
I had to experiments various hacks in get_fixed_ipv6_csum().
I must admit I could not find how to implement this :/

However, get_fixed_ipv6_csum() does a lot of redundant operations,
calling csum_partial() twice.

First csum_partial() computes the checksum of saddr and daddr,
put in @csum_pseudo_hdr. Undone later in the second csum_partial()
computed on whole ipv6 header.

Then nexthdr is added once, added a second time, then substracted.

payload_len is added once, then substracted.

Really all this can be reduced to two add_csum(), to add back 6 bytes
that were removed by mlx4 when providing hw_checksum in RX descriptor.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Saeed Mahameed <saeedm@mellanox.com>
Cc: Tariq Toukan <tariqt@mellanox.com>
---
Note: This patch, like other mlx4 patches can definitely wait
Tariq approval, thanks !

 drivers/net/ethernet/mellanox/mlx4/en_rx.c | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_rx.c b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
index 5c613c6663da51a4ae792eeb4d8956b54655786b..38c56fb6e5f5970f245dd56c38e1fc63a9349a07 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_rx.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
@@ -593,30 +593,25 @@ static int get_fixed_ipv4_csum(__wsum hw_checksum, struct sk_buff *skb,
 }
 
 #if IS_ENABLED(CONFIG_IPV6)
-/* In IPv6 packets, besides subtracting the pseudo header checksum,
- * we also compute/add the IP header checksum which
- * is not added by the HW.
+/* In IPv6 packets, hw_checksum lacks 6 bytes from IPv6 header:
+ * 4 first bytes : priority, version, flow_lbl
+ * and 2 additional bytes : nexthdr, hop_limit.
  */
 static int get_fixed_ipv6_csum(__wsum hw_checksum, struct sk_buff *skb,
 			       struct ipv6hdr *ipv6h)
 {
 	__u8 nexthdr = ipv6h->nexthdr;
-	__wsum csum_pseudo_hdr = 0;
+	__wsum temp;
 
 	if (unlikely(nexthdr == IPPROTO_FRAGMENT ||
 		     nexthdr == IPPROTO_HOPOPTS ||
 		     nexthdr == IPPROTO_SCTP))
 		return -1;
-	hw_checksum = csum_add(hw_checksum, (__force __wsum)htons(nexthdr));
 
-	csum_pseudo_hdr = csum_partial(&ipv6h->saddr,
-				       sizeof(ipv6h->saddr) + sizeof(ipv6h->daddr), 0);
-	csum_pseudo_hdr = csum_add(csum_pseudo_hdr, (__force __wsum)ipv6h->payload_len);
-	csum_pseudo_hdr = csum_add(csum_pseudo_hdr,
-				   (__force __wsum)htons(nexthdr));
-
-	skb->csum = csum_sub(hw_checksum, csum_pseudo_hdr);
-	skb->csum = csum_add(skb->csum, csum_partial(ipv6h, sizeof(struct ipv6hdr), 0));
+	/* priority, version, flow_lbl */
+	temp = csum_add(hw_checksum, *(__wsum *)ipv6h);
+	/* nexthdr and hop_limit */
+	skb->csum = csum_add(temp, (__force __wsum)*(__be16 *)&ipv6h->nexthdr);
 	return 0;
 }
 #endif
-- 
2.17.0.484.g0c8726318c-goog

Re: [PATCH net-next] net/mlx4_en: optimizes get_fixed_ipv6_csum()

[Saeed Mahameed]

On Thu, 2018-04-19 at 08:49 -0700, Eric Dumazet wrote:
> While trying to support CHECKSUM_COMPLETE for IPV6 fragments,
> I had to experiments various hacks in get_fixed_ipv6_csum().
> I must admit I could not find how to implement this :/
> 
> However, get_fixed_ipv6_csum() does a lot of redundant operations,
> calling csum_partial() twice.
> 
> First csum_partial() computes the checksum of saddr and daddr,
> put in @csum_pseudo_hdr. Undone later in the second csum_partial()
> computed on whole ipv6 header.
> 
> Then nexthdr is added once, added a second time, then substracted.
> 
> payload_len is added once, then substracted.
> 
> Really all this can be reduced to two add_csum(), to add back 6 bytes
> that were removed by mlx4 when providing hw_checksum in RX
> descriptor.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Saeed Mahameed <saeedm@mellanox.com>
> Cc: Tariq Toukan <tariqt@mellanox.com>
> ---
> Note: This patch, like other mlx4 patches can definitely wait
> Tariq approval, thanks !
> 

LGTM,

Reviewed-by: Saeed Mahameed <saeedm@mellanox.com>

>  drivers/net/ethernet/mellanox/mlx4/en_rx.c | 21 ++++++++----------
> ---
>  1 file changed, 8 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_rx.c
> b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
> index
> 5c613c6663da51a4ae792eeb4d8956b54655786b..38c56fb6e5f5970f245dd56c38e
> 1fc63a9349a07 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/en_rx.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
> @@ -593,30 +593,25 @@ static int get_fixed_ipv4_csum(__wsum
> hw_checksum, struct sk_buff *skb,
>  }
>  
>  #if IS_ENABLED(CONFIG_IPV6)
> -/* In IPv6 packets, besides subtracting the pseudo header checksum,
> - * we also compute/add the IP header checksum which
> - * is not added by the HW.
> +/* In IPv6 packets, hw_checksum lacks 6 bytes from IPv6 header:
> + * 4 first bytes : priority, version, flow_lbl
> + * and 2 additional bytes : nexthdr, hop_limit.
>   */
>  static int get_fixed_ipv6_csum(__wsum hw_checksum, struct sk_buff
> *skb,
>  			       struct ipv6hdr *ipv6h)
>  {
>  	__u8 nexthdr = ipv6h->nexthdr;
> -	__wsum csum_pseudo_hdr = 0;
> +	__wsum temp;
>  
>  	if (unlikely(nexthdr == IPPROTO_FRAGMENT ||
>  		     nexthdr == IPPROTO_HOPOPTS ||
>  		     nexthdr == IPPROTO_SCTP))
>  		return -1;
> -	hw_checksum = csum_add(hw_checksum, (__force
> __wsum)htons(nexthdr));
>  
> -	csum_pseudo_hdr = csum_partial(&ipv6h->saddr,
> -				       sizeof(ipv6h->saddr) +
> sizeof(ipv6h->daddr), 0);
> -	csum_pseudo_hdr = csum_add(csum_pseudo_hdr, (__force
> __wsum)ipv6h->payload_len);
> -	csum_pseudo_hdr = csum_add(csum_pseudo_hdr,
> -				   (__force __wsum)htons(nexthdr));
> -
> -	skb->csum = csum_sub(hw_checksum, csum_pseudo_hdr);
> -	skb->csum = csum_add(skb->csum, csum_partial(ipv6h,
> sizeof(struct ipv6hdr), 0));
> +	/* priority, version, flow_lbl */
> +	temp = csum_add(hw_checksum, *(__wsum *)ipv6h);
> +	/* nexthdr and hop_limit */
> +	skb->csum = csum_add(temp, (__force __wsum)*(__be16
> *)&ipv6h->nexthdr);
>  	return 0;
>  }
>  #endif

Re: [PATCH net-next] net/mlx4_en: optimizes get_fixed_ipv6_csum()

[Tariq Toukan]

On 27/04/2018 1:56 AM, Saeed Mahameed wrote:
> On Thu, 2018-04-19 at 08:49 -0700, Eric Dumazet wrote:
>> While trying to support CHECKSUM_COMPLETE for IPV6 fragments,
>> I had to experiments various hacks in get_fixed_ipv6_csum().
>> I must admit I could not find how to implement this :/
>>
>> However, get_fixed_ipv6_csum() does a lot of redundant operations,
>> calling csum_partial() twice.
>>
>> First csum_partial() computes the checksum of saddr and daddr,
>> put in @csum_pseudo_hdr. Undone later in the second csum_partial()
>> computed on whole ipv6 header.
>>
>> Then nexthdr is added once, added a second time, then substracted.
>>
>> payload_len is added once, then substracted.
>>
>> Really all this can be reduced to two add_csum(), to add back 6 bytes
>> that were removed by mlx4 when providing hw_checksum in RX
>> descriptor.
>>
>> Signed-off-by: Eric Dumazet <edumazet@google.com>
>> Cc: Saeed Mahameed <saeedm@mellanox.com>
>> Cc: Tariq Toukan <tariqt@mellanox.com>
>> ---
>> Note: This patch, like other mlx4 patches can definitely wait
>> Tariq approval, thanks !
>>
> 
> LGTM,
> 
> Reviewed-by: Saeed Mahameed <saeedm@mellanox.com>
> 

Acked-by: Tariq Toukan <tariqt@mellanox.com>

Thanks Eric.

>>   drivers/net/ethernet/mellanox/mlx4/en_rx.c | 21 ++++++++----------
>> ---
>>   1 file changed, 8 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_rx.c
>> b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
>> index
>> 5c613c6663da51a4ae792eeb4d8956b54655786b..38c56fb6e5f5970f245dd56c38e
>> 1fc63a9349a07 100644
>> --- a/drivers/net/ethernet/mellanox/mlx4/en_rx.c
>> +++ b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
>> @@ -593,30 +593,25 @@ static int get_fixed_ipv4_csum(__wsum
>> hw_checksum, struct sk_buff *skb,
>>   }
>>   
>>   #if IS_ENABLED(CONFIG_IPV6)
>> -/* In IPv6 packets, besides subtracting the pseudo header checksum,
>> - * we also compute/add the IP header checksum which
>> - * is not added by the HW.
>> +/* In IPv6 packets, hw_checksum lacks 6 bytes from IPv6 header:
>> + * 4 first bytes : priority, version, flow_lbl
>> + * and 2 additional bytes : nexthdr, hop_limit.
>>    */
>>   static int get_fixed_ipv6_csum(__wsum hw_checksum, struct sk_buff
>> *skb,
>>   			       struct ipv6hdr *ipv6h)
>>   {
>>   	__u8 nexthdr = ipv6h->nexthdr;
>> -	__wsum csum_pseudo_hdr = 0;
>> +	__wsum temp;
>>   
>>   	if (unlikely(nexthdr == IPPROTO_FRAGMENT ||
>>   		     nexthdr == IPPROTO_HOPOPTS ||
>>   		     nexthdr == IPPROTO_SCTP))
>>   		return -1;
>> -	hw_checksum = csum_add(hw_checksum, (__force
>> __wsum)htons(nexthdr));
>>   
>> -	csum_pseudo_hdr = csum_partial(&ipv6h->saddr,
>> -				       sizeof(ipv6h->saddr) +
>> sizeof(ipv6h->daddr), 0);
>> -	csum_pseudo_hdr = csum_add(csum_pseudo_hdr, (__force
>> __wsum)ipv6h->payload_len);
>> -	csum_pseudo_hdr = csum_add(csum_pseudo_hdr,
>> -				   (__force __wsum)htons(nexthdr));
>> -
>> -	skb->csum = csum_sub(hw_checksum, csum_pseudo_hdr);
>> -	skb->csum = csum_add(skb->csum, csum_partial(ipv6h,
>> sizeof(struct ipv6hdr), 0));
>> +	/* priority, version, flow_lbl */
>> +	temp = csum_add(hw_checksum, *(__wsum *)ipv6h);
>> +	/* nexthdr and hop_limit */
>> +	skb->csum = csum_add(temp, (__force __wsum)*(__be16
>> *)&ipv6h->nexthdr);
>>   	return 0;
>>   }
>>   #endif

Re: [PATCH net-next] net/mlx4_en: optimizes get_fixed_ipv6_csum()

[Eric Dumazet]

On 05/02/2018 07:18 AM, Tariq Toukan wrote:
> 
> 
> On 27/04/2018 1:56 AM, Saeed Mahameed wrote:

>> LGTM,
>>
>> Reviewed-by: Saeed Mahameed <saeedm@mellanox.com>
>>
> 
> Acked-by: Tariq Toukan <tariqt@mellanox.com>
> 
> Thanks Eric.

Thanks guys. 

I see this patch  ( http://patchwork.ozlabs.org/patch/901336/ ) in
a state I do not know : "Awaiting Upstream"

Re: [PATCH net-next] net/mlx4_en: optimizes get_fixed_ipv6_csum()

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 3 May 2018 17:05:06 -0700

> 
> 
> On 05/02/2018 07:18 AM, Tariq Toukan wrote:
>> 
>> 
>> On 27/04/2018 1:56 AM, Saeed Mahameed wrote:
> 
>>> LGTM,
>>>
>>> Reviewed-by: Saeed Mahameed <saeedm@mellanox.com>
>>>
>> 
>> Acked-by: Tariq Toukan <tariqt@mellanox.com>
>> 
>> Thanks Eric.
> 
> Thanks guys. 
> 
> I see this patch  ( http://patchwork.ozlabs.org/patch/901336/ ) in
> a state I do not know : "Awaiting Upstream"

THat means I expect to see this change from the upstream
maintainer, which in this case is Tariq.

Re: [PATCH net-next] net/mlx4_en: optimizes get_fixed_ipv6_csum()

[Eric Dumazet]

On 05/03/2018 06:52 PM, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Thu, 3 May 2018 17:05:06 -0700
> 
>>
>>
>> On 05/02/2018 07:18 AM, Tariq Toukan wrote:
>>>
>>>
>>> On 27/04/2018 1:56 AM, Saeed Mahameed wrote:
>>
>>>> LGTM,
>>>>
>>>> Reviewed-by: Saeed Mahameed <saeedm@mellanox.com>
>>>>
>>>
>>> Acked-by: Tariq Toukan <tariqt@mellanox.com>
>>>
>>> Thanks Eric.
>>
>> Thanks guys. 
>>
>> I see this patch  ( http://patchwork.ozlabs.org/patch/901336/ ) in
>> a state I do not know : "Awaiting Upstream"
> 
> THat means I expect to see this change from the upstream
> maintainer, which in this case is Tariq.
> 

I see, but it seems Tariq does not know that, otherwise he would
not have sent an "Acked-by:"

I guess this will need an extra round-trip ...

Re: [PATCH net-next] net/mlx4_en: optimizes get_fixed_ipv6_csum()

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 3 May 2018 19:10:29 -0700

> 
> 
> On 05/03/2018 06:52 PM, David Miller wrote:
>> From: Eric Dumazet <eric.dumazet@gmail.com>
>> Date: Thu, 3 May 2018 17:05:06 -0700
>> 
>>>
>>>
>>> On 05/02/2018 07:18 AM, Tariq Toukan wrote:
>>>>
>>>>
>>>> On 27/04/2018 1:56 AM, Saeed Mahameed wrote:
>>>
>>>>> LGTM,
>>>>>
>>>>> Reviewed-by: Saeed Mahameed <saeedm@mellanox.com>
>>>>>
>>>>
>>>> Acked-by: Tariq Toukan <tariqt@mellanox.com>
>>>>
>>>> Thanks Eric.
>>>
>>> Thanks guys. 
>>>
>>> I see this patch  ( http://patchwork.ozlabs.org/patch/901336/ ) in
>>> a state I do not know : "Awaiting Upstream"
>> 
>> THat means I expect to see this change from the upstream
>> maintainer, which in this case is Tariq.
>> 
> 
> I see, but it seems Tariq does not know that, otherwise he would
> not have sent an "Acked-by:"
> 
> I guess this will need an extra round-trip ...

No need to extra round-trip, I applied it directly ;)

Re: [PATCH net-next] net/mlx4_en: optimizes get_fixed_ipv6_csum()

[Eric Dumazet]

On 05/04/2018 08:59 AM, David Miller wrote:

> 
> No need to extra round-trip, I applied it directly ;)
> 

Very nice, thanks David !