Re: [PATCH] lib/gso: gso Adds the processing of IPV6 tcp packets

[Stephen Hemminger <stephen@networkplumber.org>]

On Wed,  4 Dec 2024 18:05:51 +0800
Caiqiang Liao <18859237562@163.com> wrote:

> The rte_gso_segment function increases the processing of IPV6 tcp packets
> 
> Signed-off-by: Caiqiang Liao <18859237562@163.com>

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.