Re: [PATCH v3] gro : improve GRO performance based on hash table

[Stephen Hemminger <stephen@networkplumber.org>]

On Sat, 27 Dec 2025 23:12:21 +0530
Kumara Parameshwaran <kumaraparamesh92@gmail.com> wrote:

> Use cuckoo hash library in GRO for flow flookup
> 
> Signed-off-by: Kumara Parameshwaran <kumaraparamesh92@gmail.com>
> ---

Since this did not get a through review, used AI review to find:

Review: [PATCH v3] gro : improve GRO performance based on hash table
(test file portion only -- app/test/test_gro.c)

The test rewrite is a significant improvement over the old hardcoded
byte arrays. Constructing packets programmatically and validating
headers and payloads makes the tests more readable and maintainable.
Several issues below.

Error 1: SYN packet in test_tcp4_mixed_flags has wrong IP total_length

build_ipv4_hdr is called with payload_len=1400 for every segment
including the SYN packet (seg 0), but the SYN packet has no payload
appended (goto skip_payload). The IP total_length field will claim
sizeof(rte_ipv4_hdr) + sizeof(rte_tcp_hdr) + 1400 = 1454 bytes,
but the actual mbuf only contains the headers (54 bytes after
Ethernet). This creates a malformed packet whose IP total_length
disagrees with the mbuf data_len/pkt_len. Should pass 0 as
payload_len for the SYN segment:

  uint16_t plen = (flag_options[seg] & RTE_TCP_SYN_FLAG) ? 0 : 1400;
  build_ipv4_hdr(ip, flows[flow].src_ip, flows[flow].dst_ip,
      plen, pkt_idx);

Error 2: test_tcp4_max_flows validates gro_pkts[flow] assuming
same ordering as flows[] array

The validation loop at the end of test_tcp4_max_flows iterates
flow = 0..31 and validates gro_pkts[flow] against flows[flow].
Unlike test_tcp4_multiple_flows which correctly searches for the
matching flow by IP address, this test assumes timeout_flush returns
packets in the same order they were inserted. GRO uses a hash table
internally and does not guarantee output ordering. The test should
search for each flow's packet as the other tests do.

Error 3: test_tcp4_max_flows error message prints wrong expected count

The assertion message says "should return %d packets" and passes
NUM_FLOWS_MAX (33) as the format argument, but the actual expected
value is NUM_FLOWS_MAX-1 (32):

  TEST_ASSERT(nb_gro_pkts == NUM_FLOWS_MAX-1,
      "GRO timeout flush should return %d packets returned %d",
      NUM_FLOWS_MAX, nb_gro_pkts);

Should be NUM_FLOWS_MAX-1 in the format argument to match the
assertion condition.

Warning 1: #define NUM_FLOWS, SEGS_PER_FLOW, TOTAL_PKTS redefined

These macros are defined identically inside both
test_tcp4_multiple_flows and test_tcp4_mixed_flags. Preprocessor
macros defined inside function bodies have file scope and persist
beyond the function. The second set of #defines will produce
compiler warnings about macro redefinition. Either define them
once at file scope, or use enum constants or local variables.

Warning 2: use of bare "uint" type

validate_merged_payload declares num_segments as "uint" which is
not a C standard type (it is a POSIX typedef). Use "unsigned int"
or "uint32_t" for portability.

Warning 3: resource leaks on early return in test functions

Every test function allocates mbufs in a loop and returns
TEST_FAILED immediately if any allocation or append fails, without
freeing previously allocated mbufs. While the teardown function
destroys the mempool, the GRO context may still hold references
to mbufs submitted before the failure. Consider using a goto
cleanup pattern.

Warning 4: unnecessary void * casts

Throughout the test, return values from rte_pktmbuf_append are
cast to struct pointers:

  eth = (struct rte_ether_hdr *)rte_pktmbuf_append(...)

rte_pktmbuf_append returns char * which in C converts implicitly
to any pointer type. The casts are unnecessary.

Warning 5: implicit pointer comparisons

Multiple instances of:

  if (!pkts_mb[pkt_idx])
  if (!eth)

Should use explicit NULL comparison per DPDK coding style:

  if (pkts_mb[pkt_idx] == NULL)

Info 1: srand(time(NULL)) is called in each test function
separately. Since the tests run sequentially and quickly, the
seed may be identical across tests, producing the same "random"
payloads. Consider seeding once in setup, or using a fixed seed
for reproducibility in regression testing.

Info 2: The comment block for max_flow_num/max_item_per_flow has
a blank "*" line at the start that looks like a formatting artifact.