On Wed, 21 May 2025 12:14:28 +0100 Konstantin Ananyev <[email protected]> wrote:
> First two patches are ‘low risk’ ones. > Third one touches some core functions within rte_ring library and > would probably requires extra reviews/testing from different vendors. > 4th one enables C11 based code on all x86 platforms by default. > The stretch goal for it – make all supported platforms to use C11 > based code and get rid of legacy code in rte_ring_generic_pvt.h. > If there would be some issues with latest two patches – we can limit > ourselves with just first two to apply. > > Konstantin Ananyev (4): > ring: introduce extra run-time checks > ring/soring: fix head-tail synchronization issue > ring: fix potential sync issue between head and tail values > config/x86: enable RTE_USE_C11_MEM_MODEL by default > > config/x86/meson.build | 1 + > lib/ring/rte_ring_c11_pvt.h | 29 ++++++++++++++++++----------- > lib/ring/rte_ring_elem_pvt.h | 8 ++++++-- > lib/ring/rte_ring_hts_elem_pvt.h | 14 ++++++++++---- > lib/ring/rte_ring_rts_elem_pvt.h | 14 ++++++++++---- > lib/ring/soring.c | 2 ++ > 6 files changed, 47 insertions(+), 21 deletions(-) > This patch needs work before being seriously considered. Lots of feedback on this series is not addressed. No longer applies to main, and AI had more to say. === Patch Review: bundle-1678-ring-fix.mbox (via Claude) === # DPDK Patch Review: bundle-1678-ring-fix.mbox ## Overview This patch series (4 patches) addresses ring synchronization issues and introduces runtime checks for the DPDK ring library. The series includes bug fixes for head-tail synchronization problems discovered on ARM platforms and enables C11 memory model by default on x86. --- ## Patch 1/4: ring: introduce extra run-time checks ### Errors 1. **Missing SPDX license identifier in modified files** - The patch modifies existing files but doesn't show the license headers. Assuming they exist in the original files, this is acceptable. 2. **Double space in commit message body** - Line: "return meaningful *entries value" has two spaces before `*entries` ### Warnings 1. **Subject line could be more descriptive** - Current: "ring: introduce extra run-time checks" - The subject is acceptable but could specify "RTE_ASSERT checks" for clarity 2. **Missing Cc: [email protected]** - This patch adds debugging assertions which could be useful for stable releases, though it's primarily a development aid ### Info 1. **Good practice**: Adding `RTE_ASSERT()` checks helps catch programming errors during development 2. **Code change in rte_ring_c11_pvt.h**: Moving `*new_head = *old_head + n;` before the early return is a behavioral change that should be documented in the commit message --- ## Patch 2/4: ring/soring: fix head-tail synchronization issue ### Errors 1. **Missing `Cc: [email protected]`** - This is a bug fix with a `Fixes:` tag, so it should include `Cc: [email protected]` for backport consideration ### Warnings 1. **Commit message body line exceeds 75 characters** - Multiple lines exceed the 75-character limit: - "I believe that we are hitting the following race-condition scenario here:" (72 chars - OK) - "Note that this issue happens only on the second iteration of" (61 chars - OK) - But several lines like "One extra thing to note – I think the same problem potentially exists" appear close to or exceed the limit - Lines with em-dashes (–) and technical content should be wrapped properly 2. **Use of non-ASCII characters** - The commit message uses em-dashes (–) instead of regular hyphens (-) or double hyphens (--) - Example: "i.e. – when we use" should be "i.e., when we use" or "i.e. - when we use" 3. **Commit message starts explanation with scenario numbers but inconsistent formatting** - The numbered steps (1, 2, 3) are good for clarity but formatting varies ### Info 1. **Well-documented bug analysis**: The commit message provides excellent context about the race condition 2. **Alternative solution mentioned**: The commit notes an alternative fix approach (using Acquire-Release for CAS), which is helpful for reviewers 3. **The fix adds memory fences after tail updates** - this is a valid approach but patch 3/4 provides a potentially cleaner solution --- ## Patch 3/4: ring: fix potential sync issue between head and tail values ### Errors 1. **Missing `Cc: [email protected]`** - This is a bug fix with multiple `Fixes:` tags, so it should include `Cc: [email protected]` 2. **Fixes tags may reference commits not in stable branches** - `Fixes: 49594a63147a ("ring/c11: relax ordering for load and store of the head")` - verify this is the correct commit ### Warnings 1. **Commit message body line length issues** - Several lines appear to exceed 75 characters - Technical explanations should be wrapped appropriately 2. **Use of non-ASCII characters** - Similar to patch 2/4, uses em-dashes (–) instead of ASCII hyphens 3. **Incomplete sentence in commit message** - "But, so far I didn't manage to reproduce it in reality." - grammatically fine but informal 4. **Comment style inconsistency in code** ```c /* * on failure, *old_head is updated. * this CAS(ACQ_REL, ACQUIRE) serves as a hoist ``` - Comment starts with lowercase after `/*` which is inconsistent with DPDK style (should start with uppercase or use `/* lowercase */` format consistently) ### Info 1. **This patch supersedes patch 2/4's approach** - The series structure is logical: patch 2 provides a quick fix, patch 3 provides the "better" fix 2. **Removes the fences added in patch 2/4** - This is correct as the CAS ordering change makes them unnecessary 3. **Performance consideration noted** - Good that the author tested performance impact --- ## Patch 4/4: config/x86: enable RTE_USE_C11_MEM_MODEL by default ### Errors None identified. ### Warnings 1. **Missing documentation update** - Enabling `RTE_USE_C11_MEM_MODEL` by default on x86 is a significant change that should be noted in release notes - Consider adding an entry in `doc/guides/rel_notes/release_XX_XX.rst` 2. **Potential ABI/behavior change** - While this shouldn't cause ABI issues, it changes the default behavior on x86 platforms - Applications may see different performance characteristics ### Info 1. **Good direction**: Moving toward C11 memory model is the modern approach 2. **Completes the series**: This patch makes sense in context of the synchronization fixes 3. **Subject line is clear and accurate** --- ## General Series Issues ### Warnings 1. **Series ordering consideration** - Patches 2 and 3 both fix the same underlying issue differently - Patch 3 removes code added by patch 2 - Consider squashing patches 2 and 3, or clearly document in patch 2 that it's a "temporary" fix superseded by patch 3 2. **Missing cover letter context in individual patches** - Each patch should stand alone for `git bisect`, but the relationship between patches 2 and 3 could be clearer ### Info 1. **Good testing evidence**: The author mentions specific test failures and hardware (Ampere Altra Max) 2. **Comprehensive fix approach**: The series addresses the issue at multiple levels --- ## Summary | Severity | Count | Key Issues | |----------|-------|------------| | Error | 2 | Missing `Cc: [email protected]` for bug fixes (patches 2, 3) | | Warning | 10 | Line length issues, non-ASCII characters, missing release notes, comment style | | Info | 8 | Good documentation, well-reasoned fixes, performance consideration | ### Recommended Actions 1. **Add `Cc: [email protected]`** to patches 2/4 and 3/4 2. **Wrap commit message lines** to stay within 75 characters 3. **Replace em-dashes (–)** with regular hyphens or double hyphens 4. **Add release notes entry** for enabling C11 memory model on x86 5. **Consider squashing patches 2 and 3** or better documenting their relationship 6. **Fix double space** in patch 1/4 commit message
