On Sat, 27 Dec 2025 23:12:21 +0530 Kumara Parameshwaran <[email protected]> wrote:
> Use cuckoo hash library in GRO for flow flookup > > Signed-off-by: Kumara Parameshwaran <[email protected]> > --- 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.
