Skip to content

Conversation

@ovsrobot
Copy link
Owner

@ovsrobot ovsrobot commented Oct 31, 2025

NOTE: This is an auto submission for "[1/1] doc: fix note in FreeBSD guide".

See "http://patchwork.dpdk.org/project/dpdk/list/?series=36533" for details.

Summary by CodeRabbit

  • Tests

    • Updated hash table tests with performance-aware scaling configuration
    • Implemented dynamic test parameter sizing based on performance mode
    • Added allocation failure handling with error reporting
  • Documentation

    • Fixed documentation directive syntax for proper rendering

When reserving a specific memory amount, it was possible to pass
the first allocations and fail on a later allocation
where there was no check, resulting in a crash.
It is fixed by stopping the test if allocation failed.

Fixes: fd368e1 ("test/hash: test more corner cases")
Fixes: 9c7d8ee ("test/hash: add RCU tests")
Cc: stable@dpdk.org

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
Reviewed-by: David Marchand <david.marchand@redhat.com>
When running on limited platforms like GitHub Actions,
the functional unit test "hash_readwrite_func_autotest"
will hit a timeout, especially when running with UBSan:

46/102 DPDK:fast-tests / hash_readwrite_func_autotest  TIMEOUT  30.01s
killed by signal 15 SIGTERM

Similarly to what was done in the
commit fd368e1 ("test/hash: test more corner cases"),
some constants are decreased.
In order to keep the performance test as it was,
a multiplier is kept for performance test case only.

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
Acked-by: David Marchand <david.marchand@redhat.com>
The note about the prefix of the package pyelftools was not showed
in the documentation output because the syntax was missing a colon,
so it was considered as a simple comment in the source file.

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
Signed-off-by: 0-day Robot <robot@bytheb.org>
Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @ovsrobot, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

@coderabbitai
Copy link

coderabbitai bot commented Oct 31, 2025

Walkthrough

This pull request introduces performance-aware scaling to hash table tests. Test constants are refactored with a perf multiplier, the hash table initialization function gains a perf flag parameter for dynamic sizing, and null-check validation is added for memory allocations to prevent uninitialized pointer use.

Changes

Cohort / File(s) Change Summary
Performance-aware test scaling
app/test/test_hash_readwrite.c
Reduced TOTAL_ENTRY from 5M to 5K; introduced TOTAL_INSERT_FUNC (4.5K), TOTAL_INSERT_FUNC_EXT (5K), and PERF_MULTIPLIER (1024). Redefined TOTAL_INSERT as product of TOTAL_INSERT_FUNC and PERF_MULTIPLIER. Extended init_params() signature with bool perf parameter; hash_params.entries now scales dynamically based on perf flag. Updated memory allocations and loops to use hash_params.entries; added perf flag propagation in test_hash_readwrite_functional (false) and test_hash_readwrite_perf (true).
Allocation validation
app/test/test_hash_readwrite_lf_perf.c
Added null-check after RCU QSBR writer allocation with error handling and early return on allocation failure.
Documentation fix
doc/guides/freebsd_gsg/build_dpdk.rst
Corrected reStructuredText note directive syntax from .. note: to .. note::.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • test_hash_readwrite.c requires careful verification that the perf multiplier logic is consistently applied across all sizing, allocation, and iteration paths; confirm functional tests remain unaffected (perf=false) while perf tests correctly scale (perf=true)
  • test_hash_readwrite_lf_perf.c straightforward defensive check; briefly verify error path is appropriate
  • build_dpdk.rst cosmetic RST syntax correction; no logic impact

Poem

🐰 The tests now scale with grace and care,
With perf flags floating through the air,
Constants multiplied, allocations dance,
While null-checks catch each missed chance,
Small tweaks that make the whole thing right! ✨

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Title Check ⚠️ Warning The pull request title "[PWCI] "[1/1] doc: fix note in FreeBSD guide" describes only one aspect of the changeset: a minor ReStructuredText formatting fix in the FreeBSD build guide documentation. However, the raw summary reveals that the changeset includes substantial functional changes to test files, specifically significant modifications to app/test/test_hash_readwrite.c (rated High review effort) and app/test/test_hash_readwrite_lf_perf.c (rated Medium review effort). These test file changes involve perf-aware scaling, dynamic hash table sizing, memory allocation adjustments, and function signature changes. The title only mentions the documentation fix and fails to convey that the primary changes involve test code modifications. A teammate scanning the pull request history based on this title would have a misleading impression that this is solely a minor documentation fix. The title should be revised to reflect the primary changes in the pull request. Consider changing it to something like "test: add perf-aware scaling and null-check handling for hash table tests" or similar, which accurately captures the main functional changes across the test files. If the documentation fix is secondary, it can be mentioned in the pull request description or body rather than in the primary title. The title should be specific enough that a teammate reviewing the PR history understands the substantial nature of the changes being introduced.
Docstring Coverage ⚠️ Warning Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch series_36533

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 39b54f2 and 57f1498.

📒 Files selected for processing (3)
  • app/test/test_hash_readwrite.c (8 hunks)
  • app/test/test_hash_readwrite_lf_perf.c (1 hunks)
  • doc/guides/freebsd_gsg/build_dpdk.rst (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/test/test_hash_readwrite.c (1)
app/test/test_hash_readwrite_lf_perf.c (1)
  • init_params (148-192)
🔇 Additional comments (3)
doc/guides/freebsd_gsg/build_dpdk.rst (1)

21-24: Correct directive syntax.

Switching to .. note:: ensures Sphinx renders the admonition properly. Nice catch.

app/test/test_hash_readwrite_lf_perf.c (1)

1313-1316: Good guard on allocator result.

The null check keeps us from handing a NULL pool into the QSBR config, preventing the later dereference-and-jump from running with invalid state.

app/test/test_hash_readwrite.c (1)

149-205: Dynamic sizing looks solid.

Basing the allocation footprint and key initialization on hash_params.entries scales both functional and perf paths consistently, and the new flag makes that intent explicit.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants