|WARNING| [v3] Revert "examples/symmetric_mp: do not print or exit in handler"

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-10-27

# DPDK Patch Review

## ERRORS

### Use of exit() in Signal Handler

**File:** examples/multi_process/symmetric_mp/main.c  
**Line:** `exit(0);` in `print_stats()` function

The patch calls `exit(0)` directly from a signal handler (`print_stats()`), which is **not async-signal-safe** according to POSIX.

>From the POSIX specification, `exit()` is **not** in the list of async-signal-safe functions. Calling it from a signal handler invokes undefined behavior. The function performs cleanup operations (flushing buffers, calling atexit handlers, etc.) that are not reentrant and may deadlock or corrupt state if interrupted at the wrong point.

**Suggested fix:**

Use a `volatile sig_atomic_t` flag to request shutdown, then exit from the main thread:

```c
static volatile sig_atomic_t quit_flag = 0;

static void
signal_handler(int signum __rte_unused)
{
	quit_flag = 1;
}

/* In lcore_main(): */
while (!quit_flag) {
	/* ... forwarding loop ... */
}

/* In main(), after rte_eal_mp_wait_lcore(): */
print_stats();
```

This is the standard async-signal-safe pattern. The comment in the patch acknowledges the shutdown is not production-safe but doesn't excuse the undefined behavior--even a demonstration should avoid patterns that corrupt process state.

---

## WARNINGS

### Missing rte_eal_mp_wait_lcore() Call

**File:** examples/multi_process/symmetric_mp/main.c  
**Line:** Removal of `rte_eal_mp_wait_lcore();` after `rte_eal_mp_remote_launch()`

The patch removes the `rte_eal_mp_wait_lcore()` call. Without it, the main thread proceeds immediately to `rte_eal_cleanup()` while worker lcores are still running in `lcore_main()`.

This creates a race: if the signal arrives before workers start or during early initialization, the main thread will clean up EAL resources while workers attempt to use them (shared rings, mempools, port access), causing crashes or corruption.

Even if the signal typically arrives later, the window exists. The workers loop forever (`for (;;)`), so they will never terminate and `wait_lcore()` would never return anyway--but the proper pattern is to signal the workers to stop, wait for them, then clean up.

**Suggested fix:**

Keep `rte_eal_mp_wait_lcore()`, but change the worker loop to respect `quit_flag` and return when signaled. The signal handler sets the flag; the main thread waits for workers to see it and exit, then proceeds to cleanup.

---

### Missing rte_eal_cleanup() Call Path

**File:** examples/multi_process/symmetric_mp/main.c  
**Line:** The `exit(0)` in `print_stats()` bypasses the `rte_eal_cleanup()` at the end of `main()`

The patch removes the normal return path from `lcore_main()` (it now loops forever) and exits the entire process from the signal handler. This skips the `rte_eal_cleanup()` call at the end of `main()`.

While this is an example and not production code, omitting EAL cleanup leaks resources (hugepage mappings, device state) that make it harder to restart the application or observe clean shutdown in testing.

**Suggested fix:**

Return from the signal handler to the main thread so that `main()` can call `rte_eal_cleanup()` before process exit. Use the flag-based pattern from the first error.

---

## INFO

### Infinite Loop Without Return Statement

**File:** examples/multi_process/symmetric_mp/main.c  
**Line:** `lcore_main()` changed from `while (!quit)` to `for (;;)` with no `return` statement

The function signature is `static void lcore_main(void *arg __rte_unused)`, so the missing `return` is not a compilation error. However, the infinite loop with no exit condition makes the code impossible to test shutdown behavior without external termination (signals, kill).

This is less flexible than the previous pattern where the loop could be interrupted by setting a flag. If the example were extended to support graceful shutdown (e.g., for testing), the infinite loop would need to be changed back.

This is a design choice rather than a bug, so it is informational only. The patch comment acknowledges this is not production-quality shutdown logic.

---

### Commit Message References Incorrect Original Commit

**File:** Commit message  
**Line:** `This reverts commit 9a0b3c409db1963a92d0a91868b1e45254845a8c.`

The commit SHA `9a0b3c409db1963a92d0a91868b1e45254845a8c` is cited as the commit being reverted. While I cannot verify this hash without access to the DPDK git tree, the patch version history (`v3 - need to ignore signum, send correct commit`) indicates the author corrected the commit reference in this version.

This is informational only: if the SHA is correct, no issue; if it's still wrong, maintainers will catch it.

---

## FINAL CHECK

**Pass 1 (Correctness bugs included):** Yes--exit() in signal handler is a clear async-signal-safety violation.

**Pass 2 (Style/process false positives removed):** No style items concluded "acceptable" were included.

**Pass 3 (Evidence verification):**
- All findings reference specific lines added or removed by the patch.
- Quotes provided where applicable.
- No speculation about code outside the patch or unread code.

Review-Result: ERROR