Re: [dpdk-dev] [PATCH] net/mlx5: add Rx checksum offload flag return bad

[Jiawei Zhu <17826875952@163.com>]

Hi，Slava

Thanks for your explain，the multiplications and divisions are in the 
TRANSPOSE，not in the rte_be_to_cpu_16. So I think use if-else directly 
could improves the performance. And the second point about the bad sum，I 
agree with you.

With best regards,
Jiawei

On 2021/3/24 12:33 上午, Slava Ovsiienko wrote:
> Hi, Jiawei
> 
>> -----Original Message-----
>> From: Jiawei Zhu <17826875952@163.com>
>> Sent: Monday, March 22, 2021 17:46
>> To: dev@dpdk.org
>> Cc: zhujiawei12@huawei.com; Matan Azrad <matan@nvidia.com>; Shahaf
>> Shuler <shahafs@nvidia.com>; Slava Ovsiienko <viacheslavo@nvidia.com>
>> Subject: [PATCH] net/mlx5: add Rx checksum offload flag return bad
>>
>> From: Jiawei Zhu <zhujiawei12@huawei.com>
>>
>> When open the rx checksum offload and receive the wrong checksum, add
>> the ol_flags return bad. And it's not best to use multiplication and division
>> here.
> 
> I'm sorry, there should be no any multiplications and divisions - the arguments
> are constants (can be determined at compilation time) and ara power of 2,
> hence compiler engages simple shifts. For example (I did rxq_cq_to_ol_flags not inline to
> get the listing for x86-64):
> 
>    29              	rxq_cq_to_ol_flags:
>    21              	/*
>    22              	 * An architecture-optimized byte swap for a 16-bit value.
>    23              	 *
>    24              	 * Do not use this function directly. The preferred function is rte_bswap16().
>    25              	 */
>    26              	static inline uint16_t rte_arch_bswap16(uint16_t _x)
>    27              	{
>    28              		uint16_t x = _x;
>    29 0034 86D6     		asm volatile ("xchgb %b[x1],%h[x2]"
>    30              			      : [x1] "=Q" (x)
>    37              	
>    38 0036 89D0     	 movl %edx,%eax
>    39 0038 6681E200 	 andw $512,%dx
>    39      02
>    40 003d 66250004 	 andw $1024,%ax
>    41 0041 0FB7D2   	 movzwl %dx,%edx
>    42 0044 0FB7C0   	 movzwl %ax,%eax
>    43 0047 48C1EA02 	 shrq $2,%rdx
>    44 004b 48C1E802 	 shrq $2,%rax
>    45 004f 09D0     	 orl %edx,%eax
>    46 0051 C3       	 ret
> 
> As we can see - there is no any mul/div and no any branches, that
> improves the performance.
> 
>>
>> Signed-off-by: Jiawei Zhu <zhujiawei12@huawei.com>
>> ---
>>   drivers/net/mlx5/mlx5_rxtx.c  | 17 ++++++++++-------
>> drivers/net/mlx5/mlx5_utils.h |  6 ------
>>   2 files changed, 10 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/net/mlx5/mlx5_rxtx.c b/drivers/net/mlx5/mlx5_rxtx.c
>> index e3ce9fd..9233af8 100644
>> --- a/drivers/net/mlx5/mlx5_rxtx.c
>> +++ b/drivers/net/mlx5/mlx5_rxtx.c
>> @@ -1325,13 +1325,16 @@ enum mlx5_txcmp_code {
>>   	uint32_t ol_flags = 0;
>>   	uint16_t flags = rte_be_to_cpu_16(cqe->hdr_type_etc);
>>
>> -	ol_flags =
>> -		TRANSPOSE(flags,
>> -			  MLX5_CQE_RX_L3_HDR_VALID,
>> -			  PKT_RX_IP_CKSUM_GOOD) |
>> -		TRANSPOSE(flags,
>> -			  MLX5_CQE_RX_L4_HDR_VALID,
>> -			  PKT_RX_L4_CKSUM_GOOD);
>> +	if (flags & MLX5_CQE_RX_L3_HDR_VALID)
>> +		ol_flags |= PKT_RX_IP_CKSUM_GOOD;
>> +	else
>> +		ol_flags |= PKT_RX_IP_CKSUM_BAD;
>> +
> 
> If MLX5_CQE_RX_L3_HDR_VALID is not set - it does not always mean the sum is bad.
> If might happen if HW just did not recognize the packet format (for example, for
> some tunnels)
> 
> With best regards,
> Slava
> 
> 
>> +	if (flags & MLX5_CQE_RX_L4_HDR_VALID)
>> +		ol_flags |= PKT_RX_IP_CKSUM_GOOD;
>> +	else
>> +		ol_flags |= PKT_RX_IP_CKSUM_BAD;
>> +
>>   	return ol_flags;
>>   }
>>
>> diff --git a/drivers/net/mlx5/mlx5_utils.h b/drivers/net/mlx5/mlx5_utils.h
>> index 7a62187..2f71a23 100644
>> --- a/drivers/net/mlx5/mlx5_utils.h
>> +++ b/drivers/net/mlx5/mlx5_utils.h
>> @@ -44,12 +44,6 @@
>>   #define NB_SEGS(m) ((m)->nb_segs)
>>   #define PORT(m) ((m)->port)
>>
>> -/* Transpose flags. Useful to convert IBV to DPDK flags. */ -#define
>> TRANSPOSE(val, from, to) \
>> -	(((from) >= (to)) ? \
>> -	 (((val) & (from)) / ((from) / (to))) : \
>> -	 (((val) & (from)) * ((to) / (from))))
>> -
>>   /*
>>    * For the case which data is linked with sequence increased index, the
>>    * array table will be more efficiect than hash table once need to serarch
>> --
>> 1.8.3.1
>>