|WARNING| [v9,25/25] eal: remove rte_atomicNN functions

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: WARNING
https://dpdk.org/patch/170252

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-09-29

# Review of DPDK Atomic Conversion Patch Series

This is a comprehensive patch series converting DPDK from the deprecated `rte_atomicNN_*()` API to C11 stdatomic operations. I'll review each patch for correctness bugs, style compliance, and API requirements per AGENTS.md.

---

## Patch 01/25: bpf: use C11 atomics in BPF_ST_ATOMIC_REG

**Correctness:**
 No issues. The conversion from `rte_atomic32_add`/`rte_atomic32_exchange` to `rte_atomic_fetch_add_explicit`/`rte_atomic_exchange_explicit` with `rte_memory_order_seq_cst` preserves the Linux kernel BPF interpreter semantics and the implicit ordering of the legacy API.

**Style:**
 Follows DPDK C style guidelines.

---

## Patch 02/25: net/bonding: convert to stdatomic

**Correctness - Error:**

In `rte_eth_bond_8023ad.c` around line 1358:
```c
while (!rte_atomic_compare_exchange_weak_explicit(&port->rx_marker_timer,
                    &old_marker_timer, marker_timer,
                    rte_memory_order_release, rte_memory_order_acquire));
```

The loop unconditionally retries the CAS without re-checking `timer_is_expired()`. If another thread sets the timer before this thread's CAS succeeds, this thread should bail out (WRN_RX_MARKER_TO_FAST), not spin until it wins. The original code re-read `port->rx_marker_timer` inside the loop body, which implicitly re-checked expiration. With weak CAS in a tight loop, that check is lost.

**Suggested fix:**
```c
do {
    old_marker_timer = rte_atomic_load_explicit(&port->rx_marker_timer,
                                                rte_memory_order_acquire);
    if (!timer_is_expired(&old_marker_timer)) {
        wrn = WRN_RX_MARKER_TO_FAST;
        goto free_out;
    }
    timer_set(&marker_timer, mode4->rx_marker_timeout);
} while (!rte_atomic_compare_exchange_weak_explicit(&port->rx_marker_timer,
            &old_marker_timer, marker_timer,
            rte_memory_order_release, rte_memory_order_acquire));
```

**Style:**
 Follows DPDK C style guidelines.

---

## Patches 03-09: Driver atomic conversions

**Correctness:**
 Patches 03 (net/nbl remove unused field), 04 (net/ena statistics), 05 (net/failsafe refcnt), 06 (net/enic statistics), 07 (net/pfe linkstatus helpers), 08 (net/sfc restart flag), and 09 (crypto/ccp free count) all appear correct. The ena/enic/ccp patches correctly note that statistics do not require atomics under DPDK's stat model and drop them appropriately. Failsafe's refcnt conversion properly uses acquire on claim and release on drop.

**Style:**
 All follow DPDK C style guidelines.

---

## Patch 10/25: bus/dpaa: remove unused in_use field

**Correctness:**
 The field `atomic_t in_use` was set once and never read. Removal is safe.

**Style:**
 Follows DPDK C style guidelines.

---

## Patch 11/25: bus/dpaa: replace rte_atomic16 with stdatomic

**Correctness:**
 Conversion of the global portal allocation flag from `rte_atomic16_test_and_set`/`rte_atomic16_clear` to `rte_atomic_exchange_explicit` with acquire/release ordering is correct. The acquire on claim pairs with the release on free.

**Style:**
 Follows DPDK C style guidelines.

---

## Patch 12/25: bus/fslmc: replace rte_atomic with stdatomic

**Correctness:**
 Device/queue refcnt and in_use flags converted correctly. Initialization now uses relaxed store (explicit) instead of `rte_atomic*_init()` which was a plain store. Claim uses acquire CAS, drop uses release store. The commit message correctly notes this is a Dekker-style pattern and requires seq_cst on both legs in the original, and the new code preserves that with explicit acquire on CAS and release on store.

**Style:**
 Follows DPDK C style guidelines. The launder of packed struct alignment warnings is acceptable per the note.

---

## Patch 13/25: common/dpaax: remove unused atomic wrappers

**Correctness:**
 The removed wrappers were unused. Removal is safe.

**Style:**
 Follows DPDK C style guidelines.

---

## Patch 14/25: net/netvsc: replace rte_atomic32 with stdatomic

**Correctness:**
 Transaction ID and buffer outstanding counters converted correctly. The relaxed loads in the fast-path early exits are acceptable as noted in the commit message (racy is fine, caught by seq_cst later). The seq_cst CAS on `rndis_pending` preserves the publish-subscribe ordering the old `rte_atomic32_cmpset` + `rte_smp_wmb()` pair provided.

**Style:**
 Follows DPDK C style guidelines.

---

## Patch 15/25: event/sw: convert from rte_atomic32 to stdatomic

**Correctness:**
 Inflight counter conversion is correct. Relaxed on fast-path loads, acquire on credit grant, release on credit return matches the intent. Direct assignment for initialization (before threads launch) is acceptable per AGENTS.md.

**Style:**
 Follows DPDK C style guidelines.

---

## Patch 16/25: bus/vmbus: convert from rte_atomic to stdatomic

**Correctness:**
 Producer windex CAS now uses weak exchange (retry is cheap), and the publish uses `rte_wait_until_equal_32` + release store. The acquire wait pairs with the prior producer's release, carrying their data into this thread's happens-before set so the release store publishes all prior data. This is a subtle but correct improvement over the prior `rte_smp_wmb()` + spin-on-CAS.

**Style:**
 Follows DPDK C style guidelines. The (uintptr_t) launder for alignment is acceptable per the commit message.

---

## Patch 17/25: net/bnx2x: convert from rte_atomic32 to stdatomic

**Correctness:**
 The `scan_fp` flag conversion to seq_cst stores/loads preserves the notification semantics between `bnx2x_sp_post` and `bnx2x_handle_fp_tq`.

**Style:**
 Follows DPDK C style guidelines.

---

## Patch 18/25: drivers/event: replace rte_atomic32 in selftests

**Correctness:**
 Test code converted correctly. Relaxed loads on progress checks, release on dequeue path (dpaa2/octeontx).

**Style:**
 Follows DPDK C style guidelines.

---

## Patch 19/25: net/hinic: replace rte_atomic32 with stdatomic

**Correctness:**
 DMA pool inuse counter and alloc counter converted correctly. Relaxed ordering is acceptable for these local accounting fields.

**Style:**
 Follows DPDK C style guidelines. Format change from %d/%u is correct.

---

## Patch 20/25: net/txgbe: replace rte_atomic32 with stdatomic

**Correctness - Info:**
The commit message notes this fixes a bug where the old `rte_atomic32_test_and_set` return was inverted. That fix is queued separately in next-net. This patch correctly uses acquire exchange / release store for the mailbox lock.

**Style:**
 Follows DPDK C style guidelines.

---

## Patch 21/25: net/vhost: use stdatomic for state flags

**Correctness:**
 Control-plane flags converted to relaxed loads/stores as noted in comments. These are indeed only control hints; the real handshake is on `allow_queuing`/`while_queuing` (converted in patch 22).

**Style:**
 Follows DPDK C style guidelines.

---

## Patch 22/25: net/vhost: use stdatomic instead of rte_atomic32

**Correctness:**
 The Dekker-style handshake on `allow_queuing`/`while_queuing` now uses seq_cst as required. Both legs store their own flag seq_cst then load the peer's seq_cst, forbidding store-load reordering. The prior `rte_atomic32_set`/`read` were plain volatile accesses and did NOT provide ordering on weakly-ordered ISAs, so this closes a latent bug. The data-path exit store of `while_queuing=0` is release, ordering mbuf/slot accesses before the control thread observes idle. Excellent commit message explaining the ordering requirements.

**Style:**
 Follows DPDK C style guidelines.

---

## Patch 23/25: vdpa/ifc: replace rte_atomic32 with stdatomic

**Correctness:**
 `started`, `dev_attached`, and `running` flags converted to `RTE_ATOMIC(bool)` with seq_cst loads/stores. This is conservative but safe given the control-plane usage (config/close/attach/detach).

**Style:**
 Follows DPDK C style guidelines. Narrowing to bool and grouping with existing bools is a nice cleanup.

---

## Patch 24/25: test/atomic: drop old rte_atomicNN tests

**Correctness:**
 Old API tests removed, only 128-bit CAS test remains. Acceptable given the API removal in patch 25.

**Style:**
 Follows DPDK C style guidelines.

---

## Patch 25/25: eal: remove rte_atomicNN functions

**Correctness:**
 Removal is appropriate given all callers have been converted.

**Style:**
 Follows DPDK C style guidelines.

**API/Documentation:**
 Release notes updated correctly. Deprecation notice removed from `deprecation.rst`. Release notes include the API removal with guidance to use C11 atomics.

---

## Summary

**Errors (must fix):**
1. **Patch 02/25 (net/bonding):** Weak CAS loop on `rx_marker_timer` drops the expiration re-check. Must re-load and re-validate inside loop.

**Warnings:** None.

**Info:**
- Patch 20/25 references a separate bug fix queued in next-net (acceptable, just noting).

**Overall Assessment:**
This is a high-quality, well-documented series. The conversion is methodical, the commit messages explain the ordering choices clearly (especially patches 16, 22), and the removal of the deprecated API is appropriate. Fix the bonding loop condition and this is ready.