|WARNING| [v2,8/8] doc: add bonding features matrix

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-08-31

# DPDK Patch Review

## Patch 1/8: net/bonding: fix TLB member ordering with unusable member

### Errors

None.

### Warnings

None.

### Info

- **Patch improves error path handling in TLB rebalance callback**
  The callback now skips members where `rte_eth_link_get_nowait()` or `rte_eth_stats_get()` fails, avoiding garbage bandwidth values from uninitialized `member_stats`. Logging only on state change reduces spam (callback runs every millisecond).
  Good correctness fix.

- **Loop counter change: `update_stats = 1` to `update_stats`**
  Replacing literal `1` with truthiness is acceptable for bool-like `uint8_t` but explicit comparison (`update_stats != 0`) would be more consistent with DPDK guidelines. Not an error, but less clear.

---

## Patch 2/8: net/bonding: skip unavailable member stats

### Errors

None.

### Warnings

None.

### Info

- **Correct fix for accumulating stale/uninitialized statistics**
  `bond_ethdev_stats_get()` now skips members whose `rte_eth_stats_get()` fails, preventing accumulation of undefined values. Good correctness improvement.

---

## Patch 3/8: net/bonding: skip unavailable members in device info

### Errors

None.

### Warnings

- **Downgrade to WARNING log level may mask real issues**
  Original code returned `-ENODEV` immediately if any member failed to query, which is correct behavior for `dev_infos_get` -- incomplete information is worse than an error. The new code logs `WARNING` and continues, which may produce misleading `max_rx_queues`/`max_tx_queues` if a member is about to be removed or is not yet probed.
  ```c
  RTE_BOND_LOG(WARNING,
      "Skipping device (port %u) info: %s",
      member.port_id, strerror(-ret));
  continue;
  ```
  Consider: should this remain an error and fail-fast, or at least document that the returned queue limits may be stale if members are being dynamically added/removed?

- **New error path when no members queried**
  ```c
  if (queried == 0) {
      RTE_BOND_LOG(ERR, "No member device info available");
      return -ENODEV;
  }
  ```
  This is correct, but the transition from "any failure is fatal" to "all failures are fatal" is subtle. Document in commit message or code comment why partial failure is acceptable.

### Info

- **Correct handling of concurrent member removal**
  Comment notes members can be unqueryable due to concurrent removal or unprobed state. Skipping them is reasonable for a bonding device.

---

## Patch 4/8: net/bonding: use atomic link status accessors

### Errors

None.

### Warnings

None.

### Info

- **Good fix for concurrent link status access**
  Replacing direct `dev_link` access with `rte_eth_linkstatus_get()`/`rte_eth_linkstatus_set()` provides atomic read/modify/write of the 64-bit `struct rte_eth_link`. Correctness improvement.

- **New helper `bond_ethdev_link_down()`**
  Encapsulates the pattern of reading, modifying `link_status`, and writing back. Clean abstraction.

---

## Patch 5/8: net/bonding: restrict control ops in secondary process

### Errors

None.

### Warnings

- **`rte_eth_bond_8023ad_member_info()` returns `-ENOTSUP` in secondary but still checks `valid_bonding_port_id()` afterward**
  ```c
  if (rte_eal_process_type() != RTE_PROC_PRIMARY)
      return -ENOTSUP;

  if (info == NULL || valid_bonding_port_id(port_id) != 0 ||
          rte_eth_bond_mode_get(port_id) != BONDING_MODE_8023AD)
      return -EINVAL;
  ```
  The secondary check should come *after* argument validation, not before. Otherwise a secondary process calling with `info == NULL` gets `-ENOTSUP` instead of `-EINVAL`, which is confusing.
  Suggest: move `rte_eal_process_type()` check to just before accessing private data, after argument checks.

- **Missing secondary process check in `rte_eth_bond_mode_get()`**
  Several public API functions now block secondary processes, but `rte_eth_bond_mode_get()` (used in the checks above) is not modified. Verify it is safe for secondary to call -- if it reads shared memory only, no change needed; if it accesses primary-private state, add the check.

### Info

- **`secondary_dev_ops` restricts secondary to read-only operations**
  Good design: secondary processes can query stats and link state but cannot reconfigure the bonding device. Matches DPDK process model.

---

## Patch 6/8: net/bonding: add extended statistics

### Errors

None.

### Warnings

- **Dropped `RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS` but no queue stats were ever implemented**
  Release notes claim "per-queue entries, which were always zero, are no longer reported." This is correct -- the flag requested ethdev auto-fill queue stats, but bonding never implemented `dev_ops->queue_stats_mapping_set()` so they were always zero. Removing the flag is the right fix.
  However, existing applications may have been parsing the (zero) queue stats. This is technically an ABI break for the xstats array layout. Not a blocker, but should be noted in release notes as a behavior change.

- **`bond_ethdev_xstats_get()` ignores `rte_eth_stats_get()` failure and reports zero**
  ```c
  /* If member query fails just report zero. */
  if (rte_eth_stats_get(member_id, &member_stats) < 0)
      memset(&member_stats, 0, sizeof(member_stats));
  ```
  This silently hides failures. At minimum, log the failure at `DEBUG` level so the user knows why counters are zero. Or, skip the member entirely (return fewer xstats), similar to patch 2/8.

### Info

- **Good feature addition**
  Per-member statistics make bonding behavior observable, which is valuable for debugging and monitoring. Implementation is straightforward.

---

## Patch 7/8: test/bonding: add extended statistics test

### Errors

None.

### Warnings

None.

### Info

- **Good test coverage**
  Validates xstats name/value query contract, per-member naming, attribution of a burst to the correct member, and reset behavior. Thorough.

---

## Patch 8/8: doc: add bonding features matrix

### Errors

None.

### Warnings

- **`Flow rules are passed through to the members` -- incomplete information**
  The `[rte_flow]` comment says only `eth`, `queue`, and `count` are listed, but bonding in passthrough mode should support all items/actions the members support. The features file should either:
  1. Mark all common items/actions as "P" (passed through), or
  2. Document that only `eth`/`queue`/`count` are used by bonding itself for mode 4, and all others are member-dependent.
  
  Current wording is ambiguous. Suggest:
  ```
  ; Flow rules are passed through to the members. The usable items and
  ; actions are the intersection of all member capabilities. Only the
  ; types used by bonding itself (mode 4 dedicated LACP queue) are
  ; listed below. All other items/actions depend on member support.
  ```

### Info

- **Good documentation addition**
  Bonding now appears in the driver overview table.

---

## Summary

**Errors:** 0  
**Warnings:** 4 (patches 3, 5, 6, 8)  
**Info:** 8 (general observations, no action required)

### Key issues to address:

1. **Patch 3:** Downgrading member query failure to WARNING may hide real issues. Consider failing fast or documenting the partial-failure semantics.

2. **Patch 5:** Secondary process check in `rte_eth_bond_8023ad_member_info()` comes before argument validation, which produces confusing error codes. Move check after argument validation.

3. **Patch 6:** `bond_ethdev_xstats_get()` silently reports zero for failed member stats queries. Log the failure or skip the member.

4. **Patch 8:** Flow rule passthrough documentation is ambiguous. Clarify that only bonding's own usage is listed, and passthrough items depend on member capabilities.

### Correctness improvements noted:

- Patches 1, 2, 4 fix real bugs (uninitialized stats, stale counters, non-atomic link status access).
- Patch 5 correctly restricts secondary process to read-only operations.
- Patches 6, 7 add valuable observability with good test coverage.