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

[Tariq Toukan <tariqt@mellanox.com>]

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>