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

[Bin Huang]

Hi Yongseok,

Thanks for the comments, I have submitted v2 patch following your comments.
https://dpdk.org/dev/patchwork/patch/36763/

On Fri, Mar 30, 2018 at 4:46 AM, Yongseok Koh <yskoh at mellanox.com> wrote:
> On Thu, Mar 29, 2018 at 07:25:25PM +0800, Bin Huang wrote:
>> From: Bin Huang <bin.huang at 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 at 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

Thanks,
Bin