[PATCH net-next] net/mlx4_en: CHECKSUM_COMPLETE support for fragments

[PATCH net-next] net/mlx4_en: CHECKSUM_COMPLETE support for fragments

[Eric Dumazet]

Refine the RX check summing handling to propagate the
hardware provided checksum so that we do not have to
compute it later in software.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Willem de Bruijn <willemb@google.com>
Cc: Tariq Toukan <tariqt@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlx4/en_rx.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_rx.c b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
index 05787efef492b1c0c6ce540ef73647fad91ce282..5c613c6663da51a4ae792eeb4d8956b54655786b 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_rx.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
@@ -821,14 +821,12 @@ int mlx4_en_process_rx_cq(struct net_device *dev, struct mlx4_en_cq *cq, int bud
 		skb_record_rx_queue(skb, cq_ring);
 
 		if (likely(dev->features & NETIF_F_RXCSUM)) {
-			if (cqe->status & cpu_to_be16(MLX4_CQE_STATUS_TCP |
-						      MLX4_CQE_STATUS_UDP)) {
+			if ((cqe->status & cpu_to_be16(MLX4_CQE_STATUS_TCP |
+						       MLX4_CQE_STATUS_UDP)) &&
+			    (cqe->status & cpu_to_be16(MLX4_CQE_STATUS_IPOK)) &&
+			    cqe->checksum == cpu_to_be16(0xffff)) {
 				bool l2_tunnel;
 
-				if (!((cqe->status & cpu_to_be16(MLX4_CQE_STATUS_IPOK)) &&
-				      cqe->checksum == cpu_to_be16(0xffff)))
-					goto csum_none;
-
 				l2_tunnel = (dev->hw_enc_features & NETIF_F_RXCSUM) &&
 					(cqe->vlan_my_qpn & cpu_to_be32(MLX4_CQE_L2_TUNNEL));
 				ip_summed = CHECKSUM_UNNECESSARY;
-- 
2.17.0.rc1.321.gba9d0f2565-goog

Re: [PATCH net-next] net/mlx4_en: CHECKSUM_COMPLETE support for fragments

[David Miller]

From: Eric Dumazet <edumazet@google.com>
Date: Tue, 27 Mar 2018 14:21:14 -0700

> Refine the RX check summing handling to propagate the
> hardware provided checksum so that we do not have to
> compute it later in software.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Tariq, please review.

Re: [PATCH net-next] net/mlx4_en: CHECKSUM_COMPLETE support for fragments

[Saeed Mahameed]

On Tue, Mar 27, 2018 at 2:21 PM, Eric Dumazet <edumazet@google.com> wrote:
> Refine the RX check summing handling to propagate the
> hardware provided checksum so that we do not have to
> compute it later in software.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Willem de Bruijn <willemb@google.com>
> Cc: Tariq Toukan <tariqt@mellanox.com>
> ---
>  drivers/net/ethernet/mellanox/mlx4/en_rx.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_rx.c b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
> index 05787efef492b1c0c6ce540ef73647fad91ce282..5c613c6663da51a4ae792eeb4d8956b54655786b 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/en_rx.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
> @@ -821,14 +821,12 @@ int mlx4_en_process_rx_cq(struct net_device *dev, struct mlx4_en_cq *cq, int bud
>                 skb_record_rx_queue(skb, cq_ring);
>
>                 if (likely(dev->features & NETIF_F_RXCSUM)) {
> -                       if (cqe->status & cpu_to_be16(MLX4_CQE_STATUS_TCP |
> -                                                     MLX4_CQE_STATUS_UDP)) {
> +                       if ((cqe->status & cpu_to_be16(MLX4_CQE_STATUS_TCP |
> +                                                      MLX4_CQE_STATUS_UDP)) &&
> +                           (cqe->status & cpu_to_be16(MLX4_CQE_STATUS_IPOK)) &&
> +                           cqe->checksum == cpu_to_be16(0xffff)) {
>                                 bool l2_tunnel;
>

LGTM, this code even aligns better with the mlx4 HW documentation:

"When L4_CSUM field is not supported, L4 checksum for TCP/UDP packets
can be validated by: (IP_OK && (TCP || UDP)) && (checksum ==
0xFFFF))."

in the code we don't even consider L4_CSUM at the moment, As a future
patch, it could be a nice acceleration for the above 3 steps
condition.

Small comment, if we expect that  cqe->checksum is NOT likely to be
0xffff for UDP/TCP packets, maybe it is better performance wise to
move (cqe->checksum == cpu_to_be16(0xffff)) to be evaluated first in
the condition.

> -                               if (!((cqe->status & cpu_to_be16(MLX4_CQE_STATUS_IPOK)) &&
> -                                     cqe->checksum == cpu_to_be16(0xffff)))
> -                                       goto csum_none;
> -
>                                 l2_tunnel = (dev->hw_enc_features & NETIF_F_RXCSUM) &&
>                                         (cqe->vlan_my_qpn & cpu_to_be32(MLX4_CQE_L2_TUNNEL));
>                                 ip_summed = CHECKSUM_UNNECESSARY;
> --
> 2.17.0.rc1.321.gba9d0f2565-goog
>

Re: [PATCH net-next] net/mlx4_en: CHECKSUM_COMPLETE support for fragments

[Saeed Mahameed]

On Thu, Mar 29, 2018 at 11:07 AM, David Miller <davem@davemloft.net> wrote:
> From: Eric Dumazet <edumazet@google.com>
> Date: Tue, 27 Mar 2018 14:21:14 -0700
>
>> Refine the RX check summing handling to propagate the
>> hardware provided checksum so that we do not have to
>> compute it later in software.
>>
>> Signed-off-by: Eric Dumazet <edumazet@google.com>
>
> Tariq, please review.

Hi Dave, Eric.

The patch looks ok but i would let tariq review it and decide if he
wants to run full regression coverage on it
since it changes the default behavior of the driver's checksum reporting.

It is already weekend for him and for the team in Israel, and i don't
think this can be handled before next week :).
So it is really up to you guys.

Thanks,
Saeed.

Re: [PATCH net-next] net/mlx4_en: CHECKSUM_COMPLETE support for fragments

[Eric Dumazet]

On Thu, Mar 29, 2018 at 5:44 PM Saeed Mahameed <saeedm@dev.mellanox.co.il>
wrote:

> On Thu, Mar 29, 2018 at 11:07 AM, David Miller <davem@davemloft.net>
wrote:
> > From: Eric Dumazet <edumazet@google.com>
> > Date: Tue, 27 Mar 2018 14:21:14 -0700
> >
> >> Refine the RX check summing handling to propagate the
> >> hardware provided checksum so that we do not have to
> >> compute it later in software.
> >>
> >> Signed-off-by: Eric Dumazet <edumazet@google.com>
> >
> > Tariq, please review.

> Hi Dave, Eric.

> The patch looks ok but i would let tariq review it and decide if he
> wants to run full regression coverage on it
> since it changes the default behavior of the driver's checksum reporting.

> It is already weekend for him and for the team in Israel, and i don't
> think this can be handled before next week :).
> So it is really up to you guys.

> Thanks,
> Saeed.


Hi Saaed

This definitely can wait, nothing urgent really.

Thanks.

Re: [PATCH net-next] net/mlx4_en: CHECKSUM_COMPLETE support for fragments

[Tariq Toukan]

On 30/03/2018 3:34 AM, Saeed Mahameed wrote:
> On Tue, Mar 27, 2018 at 2:21 PM, Eric Dumazet <edumazet@google.com> wrote:
>> Refine the RX check summing handling to propagate the
>> hardware provided checksum so that we do not have to
>> compute it later in software.
>>
>> Signed-off-by: Eric Dumazet <edumazet@google.com>
>> Cc: Willem de Bruijn <willemb@google.com>
>> Cc: Tariq Toukan <tariqt@mellanox.com>
>> ---
>>   drivers/net/ethernet/mellanox/mlx4/en_rx.c | 10 ++++------
>>   1 file changed, 4 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_rx.c b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
>> index 05787efef492b1c0c6ce540ef73647fad91ce282..5c613c6663da51a4ae792eeb4d8956b54655786b 100644
>> --- a/drivers/net/ethernet/mellanox/mlx4/en_rx.c
>> +++ b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
>> @@ -821,14 +821,12 @@ int mlx4_en_process_rx_cq(struct net_device *dev, struct mlx4_en_cq *cq, int bud
>>                  skb_record_rx_queue(skb, cq_ring);
>>
>>                  if (likely(dev->features & NETIF_F_RXCSUM)) {
>> -                       if (cqe->status & cpu_to_be16(MLX4_CQE_STATUS_TCP |
>> -                                                     MLX4_CQE_STATUS_UDP)) {
>> +                       if ((cqe->status & cpu_to_be16(MLX4_CQE_STATUS_TCP |
>> +                                                      MLX4_CQE_STATUS_UDP)) &&
>> +                           (cqe->status & cpu_to_be16(MLX4_CQE_STATUS_IPOK)) &&
>> +                           cqe->checksum == cpu_to_be16(0xffff)) {
>>                                  bool l2_tunnel;
>>
> 
> LGTM, this code even aligns better with the mlx4 HW documentation:
> 
> "When L4_CSUM field is not supported, L4 checksum for TCP/UDP packets
> can be validated by: (IP_OK && (TCP || UDP)) && (checksum ==
> 0xFFFF))."
> 

Hi,
Thanks Eric for your patch.
Thanks Saeed for your review while I'm off-work.

In IP fragments, MLX4_CQE_STATUS_IPOK is not set in cqe->status.
This patch suggests falling back to CHECKSUM_COMPLETE instead of 
CHECKSUM_NONE.
Looks good to me.

> in the code we don't even consider L4_CSUM at the moment, As a future
> patch, it could be a nice acceleration for the above 3 steps
> condition.
> 

Right. Good idea. I'll add it to plans.

> Small comment, if we expect that  cqe->checksum is NOT likely to be
> 0xffff for UDP/TCP packets, maybe it is better performance wise to
> move (cqe->checksum == cpu_to_be16(0xffff)) to be evaluated first in
> the condition.
> 

It _is_ likely  for common TCP/UDP (no IP fragments).
Let's keep it this way.

>> -                               if (!((cqe->status & cpu_to_be16(MLX4_CQE_STATUS_IPOK)) &&
>> -                                     cqe->checksum == cpu_to_be16(0xffff)))
>> -                                       goto csum_none;
>> -
>>                                  l2_tunnel = (dev->hw_enc_features & NETIF_F_RXCSUM) &&
>>                                          (cqe->vlan_my_qpn & cpu_to_be32(MLX4_CQE_L2_TUNNEL));
>>                                  ip_summed = CHECKSUM_UNNECESSARY;
>> --
>> 2.17.0.rc1.321.gba9d0f2565-goog
>>

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

Re: [PATCH net-next] net/mlx4_en: CHECKSUM_COMPLETE support for fragments

[Eric Dumazet]

On 04/01/2018 01:06 AM, Tariq Toukan wrote:

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

Thanks Tariq and Saeed for the review !

Re: [PATCH net-next] net/mlx4_en: CHECKSUM_COMPLETE support for fragments

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Sun, 1 Apr 2018 09:39:38 -0700

> On 04/01/2018 01:06 AM, Tariq Toukan wrote:
> 
>> 
>> Acked-by: Tariq Toukan <tariqt@mellanox.com>
> 
> Thanks Tariq and Saeed for the review !

Applied, thanks everyone.