Re: [PATCH v1 0/4] ring: some fixes and improvements

Stephen Hemminger
2026-01-23T05:01:57Z

On Wed, 21 May 2025 12:14:28 +0100
Konstantin Ananyev <konstantin.ananyev@huawei.com> 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: stable@dpdk.org**
- 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: stable@dpdk.org`**
- This is a bug fix with a `Fixes:` tag, so it should include `Cc: stable@dpdk.org` 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: stable@dpdk.org`**
- This is a bug fix with multiple `Fixes:` tags, so it should include `Cc: stable@dpdk.org`

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: stable@dpdk.org` 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: stable@dpdk.org`** 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