On Wed, 4 Dec 2024 18:05:51 +0800 Caiqiang Liao <[email protected]> wrote:
> The rte_gso_segment function increases the processing of IPV6 tcp packets > > Signed-off-by: Caiqiang Liao <[email protected]> This patch never got any review. Turned to AI review and it found a lot to fix. Summary of Findings The most significant issue is a logic error in rte_gso_segment that prevents plain IPv6/TCP packets from being segmented and incorrectly handles tunnel flags. There are also widespread indentation issues and inconsistent use of DPDK-preferred endianness functions. --- [ERROR] Correctness Bugs 1. Logic Error in rte_gso_segment Conditionals In lib/gso/rte_gso.c, the new block for IPv6 TCP segmentation has incorrect flag checks and redundant/wrong protocol handling: 1 + } else if ((IS_IPV6_TCP(pkt->ol_flags) && 2 + (gso_ctx->gso_types & RTE_ETH_TX_OFFLOAD_VXLAN_TNL_TSO)) || 3 + ((IS_IPV4_GRE_TCP4(pkt->ol_flags) && 4 + (gso_ctx->gso_types & RTE_ETH_TX_OFFLOAD_GRE_TNL_TSO)))) { 5 + pkt->ol_flags &= (~RTE_MBUF_F_TX_TCP_SEG); 6 + ret = gso_tcp6_segment(pkt, gso_size, 7 + direct_pool, indirect_pool, 8 + pkts_out, nb_pkts_out); - Plain IPv6/TCP packets will not be segmented because the code checks for RTE_ETH_TX_OFFLOAD_VXLAN_TNL_TSO instead of RTE_ETH_TX_OFFLOAD_TCP_TSO. - IS_IPV4_GRE_TCP4 is an IPv4 protocol but is incorrectly grouped here to be processed by gso_tcp6_segment. Additionally, this protocol is already handled earlier in the function by gso_tunnel_tcp4_segment. 2. Inconsistent Endianness Conversion The patch uses standard libc functions (htons, ntohs, ntohl) instead of the DPDK-preferred rte_cpu_to_be_X and rte_be_to_cpu_X macros. This is found in: - lib/gso/gso_common.h: update_ipv6_header uses htons. - lib/gso/gso_tcp6.c: update_ipv6_tcp_headers uses ntohl. - lib/gso/gso_tunnel_tcp6.c: update_tunnel_ipv6_tcp_headers uses ntohs and ntohl. --- [WARNING] Style and Process Issues 1. Indentation The patch consistently uses 4-space indentation in the new files (gso_tcp6.c, gso_tcp6.h, gso_tunnel_tcp6.c, etc.) and some modified blocks. DPDK coding style requires the use of tabs for indentation. --- [ADVISORY] Improvements 1. Use of rte_pktmbuf_mtod_offset The patch frequently uses rte_pktmbuf_mtod(pkt, char *) + offset. DPDK style prefers the more concise and safer rte_pktmbuf_mtod_offset(pkt, type, offset) macro, which is already used elsewhere in the GSO library.
