|WARNING| [v2] pdump: fix teardown race with opt-in MP request mode

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-07-05

# DPDK Patch Review

## Patch: pdump: fix teardown race with opt-in MP request mode

---

## Errors

### 1. `pdump_wait_inflight()` busy-wait without CPU yield when timeout remains

**File:** `lib/pdump/rte_pdump.c`
**Line:** ~154

The loop busy-waits checking `ctl->inflight` but only calls `rte_pause()` which is a hint to the CPU, not a scheduling yield. On a busy system with many lcores, this can burn cycles unnecessarily when the timeout window is long (5 seconds).

**Fix:** Add a short sleep or explicitly yield to the scheduler periodically:
```c
static int
pdump_wait_inflight(struct pdump_shared_ctl *ctl)
{
	uint64_t end_tsc = rte_get_timer_cycles() + MP_TIMEOUT_S * rte_get_timer_hz();
	unsigned int backoff = 0;

	while (rte_atomic_load_explicit(&ctl->inflight, rte_memory_order_acquire) != 0) {
		if (rte_get_timer_cycles() > end_tsc)
			return -ETIMEDOUT;
		rte_pause();
		if (++backoff > 1000) {
			usleep(1);
			backoff = 0;
		}
	}

	return 0;
}
```

---

### 2. `pdump_enter()` performs two separate loads of `generation` without ensuring consistency

**File:** `lib/pdump/rte_pdump.c`
**Line:** ~120-133

`pdump_enter()` loads `generation` before taking the inflight reference, then loads it again after incrementing inflight. If generation is incremented between these two loads by a concurrent disable, the second check may pass even though the callback entered with a stale generation. The `enabled` flag protects against acting on the data, but the function's intent is to check both conditions atomically with the inflight increment.

**Fix:** Load generation once and use that value for both checks:
```c
static int
pdump_enter(struct pdump_shared_ctl *ctl, const struct pdump_rxtx_cbs *cbs)
{
	uint32_t generation = rte_atomic_load_explicit(&pdump_stats->generation,
						       rte_memory_order_relaxed);

	if (rte_atomic_load_explicit(&ctl->enabled, rte_memory_order_relaxed) == 0 ||
	    cbs->generation != generation)
		return 0;

	rte_atomic_fetch_add_explicit(&ctl->inflight, 1, rte_memory_order_acquire);

	/* Re-check enabled after taking inflight reference so disable can wait. */
	if (rte_atomic_load_explicit(&ctl->enabled, rte_memory_order_relaxed) != 0 &&
	    cbs->generation == generation)  /* Use the same generation value */
		return 1;

	rte_atomic_fetch_sub_explicit(&ctl->inflight, 1, rte_memory_order_release);
	return 0;
}
```

---

### 3. `__pdump_request()` increments generation on DISABLE before calling `set_pdump_rxtx_cbs()`, but `set_pdump_rxtx_cbs()` stores the new generation into `cbs->generation` on ENABLE, not DISABLE

**File:** `lib/pdump/rte_pdump.c`
**Line:** ~757-765, ~368, ~469

On DISABLE, `__pdump_request()` increments generation and stores it in `req->generation`. Then `set_pdump_rxtx_cbs()` calls `pdump_register_rx_callbacks()` and `pdump_register_tx_callbacks()`, which set `ctl->enabled = 0` but do NOT update `cbs->generation`. The new generation is only written to `cbs->generation` on ENABLE (lines ~368, ~469). This means a callback entering after disable but before the next enable will compare `cbs->generation` (old value) against the global generation (new value) and incorrectly bail out even if a subsequent ENABLE has not yet occurred.

**Analysis:** The generation mechanism's goal is to make stale callbacks no-op. The current code increments generation on DISABLE, but the per-queue `cbs->generation` is only updated on ENABLE. This leaves a window where `cbs->generation` is stale (old) while the global generation is new, causing callbacks to exit even when they should still be active.

**Fix:** Either:
1. Update `cbs->generation` on DISABLE to match the incremented global generation, OR
2. Only increment generation on ENABLE and use generation to detect stale enable cycles, not disable cycles.

The intent appears to be option 2 (generation tracks enable cycles). In that case, remove the generation increment from DISABLE:

```c
/* In __pdump_request(): */
if (req->op == ENABLE) {
	req->generation = rte_atomic_fetch_add_explicit(&pdump_stats->generation, 1,
						   rte_memory_order_acq_rel) + 1;
} /* No generation update on DISABLE */
```

And ensure `cbs->generation` is only checked against the global generation when `enabled == 1`.

---

### 4. Potential resource leak: `pdump_register_rx_callbacks()` and `pdump_register_tx_callbacks()` reconcile stale callbacks but do not free the old filter BPF

**File:** `lib/pdump/rte_pdump.c`
**Line:** ~343-361, ~444-460

When reconciling a stale callback (lines ~343-361 for RX, ~444-460 for TX), the code removes the old callback and waits for it to drain, but it does not release the old `cbs->filter` BPF before overwriting it with the new filter. If the old callback had a BPF filter, it leaks.

**Fix:** Before overwriting `cbs->filter`, free the old filter if it exists:
```c
/* After pdump_cb_wait(cbs); and before assigning new values: */
if (cbs->filter != NULL) {
	rte_bpf_destroy(cbs->filter);
	cbs->filter = NULL;
}
```

Note: The filter is stored as `const struct rte_bpf *`, and `rte_bpf_destroy()` takes non-const, so you may need to cast or adjust the field type.

---

### 5. `rte_mp_request_sync_ex()` stub on Windows does not set `rte_errno` before returning -1

**File:** `lib/eal/windows/eal_mp.c`
**Line:** ~66

The Windows stub returns -1 but the calling code in pdump checks `rte_errno` after a failure. The stub should set `rte_errno` to `ENOTSUP` for consistency with other stubs in the same file.

**Fix:**
```c
int
rte_mp_request_sync_ex(struct rte_mp_msg *req, struct rte_mp_reply *reply,
	const struct timespec *ts, uint32_t flags)
{
	RTE_SET_USED(req);
	RTE_SET_USED(reply);
	RTE_SET_USED(ts);
	RTE_SET_USED(flags);
	rte_errno = ENOTSUP;
	EAL_LOG_NOT_IMPLEMENTED();
	return -1;
}
```

---

## Warnings

### 1. `pdump_request_to_secondary_sync()` does not free `mp_reply.msgs` on the early return path (timeout)

**File:** `lib/pdump/rte_pdump.c`
**Line:** ~646-647

If `rte_mp_request_sync_ex()` fails (returns != 0), the function logs the error and returns `-rte_errno` without calling `free(mp_reply.msgs)`. However, the documentation for `rte_mp_request_sync()` states "the caller is responsible for free reply->msgs". It is unclear whether `reply->msgs` is allocated on failure, but the safe pattern is to always free it.

**Fix:**
```c
if (rte_mp_request_sync_ex(&mp_req, &mp_reply, &ts,
			    RTE_MP_REQ_F_IGNORE_NO_ACTION) != 0) {
	PDUMP_LOG_LINE(ERR, "rte_mp_request_sync failed: %s",
		      strerror(rte_errno));
	ret = -rte_errno;
	goto cleanup;
}

/* ... rest of function ... */

cleanup:
	free(mp_reply.msgs);
	return ret;
```

Or verify in the EAL code that `msgs` is NULL on error and document that behavior.

---

### 2. Release notes entry does not document the new EAL API `rte_mp_request_sync_ex()`

**File:** `doc/guides/rel_notes/release_26_07.rst`
**Line:** ~180-185

The patch adds a new public EAL function `rte_mp_request_sync_ex()` but the release notes only mention the pdump hardening. New API additions should be documented separately.

**Fix:** Add an entry under "New Features":
```rst
* **Added opt-in multiprocess request mode to EAL.**

  Added ``rte_mp_request_sync_ex()`` to allow callers to request that peers
  without a registered action return ``MP_IGN`` instead of causing a timeout.
  This enables intent-driven APIs like pdump to ignore unrelated secondary
  processes while preserving legacy behavior for existing users of
  ``rte_mp_request_sync()``.
```

---

### 3. `pdump_stats` global variable not prefixed with `pdump_` or library-specific prefix

**File:** `lib/pdump/rte_pdump.c`
**Line:** ~111

The static global variable `pdump_stats` does not follow the library naming convention. While it is static (file scope), the guideline recommends prefixing all global variables in libraries for clarity and to avoid clashes when code is copied or refactored.

**Fix (low priority):** Rename to `pdump_global_stats` or similar.

---

### 4. `disable_pdump()` in `app/pdump/main.c` can return without setting `rte_errno` on the `EINVAL` path

**File:** `app/pdump/main.c`
**Line:** ~463

The else branch sets `rte_errno = EINVAL` and returns `-EINVAL`, which is correct, but it is more typical in DPDK to return `-1` and rely on `rte_errno` being set by the called function or set it explicitly. The current pattern is acceptable but inconsistent with the branches above it (which return the result of `rte_pdump_disable*` directly).

**Suggested improvement (style consistency):**
```c
static inline int
disable_pdump(struct pdump_tuples *pt)
{
	if (pt->dump_by_type == DEVICE_ID)
		return rte_pdump_disable_by_deviceid(pt->device_id, pt->queue,
						     pt->dir);
	if (pt->dump_by_type == PORT_ID)
		return rte_pdump_disable(pt->port, pt->queue, pt->dir);

	rte_errno = EINVAL;
	return -1;
}
```

---

### 5. Comment in `cleanup_pdump_resources()` says "Disable all callbacks first" but does not mention the fail-closed behavior

**File:** `app/pdump/main.c`
**Line:** ~528

The comment should clarify that the function is fail-closed: if any disable fails, the function returns early without tearing down shared resources.

**Fix:**
```c
/* Disable all callbacks first. If any disable fails, skip teardown
 * to avoid stale callback access (fail-closed behavior). */
```

---

## Info

### 1. `pdump_enter()` could use a single atomic operation instead of load-then-fetch_add

**File:** `lib/pdump/rte_pdump.c`
**Line:** ~127

The pattern of loading `enabled`, then incrementing `inflight`, then re-checking `enabled` is standard for ref-counted enable/disable. An alternative would be to use a single atomic compare-and-swap on a combined state word, but the current approach is clear and correct.

No change needed; this is for awareness.

---

### 2. `pdump_wait_inflight()` timeout is hardcoded to `MP_TIMEOUT_S` (5 seconds)

**File:** `lib/pdump/rte_pdump.c`
**Line:** ~147

The timeout is the same as the MP request timeout, which is reasonable, but it is not configurable. If a callback is truly stuck, 5 seconds is a long time for a control-plane operation.

Consider: exposing a tunable timeout parameter in a future enhancement, or logging a progress message every second during the wait.

---

### 3. Stale callback reconciliation logs at `DEBUG` level, which may be too low for production debugging

**File:** `lib/pdump/rte_pdump.c`
**Line:** ~347, ~448

The "reconciling stale callback" message is logged at DEBUG level. In production, this would not be visible unless debug logging is enabled. If stale callbacks are a sign of a race or timeout, it might be worth logging at INFO or WARNING level.

**Consideration:** Change to `PDUMP_LOG_LINE(INFO, ...)` for the reconciliation message.

---

### 4. The patch adds `errno.h` include but does not use `errno` directly

**File:** `lib/pdump/rte_pdump.c`
**Line:** ~6

The include of `<errno.h>` is added but the only new errno usage is `ETIMEDOUT`, which is already available via `<rte_errno.h>`. The include may be redundant.

**Verify:** Check if `ETIMEDOUT` requires `<errno.h>` or if it is available from DPDK headers. If redundant, remove the include.

---

## Summary

**Critical issues to address:**
- Error 1: `pdump_wait_inflight()` busy-wait should yield periodically
- Error 2: `pdump_enter()` generation consistency
- Error 3: Generation update logic mismatch between ENABLE and DISABLE
- Error 4: Filter BPF leak on stale callback reconciliation
- Error 5: Windows stub missing `rte_errno = ENOTSUP`

**Important improvements:**
- Warning 1: `mp_reply.msgs` leak on error path
- Warning 2: Document new EAL API in release notes

**Minor/Informational:**
- Warnings 3-5, Info 1-4: Style, consistency, and maintainability improvements

The core fix (adding generation tracking and waiting for inflight callbacks) is sound, but the generation update logic needs refinement, and resource cleanup (BPF filter) must be added to avoid leaks.