|WARNING| [13/13] examples/eventdev_pipeline: make signal handler safe

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## Summary

This series removes async-signal-unsafe operations from signal handlers across multiple DPDK examples and applications. The patches generally set a flag in the handler and defer work to main context, which is the correct pattern.

---

## Patch 01/13: graph: do not call printf in signal

**No issues found.**

---

## Patch 02/13: examples: remove printf from signal handler

**No issues found.**

---

## Patch 03/13: examples/vmdq: do not print from signal handler

**Error: Unsafe shared variable access**

`stats_requested` is written from signal context and read from main lcore, but is declared `volatile sig_atomic_t`. While `sig_atomic_t` guarantees atomic access, `volatile` does not provide memory ordering between threads (main lcore vs signal delivery on potentially different lcore).

Correct fix:
```c
static _Atomic sig_atomic_t stats_requested;

/* In handler */
atomic_store_explicit(&stats_requested, 1, memory_order_relaxed);

/* In lcore_main */
if (atomic_load_explicit(&stats_requested, memory_order_relaxed) && ...) {
    atomic_store_explicit(&stats_requested, 0, memory_order_relaxed);
    print_stats();
}
```

Note: `sig_atomic_t` is for signal-to-same-thread communication. For signal-to-different-thread, use `rte_atomic_*_explicit()` wrappers around `_Atomic sig_atomic_t`.

---

## Patch 04/13: examples/symmetric_mp: do not print or exit in handler

**Error: Missing volatile on shared flag**

`quit` is a plain `int` set by signal handler and read by worker loops on other lcores. Without `volatile`, the compiler can hoist the load out of the loop, preventing the workers from ever seeing the change.

Correct fix:
```c
static volatile sig_atomic_t quit;
```

The patch declared it `volatile sig_atomic_t` in the handler but not where it's used in `lcore_main()`. The declaration must be in the shared location visible to both (file scope).

---

## Patch 05/13: examples/vdpa: make signal handler safe

**No issues found.**

---

## Patch 06/13: examples/vhost: make signal handler safe

**No issues found.**

---

## Patch 07/13: examples/vhost_blk: do not tear down from signal handler

**No issues found.**

---

## Patch 08/13: examples/ntb: do not print and re-raise from signal handler

**Warning: Unsafe shared variable access**

`conf->stopped` is marked `volatile uint8_t` which is correct for signal-to-same-thread, but the signal may be delivered to any lcore. If the signal arrives on lcore A and sets `stopped` for lcore B's streams, lcore B reads it without memory ordering.

This is a gray area: if the signal is always delivered to a specific lcore, `volatile` suffices. But DPDK signal handling doesn't guarantee delivery lcore, so strictly speaking this should be:
```c
volatile _Atomic uint8_t stopped;
```
with `rte_atomic_store_explicit(..., memory_order_relaxed)` in the handler.

Severity: **Warning** (depends on signal delivery behavior, which is unspecified for multi-threaded DPDK apps).

---

## Patch 09/13: examples/ipsecgw: do not print from signal handler

**No issues found.**

---

## Patch 10/13: examples/l2fwd-macsec: remove print in signal handler

**No issues found.**

---

## Patch 11/13: examples/ethtool: fix exit flag and unchecked cmdline

**Error: Missing volatile on shared flag**

The patch changes `exit_now` to `volatile int`, which is correct for the signal-handler case but insufficient if `exit_now` is set on one lcore and read on another (the patch doesn't show the full context, but `worker_main()` polling it suggests multi-lcore access).

If `exit_now` is only set by main and read by workers after fork/launch, `volatile` suffices. If it can be set by any lcore, should be `volatile _Atomic int` with `rte_atomic_*_explicit()`.

Assuming signal-only writes: the patch is correct. If there are non-signal writes from other lcores, add atomics.

The `cmdline_stdin_new()` check is good.

---

## Patch 12/13: examples/vmdq_dcb: allow exit on signal

**Error: Missing volatile on shared flag**

`quit` is declared `volatile sig_atomic_t`, which is correct for signal writes. However, the patch does not show where `quit` is declared; if it's at file scope, it must be visible to both `signal_handler` and `lcore_main`. Assuming the declaration is correct, no issue.

Actually, re-reading: the patch declares `quit` at file scope before `signal_handler()`, which is correct. **No issue.**

---

## Patch 13/13: examples/eventdev_pipeline: make signal handler safe

**Warning: Potential race on `fdata->done`**

The handler reads `fdata->done` (first `if` checks it, then second `if` sets it). `fdata->done` is not shown in the patch but is presumably a plain `int` in a shared structure. If another thread is concurrently writing `fdata->done`, the handler's read races.

The pattern:
```c
if (fdata->done)
    _exit(1);
fdata->done = 1;
```
is safe only if `fdata->done` is `volatile sig_atomic_t` or `_Atomic`. Without seeing the declaration of `fdata`, cannot confirm this is safe. If `fdata->done` is a plain `int`, this is a data race.

The change to `cdata.dump_dev_signal` from `int` to `volatile sig_atomic_t` is correct.

**Recommendation:** Ensure `fdata->done` is `volatile sig_atomic_t` or `_Atomic int`.

---

## Summary of Findings

### Errors

1. **Patch 03/13** (`examples/vmdq`): `stats_requested` written from signal and read from different lcore without atomic operations. Use `_Atomic sig_atomic_t` with `rte_atomic_*_explicit()`.

2. **Patch 04/13** (`examples/symmetric_mp`): `quit` must be `volatile sig_atomic_t` at file scope visible to both handler and `lcore_main()`. Patch declares it in handler only.

3. **Patch 11/13** (`examples/ethtool`): Insufficient information to confirm `exit_now` is only signal-written. If non-signal writes exist, needs atomics. Likely okay as-is, but flag for review.

### Warnings

4. **Patch 08/13** (`examples/ntb`): `conf->stopped` is `volatile uint8_t`, which is correct for same-thread signal handling but may race if signal is delivered to a different lcore than the one reading. Recommend `_Atomic uint8_t` with relaxed ordering.

5. **Patch 13/13** (`examples/eventdev_pipeline`): `fdata->done` must be `volatile sig_atomic_t` or `_Atomic` for the handler's read/write to be safe. Cannot confirm from patch; flag for verification.

---

## Style and Process

All patches follow DPDK coding style. No forbidden tokens, no formatting issues. The approach--set flag, defer work to main--is the correct pattern for making signal handlers safe.

No release notes needed (all are test/example changes, not public API).

---

## Overall Assessment

The series makes significant safety improvements. The primary issue is **incomplete use of atomics for cross-lcore flag access**. Patches 03, 04, and possibly 08/13 need atomics, not just `volatile`, because the flag is written by a signal handler and read by a different lcore. Patches 11 and 13 need verification of the shared variable's type.