|WARNING| [v2, 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/169162

_AI Code Review_

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

# Review of DPDK Patch Series: Signal Handler Safety

## Overview

This patch series addresses async-signal-safety violations across multiple DPDK examples and applications. The changes remove unsafe operations (printf, library calls, exit) from signal handlers and replace them with proper patterns using volatile flags.

---

## Patch-by-Patch Review

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

**Assessment: Clean**

Removes printf from signal handler. The `force_quit` flag should be `volatile sig_atomic_t` for correctness, but this appears to be existing code outside the patch scope.

---

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

**Assessment: Clean**

Multiple examples simplified to just set flags. Consistent pattern across all files.

---

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

**Assessment: Good**

Properly moves statistics printing out of signal context. The `stats_requested` variable is correctly declared `volatile sig_atomic_t`. The main loop polls it on the main lcore and clears it after printing.

---

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

**Findings:**

#### Error: Missing volatile qualifier on shared flag

The `quit` variable is written from signal context and read by worker threads, but is declared as `static volatile sig_atomic_t` only. While `sig_atomic_t` ensures atomic access for signal handlers, the `volatile` qualifier here does not provide the necessary semantics for multi-threaded visibility.

When worker threads on other cores check `while (!quit)`, the compiler/CPU can cache the value of `quit` and never observe the signal handler's write.

**Fix:** Use C11 atomic with appropriate memory ordering:

```c
/* Set by SIGTERM and SIGINT handler to stop the forwarding loops. */
static _Atomic bool quit;

static void
signal_handler(__rte_unused int signum)
{
	rte_atomic_store_explicit(&quit, true, rte_memory_order_relaxed);
}

static int
lcore_main(void *arg __rte_unused)
{
	/* ... */
	while (!rte_atomic_load_explicit(&quit, rte_memory_order_relaxed)) {
		/* ... */
	}
	return 0;
}
```

Alternatively, if targeting older C standards or avoiding atomics for simplicity, document that `volatile sig_atomic_t` is acceptable here because the flag is only ever written once (set to 1) and the race is benign (workers may process a few extra packets before observing the quit). However, the atomic approach is preferred for correctness.

---

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

**Assessment: Clean**

Proper pattern: flag set in handler, teardown moved to main(). The `quit` variable is `volatile sig_atomic_t`.

---

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

**Assessment: Clean**

Worker loop checks `volatile sig_atomic_t quit`, unregister moved to main after join. Correct.

---

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

**Assessment: Clean**

Sleep loop breaks on `quit`, cleanup in main(). Correct pattern.

---

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

**Assessment: Clean**

Replaces the problematic `signal(signum, SIG_DFL); kill(getpid(), signum)` pattern with setting per-lcore `stopped` flags. The `stopped` field is correctly marked `volatile uint8_t` since it's written from signal context and read by worker loops.

---

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

**Assessment: Clean**

Simple printf removal. The `force_quit` flag is not shown in the diff, but the pattern is consistent with other patches.

---

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

**Assessment: Clean**

Printf removal. Correct.

---

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

**Findings:**

#### Warning: volatile used for inter-thread synchronization

The `exit_now` flag is changed to `volatile int`. This is written by main() and read by worker_main() on another lcore. While the comment correctly identifies the visibility issue, `volatile` is not the right solution for multi-threaded synchronization in C11.

**Recommendation:** Use atomic operations:

```c
struct app_config {
	struct app_port ports[MAX_PORTS];
	int cnt_ports;
	_Atomic int exit_now;
};

/* In main(): */
rte_atomic_store_explicit(&app_cfg.exit_now, 1, rte_memory_order_relaxed);

/* In worker_main(): */
while (!rte_atomic_load_explicit(&app_cfg.exit_now, rte_memory_order_relaxed)) {
	/* ... */
}
```

However, given that this is a simple flag set once and polled, and the race is benign (at worst the worker processes a few extra packets), the `volatile` approach is acceptable here. The patch improves the existing code.

#### Info: cmdline_stdin_new() check is good practice

The added NULL check on `cmdline_stdin_new()` is correct. The use of `rte_panic()` is appropriate for initialization failures in an example application.

---

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

**Assessment: Clean**

Adds signal handlers for SIGINT/SIGTERM that set `volatile sig_atomic_t quit`, modifies the infinite loop to check the flag, and waits for workers before cleanup. Correct pattern throughout.

---

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

**Findings:**

#### Error: Use of sigaction without checking return value

The patch uses `sigaction()` three times but does not check the return values. While `sigaction()` failures are rare in practice (typically only on invalid signal numbers), robustness requires checking.

**Fix:**

```c
if (sigaction(SIGINT, &sa, NULL) < 0)
	rte_exit(EXIT_FAILURE, "Failed to set SIGINT handler\n");
if (sigaction(SIGTERM, &sa, NULL) < 0)
	rte_exit(EXIT_FAILURE, "Failed to set SIGTERM handler\n");
if (sigaction(SIGUSR1, &sa, NULL) < 0)
	rte_exit(EXIT_FAILURE, "Failed to set SIGUSR1 handler\n");
```

#### Info: Signal handler design is sound

The split between quit (SIGINT/SIGTERM) and dump (SIGUSR1) handlers is good design. The `SA_RESETHAND` flag on the quit signals ensures a second SIGINT/SIGTERM gets default behavior (process termination), which is standard practice. The dump signal correctly sets `volatile sig_atomic_t dump_dev_signal` and the main loop clears it after dumping.

#### Info: Comment update needed

The commit message says "SIGTSTP (^Z) to use SIGUSR1" but should note that SIGUSR1 has no keyboard shortcut--users must send it via `kill -USR1 <pid>`. Consider adding a note in the example's documentation or startup message.

---

## Summary

### Errors (Must Fix)

1. **Patch 4 (symmetric_mp):** `quit` flag needs proper multi-threaded synchronization (atomic or documented rationale for `volatile sig_atomic_t`)
2. **Patch 13 (eventdev_pipeline):** `sigaction()` calls lack error checking

### Warnings (Should Fix)

1. **Patch 11 (ethtool):** `volatile int exit_now` for multi-threaded flag should use atomics (or document why `volatile` is acceptable here)

### Info (Consider)

1. **Patch 13:** Document that SIGUSR1 has no keyboard shortcut, unlike the old SIGTSTP (^Z)

### Positive Observations

- Consistent pattern across all patches: set flag in handler, act on it in normal context
- Proper use of `volatile sig_atomic_t` for signal-set flags in single-threaded or signal-only scenarios
- Good cleanup discipline: wait for workers, then teardown resources
- `SA_RESETHAND` usage in patch 13 is excellent practice

### General Recommendation

This series makes substantial improvements to signal safety. The two errors above should be addressed before merging. The overall approach is sound and follows best practices for async-signal-safety.