[PWCI] "[v5] acl: support custom memory allocators in rte_acl_build" #510
Conversation
Starting with NVIDIA ConnectX-9, the future devices will support only hardware steering (HWS) flow engine. The software steering options (legacy Verbs and Direct Verbs) have lower performances, and won't be available for new devices. Both flow APIs (sync and async template) will still be supported with the hardware steering flow engine. Fixes: 1b55eeb ("common/mlx5: add ConnectX-9 SuperNIC") Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
The tool ripgrep allows to find files not ending with a line break: rg -Ul '[^\n]\z' The files with a trailing blank lines are shown with this command: rg -Ul '\n\n\z' Files are fixed to end with a single line break. Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
Allow users to provide custom memory allocation callbacks for runtime memory in rte_acl_ctx, via struct rte_acl_mem_cb. Key changes: - Added struct rte_acl_mem_cb with zalloc, free, and udata. - Added rte_acl_set_mem_cb() / rte_acl_get_mem_cb() to set/get callbacks. - Default allocation uses existing rte_zmalloc_socket/rte_free. - Modified ACL code to call callbacks for runtime allocations instead of rte_zmalloc_socket/rte_free directly. v5: - Remove temporary memory allocation callback for build stage. - Introduce new API (rte_acl_set_mem_cb / rte_acl_get_mem_cb) instead of modifying existing rte_acl_config to preserve ABI compatibility. Signed-off-by: YongFeng Wang <mannywang@tencent.com> Signed-off-by: 0-day Robot <robot@bytheb.org>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
WalkthroughVersion bump to 25.11.0 (final release), ACL memory callback infrastructure added for custom allocator support, test additions for callback validation, documentation updates for driver features, and widespread trailing newline formatting corrections across multiple files. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Key areas requiring attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
doc/guides/nics/mlx5.rst (1)
1337-1365: Clarify device support scope for Hardware Steering.Line 1343-1344 states HWS is "the only mode supported on device ConnectX-9 and later," but the requirements table (lines 1352-1358) shows HWS minimum hardware is ConnectX-6 Dx. This appears inconsistent—line 1343-1344 suggests exclusive support on ConnectX-9+, while the requirements table and earlier context indicate HWS is supported on older devices.
Consider revising line 1343-1344 to clarify the distinction: HWS may be the preferred or only stable mode on ConnectX-9+, but it is supported (with limitations) on earlier hardware like ConnectX-6 Dx.
lib/acl/rte_acl.c (1)
575-600: Document callback lifecycle constraints.The implementation is correct. Consider adding a note in the header documentation that
rte_acl_set_mem_cbshould be called beforerte_acl_build, or afterrte_acl_reset, to avoid mismatched allocator/deallocator pairs for existing runtime structures.app/test/test_acl.c (1)
1732-1749: Alignment parameter is ignored.The
alignparameter is not used, which could cause issues if the ACL code requires specific memory alignment. Sincerunning_bufis allocated withRTE_CACHE_LINE_SIZEalignment, it works for this test, but the comment documents this limitation.Consider adding an alignment check or at least documenting the assumption:
static void *running_alloc(void *udata, char *name, size_t size, size_t align, int32_t socket_id) { - (void)align; (void)name; (void)socket_id; + /* running_buf is allocated with RTE_CACHE_LINE_SIZE alignment, + * which should satisfy typical ACL alignment requirements */ + (void)align; if (size > ACL_RUNNING_BUF_SIZE) return NULL;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (17)
doc/guides/contributing/img/abi_stability_policy.svgis excluded by!**/*.svgdoc/guides/contributing/img/what_is_an_abi.svgis excluded by!**/*.svgdoc/guides/howto/img/flow_bifurcation_overview.svgis excluded by!**/*.svgdoc/guides/howto/img/virtio_user_for_container_networking.svgis excluded by!**/*.svgdoc/guides/prog_guide/img/bond-mode-0.svgis excluded by!**/*.svgdoc/guides/prog_guide/img/bond-mode-1.svgis excluded by!**/*.svgdoc/guides/prog_guide/img/bond-mode-2.svgis excluded by!**/*.svgdoc/guides/prog_guide/img/bond-mode-3.svgis excluded by!**/*.svgdoc/guides/prog_guide/img/bond-mode-4.svgis excluded by!**/*.svgdoc/guides/prog_guide/img/bond-mode-5.svgis excluded by!**/*.svgdoc/guides/prog_guide/img/lcore_var_mem_layout.svgis excluded by!**/*.svgdoc/guides/prog_guide/img/pdcp_functional_overview.svgis excluded by!**/*.svgdoc/guides/prog_guide/img/predictable_snat_1.svgis excluded by!**/*.svgdoc/guides/prog_guide/img/predictable_snat_2.svgis excluded by!**/*.svgdoc/guides/prog_guide/img/rss_queue_assign.svgis excluded by!**/*.svgdoc/guides/prog_guide/img/static_array_mem_layout.svgis excluded by!**/*.svgdoc/guides/sample_app_ug/img/overlay_networking.svgis excluded by!**/*.svg
📒 Files selected for processing (55)
VERSION(1 hunks)app/test-bbdev/test_vectors/bbdev_null.data(1 hunks)app/test-bbdev/test_vectors/turbo_dec_c1_k6144_r0_e10376_crc24b_sbd_negllr_high_snr.data(1 hunks)app/test-bbdev/test_vectors/turbo_dec_c1_k6144_r0_e10376_crc24b_sbd_negllr_low_snr.data(1 hunks)app/test-bbdev/test_vectors/turbo_dec_c1_k6144_r0_e34560_sbd_posllr.data(1 hunks)app/test-bbdev/test_vectors/turbo_dec_c2_k3136_r0_e4920_sbd_negllr_crc24b.data(1 hunks)app/test-bbdev/test_vectors/turbo_enc_c1_k6144_r0_e18448_crc24a.data(1 hunks)app/test-bbdev/test_vectors/turbo_enc_c3_k4800_r2_e14412_crc24b.data(1 hunks)app/test/test_acl.c(2 hunks)doc/guides/nics/cxgbe.rst(0 hunks)doc/guides/nics/ipn3ke.rst(0 hunks)doc/guides/nics/mlx5.rst(2 hunks)drivers/event/dlb2/dlb2_iface.c(0 hunks)drivers/net/bnxt/tf_core/tf_em_common.c(0 hunks)drivers/net/bnxt/tf_ulp/generic_templates/ulp_template_db_act.c(0 hunks)drivers/net/bnxt/tf_ulp/generic_templates/ulp_template_db_class.c(0 hunks)drivers/net/bnxt/tf_ulp/generic_templates/ulp_template_db_defs.h(0 hunks)drivers/net/bnxt/tf_ulp/generic_templates/ulp_template_db_tbl.c(0 hunks)drivers/net/bnxt/tf_ulp/generic_templates/ulp_template_db_thor2_class.c(0 hunks)drivers/net/bnxt/tf_ulp/generic_templates/ulp_template_db_thor_class.c(0 hunks)drivers/net/bnxt/tf_ulp/generic_templates/ulp_template_db_wh_plus_class.c(0 hunks)drivers/net/gve/base/gve_adminq.c(0 hunks)drivers/net/gve/gve_rss.c(0 hunks)drivers/net/gve/gve_rss.h(0 hunks)drivers/net/hinic/base/hinic_pmd_mbox.c(0 hunks)drivers/net/hinic/base/hinic_pmd_niccfg.c(0 hunks)drivers/net/hinic/base/meson.build(0 hunks)drivers/net/intel/e1000/base/e1000_82575.c(0 hunks)drivers/net/intel/e1000/base/e1000_mbx.c(0 hunks)drivers/net/intel/e1000/base/e1000_vf.c(0 hunks)drivers/net/intel/i40e/base/i40e_adminq.c(0 hunks)drivers/net/intel/ice/base/ice_flex_pipe.c(0 hunks)drivers/net/intel/ice/base/ice_hw_autogen.h(0 hunks)drivers/net/intel/ixgbe/base/ixgbe_dcb_82599.c(0 hunks)drivers/net/intel/ixgbe/base/ixgbe_x540.h(0 hunks)drivers/net/mlx5/hws/mlx5dr_buddy.c(0 hunks)drivers/net/nfp/nfp_ipsec.c(0 hunks)drivers/net/nfp/nfpcore/nfp_elf.c(0 hunks)drivers/net/ngbe/base/ngbe_dummy.h(0 hunks)drivers/net/ngbe/base/ngbe_phy.c(0 hunks)drivers/net/octeontx/base/octeontx_bgx.h(0 hunks)drivers/net/txgbe/base/txgbe_dcb.c(0 hunks)drivers/net/txgbe/base/txgbe_dcb_hw.c(0 hunks)drivers/net/txgbe/base/txgbe_dummy.h(0 hunks)drivers/net/txgbe/base/txgbe_eeprom.c(0 hunks)drivers/net/virtio/virtio_cvq.c(0 hunks)drivers/raw/cnxk_rvu_lf/cnxk_rvu_lf_selftest.c(0 hunks)drivers/vdpa/mlx5/mlx5_vdpa_event.c(0 hunks)dts/configurations/tests_config.example.yaml(1 hunks)lib/acl/acl.h(1 hunks)lib/acl/acl_bld.c(1 hunks)lib/acl/acl_gen.c(1 hunks)lib/acl/rte_acl.c(4 hunks)lib/acl/rte_acl.h(1 hunks)license/Linux-syscall-note(0 hunks)
💤 Files with no reviewable changes (39)
- drivers/net/intel/e1000/base/e1000_vf.c
- drivers/net/hinic/base/hinic_pmd_mbox.c
- doc/guides/nics/cxgbe.rst
- drivers/net/txgbe/base/txgbe_dummy.h
- drivers/net/virtio/virtio_cvq.c
- drivers/net/gve/base/gve_adminq.c
- drivers/net/hinic/base/hinic_pmd_niccfg.c
- drivers/net/txgbe/base/txgbe_eeprom.c
- drivers/net/bnxt/tf_ulp/generic_templates/ulp_template_db_tbl.c
- drivers/net/intel/ice/base/ice_flex_pipe.c
- drivers/raw/cnxk_rvu_lf/cnxk_rvu_lf_selftest.c
- drivers/net/intel/i40e/base/i40e_adminq.c
- drivers/net/ngbe/base/ngbe_dummy.h
- drivers/net/gve/gve_rss.c
- doc/guides/nics/ipn3ke.rst
- drivers/net/txgbe/base/txgbe_dcb_hw.c
- drivers/net/txgbe/base/txgbe_dcb.c
- drivers/net/ngbe/base/ngbe_phy.c
- drivers/net/gve/gve_rss.h
- drivers/net/intel/ixgbe/base/ixgbe_dcb_82599.c
- drivers/net/nfp/nfpcore/nfp_elf.c
- drivers/net/octeontx/base/octeontx_bgx.h
- drivers/net/nfp/nfp_ipsec.c
- drivers/net/bnxt/tf_ulp/generic_templates/ulp_template_db_class.c
- drivers/net/bnxt/tf_ulp/generic_templates/ulp_template_db_defs.h
- drivers/net/bnxt/tf_ulp/generic_templates/ulp_template_db_act.c
- drivers/net/hinic/base/meson.build
- drivers/net/intel/e1000/base/e1000_82575.c
- license/Linux-syscall-note
- drivers/net/mlx5/hws/mlx5dr_buddy.c
- drivers/net/bnxt/tf_ulp/generic_templates/ulp_template_db_thor2_class.c
- drivers/net/bnxt/tf_core/tf_em_common.c
- drivers/net/bnxt/tf_ulp/generic_templates/ulp_template_db_thor_class.c
- drivers/event/dlb2/dlb2_iface.c
- drivers/net/intel/ixgbe/base/ixgbe_x540.h
- drivers/net/intel/e1000/base/e1000_mbx.c
- drivers/net/bnxt/tf_ulp/generic_templates/ulp_template_db_wh_plus_class.c
- drivers/vdpa/mlx5/mlx5_vdpa_event.c
- drivers/net/intel/ice/base/ice_hw_autogen.h
🧰 Additional context used
🧬 Code graph analysis (1)
app/test/test_acl.c (1)
lib/acl/rte_acl.c (1)
rte_acl_create(385-455)
🔇 Additional comments (20)
dts/configurations/tests_config.example.yaml (1)
17-17: No concerns.This is a non-functional trailing whitespace/newline adjustment on a commented configuration line.
app/test-bbdev/test_vectors/turbo_dec_c2_k3136_r0_e4920_sbd_negllr_crc24b.data (1)
676-677: Good: Trailing newline added for POSIX compliance.The addition of a trailing newline after the
expected_statusvalue follows POSIX conventions and is consistent with the PR's broader formatting standardization across test vector files. This change has no functional impact on test execution.app/test-bbdev/test_vectors/turbo_dec_c1_k6144_r0_e10376_crc24b_sbd_negllr_low_snr.data (1)
640-640: Trailing newline addition aligns with best practices.The file now ends with a trailing newline, which is consistent with POSIX text file conventions and improves formatting consistency across the test vector files. No functional changes to test data or configuration parameters.
app/test-bbdev/test_vectors/turbo_dec_c1_k6144_r0_e34560_sbd_posllr.data (1)
1221-1222: Formatting-only change; no functional impact.The modification adds a trailing newline after the
expected_statusvalue. This is a consistent formatting improvement across the test vector files with no effect on test execution or data integrity.app/test-bbdev/test_vectors/turbo_enc_c3_k4800_r2_e14412_crc24b.data (1)
152-153: LGTM!Adding a trailing newline at EOF is a standard POSIX-compliant formatting improvement. No functional changes to the test vector data.
app/test-bbdev/test_vectors/bbdev_null.data (1)
4-5: LGTM!Trailing newline addition for POSIX compliance. Consistent with the broader formatting cleanup across test vector files in this PR.
doc/guides/nics/mlx5.rst (1)
692-703: Documentation improvements are clear and well-organized.The restructuring of the
dv_flow_enparameter documentation (lines 692-697) improves readability by:
- Explicitly stating device availability constraints ("devices older than ConnectX-9")
- Adding rdma-core requirement note for Value 1
- Separating values into distinct paragraphs for clarity
The changes maintain consistency with the device support table and enhance user guidance on mode selection.
app/test-bbdev/test_vectors/turbo_dec_c1_k6144_r0_e10376_crc24b_sbd_negllr_high_snr.data (1)
639-640: LGTM!Adding a trailing newline to the file is a good housekeeping practice for POSIX compliance and cleaner git diffs. No test data or semantics are affected.
VERSION (1)
1-1: LGTM!Version bump to final release 25.11.0 is appropriate.
lib/acl/rte_acl.h (2)
139-151: Well-designed callback structure.The
rte_acl_mem_cbstructure follows DPDK conventions with proper parameters for socket-aware allocation. Theudatafield enables stateful allocators.
153-179: Clean API design for memory callback accessors.The set/get functions are well-documented with proper const correctness. Using
const struct rte_acl_mem_cb *for input inrte_acl_set_mem_cbis appropriate.lib/acl/acl_gen.c (1)
481-488: LGTM!The allocation correctly uses the per-context memory callback with appropriate parameters. Error handling for allocation failure is preserved.
lib/acl/acl.h (1)
177-177: LGTM!The
mem_cbfield is appropriately added to the internal context structure, enabling per-context memory callback management.lib/acl/acl_bld.c (1)
782-782: LGTM!The deallocation correctly uses the per-context memory callback. The free callback is expected to handle NULL pointers gracefully (as demonstrated by both the default
free_dftand the test'srunning_free).lib/acl/rte_acl.c (3)
267-279: LGTM!Default allocator functions properly wrap the standard DPDK memory functions and correctly suppress unused parameter warnings.
379-379: LGTM!Using the per-context memory callback for freeing runtime memory before freeing the context itself is the correct ordering.
442-444: LGTM!Context initialization with default memory callbacks ensures backward compatibility - existing code will work without modification.
app/test/test_acl.c (3)
1724-1728: LGTM!The wrapper structure cleanly encapsulates the ACL context with a pre-allocated buffer and usage flag for tracking.
1803-1841: Good test coverage for memory callback functionality.The test properly exercises:
- Setting and retrieving memory callbacks
- ACL reset/build/classify with custom allocator
- Multiple iterations to verify callback reuse
The cleanup at lines 1838-1839 is correct.
1864-1865: LGTM!The new test is properly integrated into the test suite.
| if (!g_acl_ctx_wrapper.running_buf) { | ||
| printf("Line %i: Error allocing running buf for acl context!\n", __LINE__); | ||
| return 1; | ||
| } |
There was a problem hiding this comment.
Inconsistent error return value.
Returns 1 on allocation failure, but the test framework and other tests in this file return -1 for errors.
if (!g_acl_ctx_wrapper.running_buf) {
printf("Line %i: Error allocing running buf for acl context!\n", __LINE__);
- return 1;
+ rte_acl_free(g_acl_ctx_wrapper.ctx);
+ return -1;
}Note: Also added missing cleanup of the ACL context before returning.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!g_acl_ctx_wrapper.running_buf) { | |
| printf("Line %i: Error allocing running buf for acl context!\n", __LINE__); | |
| return 1; | |
| } | |
| if (!g_acl_ctx_wrapper.running_buf) { | |
| printf("Line %i: Error allocing running buf for acl context!\n", __LINE__); | |
| rte_acl_free(g_acl_ctx_wrapper.ctx); | |
| return -1; | |
| } |
🤖 Prompt for AI Agents
In app/test/test_acl.c around lines 1775-1778, the allocation-failure branch
prints an error and returns 1 which is inconsistent with the test framework
convention of returning -1; update the return value to -1 and ensure the ACL
context is cleaned up before returning (free or teardown whatever
g_acl_ctx_wrapper holds and set pointers to NULL as done elsewhere in this file)
so the function exits with consistent error code and no resource leak.
| ret = rte_acl_set_mem_cb(g_acl_ctx_wrapper.ctx, &mcb); | ||
| if (ret) { | ||
| printf("Line %i: Error set mem cb for acl context!\n", __LINE__); | ||
| return 1; | ||
| } | ||
| struct rte_acl_mem_cb new_mcb; | ||
| memset(&new_mcb, 0, sizeof(struct rte_acl_mem_cb)); | ||
| ret = rte_acl_get_mem_cb(g_acl_ctx_wrapper.ctx, &new_mcb); | ||
| if (ret) { | ||
| printf("Line %i: Error get mem cb for acl context!\n", __LINE__); | ||
| return 1; | ||
| } | ||
| if (memcmp(&mcb, &new_mcb, sizeof(struct rte_acl_mem_cb)) != 0) { | ||
| printf("Line %i: Error get mem cb for acl context!\n", __LINE__); | ||
| return 1; |
There was a problem hiding this comment.
Inconsistent error return values and missing cleanup.
Multiple return paths use 1 instead of -1 and don't clean up allocated resources.
ret = rte_acl_set_mem_cb(g_acl_ctx_wrapper.ctx, &mcb);
if (ret) {
printf("Line %i: Error set mem cb for acl context!\n", __LINE__);
- return 1;
+ goto err;
}
struct rte_acl_mem_cb new_mcb;
memset(&new_mcb, 0, sizeof(struct rte_acl_mem_cb));
ret = rte_acl_get_mem_cb(g_acl_ctx_wrapper.ctx, &new_mcb);
if (ret) {
printf("Line %i: Error get mem cb for acl context!\n", __LINE__);
- return 1;
+ goto err;
}
if (memcmp(&mcb, &new_mcb, sizeof(struct rte_acl_mem_cb)) != 0) {
printf("Line %i: Error get mem cb for acl context!\n", __LINE__);
- return 1;
+ goto err;
}Add an error label at the end:
err:
rte_acl_free(g_acl_ctx_wrapper.ctx);
rte_free(g_acl_ctx_wrapper.running_buf);
return -1;🤖 Prompt for AI Agents
In app/test/test_acl.c around lines 1786 to 1800 the error paths return 1 and
skip cleanup; update those error returns to jump to a new err label (e.g., "goto
err;") so allocated resources are freed; add an err label at the end that calls
rte_acl_free(g_acl_ctx_wrapper.ctx) and rte_free(g_acl_ctx_wrapper.running_buf)
and then returns -1; ensure every early failure in this block uses the goto err
path instead of returning 1 so cleanup always runs and the function returns -1
on error.
NOTE: This is an auto submission for "[v5] acl: support custom memory allocators in rte_acl_build".
See "http://patchwork.dpdk.org/project/dpdk/list/?series=36819" for details.
Summary by CodeRabbit
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.