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 seems to have gotten stuck and never reviewed thoroughly or merged.
Turned to AI for insight. It found minor things that could be fixed up
when merged. But would be good to have a release note.
Also concerning that the test did not pass CI cleanly.
## DPDK Ring Patchset Review
**Series:** ring head/tail synchronization fixes and C11 memory model
enablement
**Author:** Konstantin Ananyev (Huawei)
**Patches:** 4
---
### Patch 1/4: `ring: introduce extra run-time checks`
**Commit Message:**
| Check | Status | Notes |
|-------|--------|-------|
| Subject ≤60 chars | ✓ | 44 characters |
| Lowercase after colon | ✓ | |
| Component prefix | ✓ | `ring:` is correct |
| Imperative mood | ✓ | |
| No trailing period | ✓ | |
| Signed-off-by | ✓ | |
**Code Review:**
The patch adds `RTE_ASSERT()` calls to verify `*entries` and `*free_entries`
remain ≤ `r->capacity` after `move_head()` calls. This is a sensible defensive
check.
**Issue (Warning):** In `rte_ring_c11_pvt.h`, the code reordering moves
`*new_head = *old_head + n;` before the `n == 0` check:
```c
+ *new_head = *old_head + n;
if (n == 0)
return 0;
- *new_head = *old_head + n;
```
This is functionally correct (setting `*new_head` even when returning 0 is
harmless), but the change seems unrelated to adding asserts. If intentional,
the commit message should explain why.
**Verdict:** ✓ Acceptable with minor observation
---
### Patch 2/4: `ring/soring: fix head-tail synchronization issue`
**Commit Message:**
| Check | Status | Notes |
|-------|--------|-------|
| Subject ≤60 chars | ✓ | 47 characters |
| Lowercase after colon | ✓ | |
| Component prefix | ✓ | `ring/soring:` correct |
| Fixes tag | ✓ | `Fixes: b5458e2cc483` present |
| Signed-off-by | ✓ | |
| Body ≤75 chars | ✓ | Lines properly wrapped |
**Technical Analysis:**
Excellent, detailed commit message explaining the race condition discovered on
Ampere Altra Max. The fix adds
`rte_atomic_thread_fence(rte_memory_order_release)` after `store(&tail)`
operations.
**Code Review:**
```c
+ /* make sure that new tail value is visible */
+ rte_atomic_thread_fence(rte_memory_order_release);
```
This ensures the tail update is visible before any subsequent head updates.
However, I note this is described as a workaround—patch 3/4 provides the
"better" fix at the CAS level.
**Question:** If patch 3/4 provides the comprehensive fix, why is patch 2/4
kept separate? The comment mentions observing on Ampere Altra Max—should `Cc:
[email protected]` be added since this is a real bug fix?
**Verdict:** ✓ Acceptable (consider adding `Cc: [email protected]`)
---
### Patch 3/4: `ring: fix potential sync issue between head and tail values`
**Commit Message:**
| Check | Status | Notes |
|-------|--------|-------|
| Subject ≤60 chars | ✗ **ERROR** | **63 characters** (limit is 60) |
| Fixes tags | ✓ | Multiple Fixes present |
| Signed-off-by | ✓ | |
**Error:** Subject line exceeds 60-character limit:
```
ring: fix potential sync issue between head and tail values
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
63 characters
```
**Suggested fix:**
```
ring: fix head-tail sync in C11 memory model
```
**Technical Analysis:**
This is the core fix—changing CAS memory ordering from RELAXED to ACQ_REL:
```c
- rte_memory_order_relaxed,
- rte_memory_order_relaxed);
+ rte_memory_order_acq_rel,
+ rte_memory_order_acquire);
```
The comments explaining the change are well-written:
```c
+ * this CAS(ACQ_REL, ACQUIRE) serves as a hoist
+ * barrier to prevent:
+ * - OOO reads of cons tail value
+ * - OOO copy of elems from the ring
+ * Also RELEASE guarantees that latest tail value
+ * will become visible before the new head value.
```
**Code Review Issues:**
1. **Redundant fences in soring.c removed:** The patch removes the fences added
in patch 2/4. This creates a forward dependency—patch 2/4 adds them, patch 3/4
removes them. This violates the "each commit must compile independently"
guideline. Consider squashing patches 2 and 3, or reordering so patch 3's
ACQ_REL fix comes first.
2. **Missing `Cc: [email protected]`:** This fixes real bugs that could affect
production systems.
3. The d->head load changes from RELAXED to ACQUIRE:
```c
*old_head = rte_atomic_load_explicit(&d->head,
- rte_memory_order_relaxed);
+ rte_memory_order_acquire);
```
Combined with removing the explicit thread fence, this is cleaner but changes
the synchronization model significantly. The comment update is appropriate.
**Verdict:** ✗ Needs revision (subject too long, patch ordering concern)
---
### Patch 4/4: `config/x86: enable RTE_USE_C11_MEM_MODEL by default`
**Commit Message:**
| Check | Status | Notes |
|-------|--------|-------|
| Subject ≤60 chars | ✓ | 52 characters |
| Component prefix | ✓ | `config/x86:` correct |
| Signed-off-by | ✓ | |
**Code Review:**
Simple config change:
```c
+dpdk_conf.set('RTE_USE_C11_MEM_MODEL', true)
```
**Concerns:**
1. **Missing release notes:** Enabling C11 memory model by default on x86 is a
significant behavioral change. Per AGENTS.md, API/config changes warrant
release notes updates.
2. **Documentation:** Should document this change in programmer's guide or
platform notes.
3. **Dependency on patch 3:** This change depends on the memory ordering fixes
in patch 3/4 being correct. The commit message acknowledges this ("step towards
unification...removing rte_ring_generic_pvt.h") but the series should ensure
the fixes are validated before enabling C11 model by default.
**Verdict:** ⚠ Acceptable with warning (needs release notes)
---
## Summary
| Patch | Status | Issues |
|-------|--------|--------|
| 1/4 | ✓ OK | Minor: unexplained code reorder |
| 2/4 | ⚠ Warning | Missing `Cc: [email protected]` |
| 3/4 | ✗ **ERROR** | Subject 63 chars (max 60), patch ordering issue |
| 4/4 | ⚠ Warning | Missing release notes |
## Recommendations
1. **Fix patch 3/4 subject line** — shorten to under 60 characters
2. **Consider squashing patches 2+3** — patch 2 adds fences, patch 3 removes
them; this creates odd bisect behavior
3. **Add `Cc: [email protected]`** to patches 2 and 3 as they fix real bugs
4. **Add release notes** for patch 4/4 documenting the C11 memory model
enablement on x86
5. The technical analysis of the memory ordering issue is thorough and the fix
approach (ACQ_REL on CAS) is sound for weakly-ordered architectures
