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

[Stephen Hemminger <stephen@networkplumber.org>]

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