|WARNING| [RFC,1/1] ethdev: add per-packet Tx timestamp slot APIs

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review: ethdev TX Timestamp Slot APIs

## Summary
This patch adds experimental per-packet TX timestamp slot APIs to ethdev. The implementation has several correctness bugs and API design issues that must be addressed.

---

## Errors (Must Fix)

### 1. Use-after-free risk: missing slot validation in `rte_eth_timesync_read_tx_timestamp_slot()`

The `slot_id` parameter is user-supplied and never validated against the PMD's allocated slot range. A caller can pass a freed slot ID (after calling `_slot_release()`) or an arbitrary out-of-range value, causing the PMD to access freed memory or out-of-bounds array indices.

**Fix**: Document that PMDs MUST validate `slot_id` against their active slot bitmap/array before dereferencing it. Add a note in the driver-facing ops documentation:
```c
/** @internal Read TX timestamp by slot handle.
 * PMD must validate slot_id is allocated before accessing slot state.
 * Return -EINVAL for invalid slot_id. */
```

### 2. Double-release possible without reference counting

A caller can invoke `rte_eth_timesync_tx_timestamp_slot_release(port_id, slot_id)` twice on the same `slot_id`. The first call frees the slot; the second call causes the PMD to mark an already-free slot as free (potentially corrupting a slot that was reallocated in between).

**Fix**: Document that releasing an already-released slot is undefined behavior and that applications must not call release twice. PMDs should track slot state (allocated/free) and return `-EINVAL` if releasing a non-allocated slot.

### 3. Resource leak on error path in `rte_eth_timesync_tx_slot_dynfield_register()`

When `rte_mbuf_dynfield_register()` succeeds but the subsequent `rte_mbuf_dynflag_register()` fails, the dynfield remains registered but the function returns `-ENOTSUP`. On the next call, the lookup succeeds and `rte_eth_timesync_tx_slot_dynfield_offset >= 0`, so the flag registration is retried but fails again, and the API appears broken.

**Fix**: Either:
- Make dynflag mandatory and return error if it fails, OR
- Make dynflag optional (set to 0 on failure) and document that applications must not rely on `ol_flags` bit for slot presence detection.

Currently the code treats dynflag failure as non-fatal (attempts lookup, continues if not found), but this silently produces a partially initialized state where `stamp_mbuf()` succeeds but the flag bit is invalid.

**Recommended**: Make flag mandatory. Change to:
```c
int flag_bit = rte_mbuf_dynflag_register(...);
if (flag_bit < 0)
    flag_bit = rte_mbuf_dynflag_lookup(...);
if (flag_bit < 0)
    return -ENOTSUP;  /* Fail early if flag unavailable */
rte_eth_timesync_tx_slot_dynflag = RTE_BIT64(flag_bit);
```

### 4. Missing bounds check on `tx_queue_id` in `rte_eth_timesync_tx_timestamp_slot_alloc()`

The `tx_queue_id` is passed to the PMD without validation against `dev->data->nb_tx_queues`. An out-of-range queue ID causes the PMD to access out-of-bounds queue structures.

**Fix**: Add validation:
```c
if (tx_queue_id >= dev->data->nb_tx_queues) {
    RTE_ETHDEV_LOG_LINE(ERR, "Invalid Tx queue %u (max %u)",
        tx_queue_id, dev->data->nb_tx_queues);
    return -EINVAL;
}
```

---

## Warnings (Should Fix)

### 1. API design: opaque slot lifecycle unclear to callers

The API does not specify:
- Whether slots expire (e.g., after packet transmission completes)
- Whether `read_tx_timestamp_slot()` is idempotent (can read multiple times before release)
- Whether a slot can be read before the packet is actually transmitted
- Maximum time between alloc and transmit before the slot becomes invalid

**Suggestion**: Add to the `rte_eth_timesync_tx_timestamp_slot_alloc()` docstring:
```
 * The allocated slot remains valid until explicitly released via
 * rte_eth_timesync_tx_timestamp_slot_release() or until the device is stopped.
 * Slots should be released promptly after timestamp readback to avoid exhausting
 * the PMD's slot pool. A slot not released within a PMD-defined timeout
 * (typically several seconds) may be reclaimed automatically.
```

### 2. Missing release notes entry

This adds a new experimental API family. The release notes should document:
- New per-packet TX timestamp slot APIs (`rte_eth_timesync_tx_timestamp_slot_*`)
- New `struct rte_eth_timesync_dual_domain_timestamp` for dual-domain timestamps
- Rationale: existing `rte_eth_timesync_read_tx_timestamp()` single-latch limitation

**Fix**: Add a section to `doc/guides/rel_notes/release_26_11.rst`:
```rst
New Features
------------

* **Added per-packet TX timestamp slot APIs.**

  Introduced ``rte_eth_timesync_tx_timestamp_slot_alloc()``,
  ``rte_eth_timesync_read_tx_timestamp_slot()``,
  ``rte_eth_timesync_tx_timestamp_slot_release()``, and
  ``rte_eth_timesync_tx_timestamp_stamp_mbuf()`` to enable reliable
  concurrent TX timestamping on PMDs with per-packet slot hardware.

  Added ``struct rte_eth_timesync_dual_domain_timestamp`` to carry
  both adjusted PHC time and free-running cycle-domain time.
```

### 3. Inappropriate use of `rte_malloc()` for slot tracking (if PMD uses it)

This is a control-path operation (allocating a slot from a pool of typically 16-256 entries). PMDs should use `malloc()` or static arrays for slot tracking, not `rte_malloc()` which consumes limited hugepage memory. (Not visible in this patch, but document for PMD implementers.)

**Fix**: Add a note in `ethdev_driver.h` near the ops definitions:
```c
/** @internal Allocate a per-packet TX timestamp slot handle.
 * PMDs should use malloc() or pre-allocated slot pools (not rte_malloc())
 * as slot management is a control-path operation. */
```

### 4. Missing synchronization guidance for slot pool in driver API

The driver-facing ops provide no guidance on whether `slot_alloc`/`slot_release` must be thread-safe. Multi-queue applications will call `slot_alloc(dev, queue_id, ...)` concurrently from different lcores.

**Fix**: Document in `ethdev_driver.h`:
```c
/** @internal Allocate a per-packet TX timestamp slot handle.
 * Must be thread-safe: may be called concurrently from multiple lcores
 * for different tx_queue_id values. PMDs must synchronize access to the
 * global slot pool (e.g., using rte_spinlock_t). */
```

### 5. `valid_mask` field semantics unclear

`struct rte_eth_timesync_dual_domain_timestamp` has a `valid_mask` with two bits defined, but it's unclear whether:
- Both fields are always populated (mask = 0x3)
- PMDs can populate only one domain (mask = 0x1 or 0x2)
- A zero mask means "slot not ready" vs "timestamp failed"

**Fix**: Document in the struct comment:
```c
/**
 * Dual-domain TX timestamp payload.
 *
 * PMDs populate adjusted_ns and/or cycles_ns depending on hardware capability.
 * valid_mask indicates which fields contain valid data:
 * - Bit 0 (RTE_ETH_TIMESYNC_DUAL_DOMAIN_TIMESTAMP_ADJUSTED_VALID): adjusted_ns is valid
 * - Bit 1 (RTE_ETH_TIMESYNC_DUAL_DOMAIN_TIMESTAMP_CYCLES_VALID): cycles_ns is valid
 *
 * A zero valid_mask when rte_eth_timesync_read_tx_timestamp_slot() returns 0
 * indicates the timestamp was lost or unavailable (e.g., packet dropped before Tx).
 */
```

### 6. Missing usage example or documentation

The API sequence (alloc - stamp - tx_burst - poll - read - release) is non-obvious. Without an example, callers may misuse the API (e.g., forget to stamp the mbuf, or release before reading).

**Fix**: Add a minimal usage example to the `rte_eth_timesync_tx_timestamp_slot_alloc()` docstring:
```c
/**
 * Example usage:
 * @code
 * uint32_t slot;
 * struct rte_mbuf *m;
 * struct rte_eth_timesync_dual_domain_timestamp ts;
 *
 * rte_eth_timesync_tx_timestamp_slot_alloc(port, queue, &slot);
 * rte_eth_timesync_tx_timestamp_stamp_mbuf(port, slot, m);
 * rte_eth_tx_burst(port, queue, &m, 1);
 *
 * // Poll until ready
 * while (rte_eth_timesync_read_tx_timestamp_slot(port, slot, &ts) == -EAGAIN)
 *     rte_delay_us_sleep(10);
 *
 * if (ts.valid_mask & RTE_ETH_TIMESYNC_DUAL_DOMAIN_TIMESTAMP_ADJUSTED_VALID)
 *     printf("TX timestamp: %ld ns\n", ts.adjusted_ns);
 *
 * rte_eth_timesync_tx_timestamp_slot_release(port, slot);
 * @endcode
 */
```

---

## Info (Consider)

### 1. Dynflag vs. dynfield redundancy

The code registers both a dynfield (to store `slot_id`) and a dynflag (to mark presence), but the dynflag is never checked in the patch. If the flag is intended for TX burst fast-path filtering, document that PMDs should check `(m->ol_flags & rte_eth_timesync_tx_slot_dynflag)` before reading the dynfield. If the flag is unused, remove it to simplify the API.

### 2. Version number in `RTE_EXPORT_EXPERIMENTAL_SYMBOL`

The patch uses `26.11` but the email headers show 2026-08-17 (August), which is after the 26.11 release would have been cut (November 2026 is 3 months earlier). If this targets 26.11, the patch submission is late. If targeting 27.03, update the version numbers.

---

## Final Recommendation

**Do not merge** until Errors 1-4 are fixed. Warnings 1-2 (API lifecycle documentation and release notes) should also be addressed before merging experimental API.