On Mon, 3 Nov 2025 14:24:33 +0100 Alex Kiselev <[email protected]> wrote:
> lpm6c is a new implementation of the IPv6 LPM algorithm, based on > the existing lpm module. It utilizes a trie path compression technique to > optimize lookup operations. > > The implementation offers a performance improvement over the lpm > module starting from /64 and longer prefixes. This makes it beneficial > for use cases like BNG routers, whose FIB tables contain numerous /64 > prefixes and primarily handle traffic targeting them. > > Performance comparison results against the original LPM6 for lookups > at AMD Ryzen 9 5900XT CPU: > > prefix /48 tests: > Speedup vs LPM6: LPM6c 0.991x, LPM6c VEC 0.921x, FIB 1.000x, FIB VEC 1.048x > > prefix /64 tests: > Speedup vs LPM6: LPM6c 1.399x, LPM6c VEC 1.302x, FIB 1.145x, FIB VEC 1.292x > > prefix /80 tests: > Speedup vs LPM6: LPM6c 1.620x, LPM6c VEC 1.558x, FIB 1.096x, FIB VEC 1.226x > > prefix /128 tests: > Speedup vs LPM6: LPM6c 2.497x, LPM6c VEC 2.369x, FIB 1.255x, FIB VEC 1.349x > > Signed-off-by: Alex Kiselev <[email protected]> > --- AI code review against AGENTS.md ## DPDK Patch Review: lpm: add new LPM v6 implementation (lpm6c) ### Summary This patch adds a new path-compressed IPv6 LPM implementation with compelling performance improvements for /64+ prefixes. However, there are several compliance issues that need to be addressed before this can be merged. --- ### Errors (Must Fix) **1. SPDX License Format Issues** Multiple files have the SPDX identifier on line 2 instead of line 1: - `lib/lpm/rte_lpm6c.c` (lines 2498-2501): ```c /* * SPDX-License-Identifier: BSD-3-Clause ← Wrong: SPDX on line 2 ``` - `lib/lpm/rte_lpm6c.h` (lines 3824-3827): Same issue - `app/test/test_lpm6c_perf.c` (lines 1901-1904): Same issue - `app/test/test_lpm6c_perf_cmp.c` (lines 2068-2071): Same issue Should be: ```c /* SPDX-License-Identifier: BSD-3-Clause * Copyright(c) 2016-2025 Alex Kiselev, alex at BisonRouter.com */ ``` **2. Missing Blank Line After Copyright** All new files are missing the required blank line between the copyright block and the first `#include`: Example from `rte_lpm6c.c`: ```c */ #include <string.h> ← Should have blank line before this ``` **3. Missing `__rte_experimental` Markers** New APIs must be marked as `__rte_experimental`. None of the public functions in `rte_lpm6c.h` have this marker: ```c /* Should be: */ __rte_experimental struct rte_lpm6c * rte_lpm6c_create(const char *name, int socket_id, const struct rte_lpm6c_config *config); ``` This applies to all public functions: `rte_lpm6c_create`, `rte_lpm6c_find_existing`, `rte_lpm6c_free`, `rte_lpm6c_add`, `rte_lpm6c_delete`, `rte_lpm6c_delete_all`, `rte_lpm6c_lookup`, `rte_lpm6c_lookup_bulk`, etc. **4. Include Order Violation** In `rte_lpm6c.c`, system header `<assert.h>` appears after DPDK headers: ```c #include <rte_rwlock.h> #include <rte_spinlock.h> #include <rte_hash.h> #include <assert.h> ← System header after DPDK headers #include <rte_jhash.h> ``` System headers must come before DPDK headers. --- ### Warnings (Should Fix) **1. Missing Release Notes** This is a significant new feature (new LPM6 algorithm) and requires an entry in `doc/guides/rel_notes/release_XX_XX.rst`. Per guidelines: "New drivers or subsystems must have release notes." **2. Missing MAINTAINERS Update** No update to the `MAINTAINERS` file to add ownership of the new `lpm6c` component. **3. Missing Doxygen Documentation** Several public functions lack Doxygen comments: - `rte_lpm6c_rules_iterate()` (line 4124) - `rte_lpm6c_rules_iterate_cb()` (line 4129) - `rte_lpm6c_pool_pos()` (line 4376) - `rte_lpm6c_used_rules()` (line 4379) **4. Inconsistent Copyright Format** `test_lpm6c_perf_cmp.c` has inconsistent copyright: ```c Copyright(c) 2016-2025 Alex Kiselev BisonRouter.com ``` vs other files: ```c Copyright(c) 2016-2025 Alex Kiselev, alex at BisonRouter.com ``` **5. Use of `assert()` Instead of RTE Equivalents** Multiple uses of standard `assert()` in `rte_lpm6c.c` (lines 3564, 3569, 3601, 3609, 3680, 3694-3698). Consider using `RTE_ASSERT()` or proper error handling for production code. **6. Test Registration Style** The patch uses `REGISTER_FAST_TEST` and `REGISTER_PERF_TEST`. Verify these are the current preferred macros vs. the older `REGISTER_TEST_COMMAND`. --- ### Info (Consider) **1. Struct Alignment Syntax** The pattern used: ```c struct rte_lpm6c_tbl8 { ... } __rte_cache_aligned; ``` While this compiles, the more common DPDK style is: ```c struct __rte_cache_aligned rte_lpm6c_tbl8 { ... }; ``` **2. Public Structure Exposure** The full `struct rte_lpm6c` is exposed in the public header (necessary for inline lookup functions). This is a design tradeoff—consider documenting that the struct layout is not ABI-stable while experimental. **3. Performance Benchmarks** The commit message includes good performance data. Consider adding this to documentation as well. --- ### Checklist Summary | Category | Status | |----------|--------| | Subject line ≤60 chars | ✓ | | Correct component prefix (`lpm:`) | ✓ | | Signed-off-by present | ✓ | | SPDX on first line | ✗ | | Blank line after copyright | ✗ | | `__rte_experimental` on new APIs | ✗ | | Include order correct | ✗ | | Release notes updated | ✗ | | MAINTAINERS updated | ✗ | | Tests provided | ✓ | | Doxygen documentation | Partial | --- ### Verdict **Needs revision.** The algorithm and implementation look solid with good test coverage and compelling performance numbers. However, there are mandatory compliance issues (SPDX format, missing experimental markers, include order) that must be fixed before this can be accepted. A v2 addressing the errors above would be welcome.
