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 <ys...@mellanox.com> wrote: > On Thu, Mar 29, 2018 at 07:25:25PM +0800, Bin Huang wrote: >> From: Bin Huang <bin.hu...@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.hu...@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
