Re: [dpdk-dev] [PATCH] net/mlx5: add ptype table idx for TCP ACK packets

[Yongseok Koh <yskoh@mellanox.com>]

On Thu, Mar 29, 2018 at 07:25:25PM +0800, Bin Huang wrote:
> From: Bin Huang <bin.huang@hxt-semitech.com>
> 
> According to CQE format:
> - l4_hdr_type:
>      0 - None
>      1 - TCP header was present in the packet
>      2 - UDP header was present in the packet
>      3 - TCP header was present in the packet with Empty
>          TCP ACK indication. (TCP packet <ACK> flag is set,
>          and packet carries no data)
>      4 - TCP header was present in the packet with TCP ACK indication.
>          (TCP packet <ACK> flag is set, and packet carries data).
> 
> A packet should be identified as TCP packet if l4_hdr_type is 1, 3 or 4.
> Add corresponding idx of TCP ACK to ptype table.
> 
> previous discussion:
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.mail-archive.com%2Fusers%40dpdk.org%2Fmsg02980.html&data=02%7C01%7Cyskoh%40mellanox.com%7C6e489616778d4440d68708d59567c921%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C1%7C636579195347091211&sdata=%2BPG5X1%2BOR0tnGMw4QKQyz1QrMNqOQ%2BQ5WeJRgjrY7j8%3D&reserved=0
> 
> Signed-off-by: Bin Huang <bin.huang@hxt-semitech.com>
> ---

Thank you for the patch and nice catch!
Only small comments.

>  drivers/net/mlx5/mlx5_rxtx.c | 34 +++++++++++++++++++++++++++++++++-
>  1 file changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/mlx5/mlx5_rxtx.c b/drivers/net/mlx5/mlx5_rxtx.c
> index dc4ead9..e9c5a6f 100644
> --- a/drivers/net/mlx5/mlx5_rxtx.c
> +++ b/drivers/net/mlx5/mlx5_rxtx.c
> @@ -81,11 +81,19 @@
>  		     RTE_PTYPE_L4_FRAG;
>  	(*p)[0x22] = RTE_PTYPE_L2_ETHER | RTE_PTYPE_L3_IPV4_EXT_UNKNOWN |
>  		     RTE_PTYPE_L4_FRAG;
> -	/* TCP */
> +	/* TCP - Support TCP ACK indication */

This comment might not be needed. This can give wrong impression that the
following is to support TCP ACK. Let's leave it as is.

Or, in order to be informative, you can leave separate comments below for each
two. For example,

>  	(*p)[0x05] = RTE_PTYPE_L2_ETHER | RTE_PTYPE_L3_IPV6_EXT_UNKNOWN |
>  		     RTE_PTYPE_L4_TCP;
>  	(*p)[0x06] = RTE_PTYPE_L2_ETHER | RTE_PTYPE_L3_IPV4_EXT_UNKNOWN |
>  		     RTE_PTYPE_L4_TCP;
+	/* TCP ACK indication */
> +	(*p)[0x11] = RTE_PTYPE_L2_ETHER | RTE_PTYPE_L3_IPV6_EXT_UNKNOWN |
> +		     RTE_PTYPE_L4_TCP;
> +	(*p)[0x12] = RTE_PTYPE_L2_ETHER | RTE_PTYPE_L3_IPV4_EXT_UNKNOWN |
> +		     RTE_PTYPE_L4_TCP;
+	/* TCP ACK */
> +	(*p)[0x0d] = RTE_PTYPE_L2_ETHER | RTE_PTYPE_L3_IPV6_EXT_UNKNOWN |
> +		     RTE_PTYPE_L4_TCP;
> +	(*p)[0x0e] = RTE_PTYPE_L2_ETHER | RTE_PTYPE_L3_IPV4_EXT_UNKNOWN |
> +		     RTE_PTYPE_L4_TCP;

Please sort it. Better to move 0x0d and 0x0e earlier.
Can you also repeat it after 0x85 and 0x86?

>  	/* UDP */
>  	(*p)[0x09] = RTE_PTYPE_L2_ETHER | RTE_PTYPE_L3_IPV6_EXT_UNKNOWN |
>  		     RTE_PTYPE_L4_UDP;
> @@ -141,12 +149,36 @@
>  	(*p)[0x46] = RTE_PTYPE_L2_ETHER | RTE_PTYPE_L3_IPV4_EXT_UNKNOWN |
>  		     RTE_PTYPE_INNER_L3_IPV4_EXT_UNKNOWN |
>  		     RTE_PTYPE_INNER_L4_TCP;
> +	(*p)[0x4d] = RTE_PTYPE_L2_ETHER | RTE_PTYPE_L3_IPV4_EXT_UNKNOWN |
> +		     RTE_PTYPE_INNER_L3_IPV6_EXT_UNKNOWN |
> +		     RTE_PTYPE_INNER_L4_TCP;
> +	(*p)[0x4e] = RTE_PTYPE_L2_ETHER | RTE_PTYPE_L3_IPV4_EXT_UNKNOWN |
> +		     RTE_PTYPE_INNER_L3_IPV4_EXT_UNKNOWN |
> +		     RTE_PTYPE_INNER_L4_TCP;
> +	(*p)[0x51] = RTE_PTYPE_L2_ETHER | RTE_PTYPE_L3_IPV4_EXT_UNKNOWN |
> +		     RTE_PTYPE_INNER_L3_IPV6_EXT_UNKNOWN |
> +		     RTE_PTYPE_INNER_L4_TCP;
> +	(*p)[0x52] = RTE_PTYPE_L2_ETHER | RTE_PTYPE_L3_IPV4_EXT_UNKNOWN |
> +		     RTE_PTYPE_INNER_L3_IPV4_EXT_UNKNOWN |
> +		     RTE_PTYPE_INNER_L4_TCP;
>  	(*p)[0xc5] = RTE_PTYPE_L2_ETHER | RTE_PTYPE_L3_IPV6_EXT_UNKNOWN |
>  		     RTE_PTYPE_INNER_L3_IPV6_EXT_UNKNOWN |
>  		     RTE_PTYPE_INNER_L4_TCP;
>  	(*p)[0xc6] = RTE_PTYPE_L2_ETHER | RTE_PTYPE_L3_IPV6_EXT_UNKNOWN |
>  		     RTE_PTYPE_INNER_L3_IPV4_EXT_UNKNOWN |
>  		     RTE_PTYPE_INNER_L4_TCP;
> +	(*p)[0xcd] = RTE_PTYPE_L2_ETHER | RTE_PTYPE_L3_IPV6_EXT_UNKNOWN |
> +		     RTE_PTYPE_INNER_L3_IPV6_EXT_UNKNOWN |
> +		     RTE_PTYPE_INNER_L4_TCP;
> +	(*p)[0xce] = RTE_PTYPE_L2_ETHER | RTE_PTYPE_L3_IPV6_EXT_UNKNOWN |
> +		     RTE_PTYPE_INNER_L3_IPV4_EXT_UNKNOWN |
> +		     RTE_PTYPE_INNER_L4_TCP;
> +	(*p)[0xd1] = RTE_PTYPE_L2_ETHER | RTE_PTYPE_L3_IPV6_EXT_UNKNOWN |
> +		     RTE_PTYPE_INNER_L3_IPV6_EXT_UNKNOWN |
> +		     RTE_PTYPE_INNER_L4_TCP;
> +	(*p)[0xd2] = RTE_PTYPE_L2_ETHER | RTE_PTYPE_L3_IPV6_EXT_UNKNOWN |
> +		     RTE_PTYPE_INNER_L3_IPV4_EXT_UNKNOWN |
> +		     RTE_PTYPE_INNER_L4_TCP;
>  	/* Tunneled - UDP */
>  	(*p)[0x49] = RTE_PTYPE_L2_ETHER | RTE_PTYPE_L3_IPV4_EXT_UNKNOWN |
>  		     RTE_PTYPE_INNER_L3_IPV6_EXT_UNKNOWN |

Thanks,
Yongseok