|WARNING| [v2] 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/172195

_AI Code Review_

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

# Review of DPDK Patch: Revert "examples/symmetric_mp: do not print or exit in handler"

## ERRORS

### 1. Signal handler calls exit() directly (forbidden pattern)

**Line:** `exit(0);` in `print_stats()` function

**Issue:** The patch modifies `print_stats()` to be used as a signal handler and calls `exit(0)` directly from it. This is unsafe because `exit()` performs cleanup operations (atexit handlers, stdio flushing, etc.) that are not async-signal-safe. In a multi-process or multi-threaded application, this can deadlock or corrupt state if the signal arrives during malloc, stdio operations, or other non-reentrant code.

**Guideline reference:** "Code that is not safe against signals" under Architecture & Patterns; signal handlers must only call async-signal-safe functions per POSIX.

**Fix:** Use a flag-based approach where the signal handler sets a volatile sig_atomic_t flag, and the main loop checks it:

```c
static volatile sig_atomic_t quit;

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

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

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

This pattern was in the original code before the revert and is the correct approach.

---

### 2. Resource leak: rte_eal_mp_wait_lcore() call removed

**Line:** Removal of `rte_eal_mp_wait_lcore();` before cleanup

**Issue:** The patch removes the `rte_eal_mp_wait_lcore()` call that waits for worker lcores to finish before calling `rte_eal_cleanup()`. Without this, the primary process proceeds directly to cleanup while worker threads are still running their forwarding loops. This is a race condition: `rte_eal_cleanup()` tears down EAL resources (rings, mempools, memory mappings) that the workers are actively using, causing undefined behavior.

**Fix:** Restore the `rte_eal_mp_wait_lcore()` call:

```c
rte_eal_mp_remote_launch(lcore_main, NULL, CALL_MAIN);
rte_eal_mp_wait_lcore();

print_stats();

/* clean up the EAL */
rte_eal_cleanup();
```

---

### 3. lcore_main() unreachable return removed

**Line:** Removal of `return 0;` from `lcore_main()`

**Issue:** The function `lcore_main()` is declared to return `int` (required by `rte_eal_mp_remote_launch()`), but the patch changes the loop to `for (;;)` with no exit condition and removes the `return 0`. This makes the function never return, triggering compiler warnings ("control reaches end of non-void function" or "noreturn function does return") and violating the function signature contract.

**Fix:** Keep the `while (!quit)` loop and `return 0` at the end, as the original code had.

---

### 4. Unused parameter not marked __rte_unused

**Line:** `print_stats(int signum)` function parameter

**Issue:** The `signum` parameter is required by the signal handler signature `void (*)(int)` but is never used in the function body. The v2 commit message says "need to mark arg unused" but the code does not mark it. This will produce a compiler warning.

**Fix:**

```c
static void
print_stats(int signum __rte_unused)
{
	/* ... */
}
```

---

## WARNINGS

### 1. Signal handler performs I/O operations (printf)

**Line:** `printf()` calls within `print_stats()` when used as a signal handler

**Issue:** `printf()` is not async-signal-safe. Calling it from a signal handler can deadlock if the signal interrupts another `printf()` or stdio operation, as stdio uses internal locks. While this is noted as "not safe" in the comment, it remains a correctness issue.

**Severity:** Warning (the comment acknowledges this is example code, not production-ready, and the alternative of proper multi-process shutdown is too complex for an example).

**Guideline reference:** "Code that is not safe against signals" under Architecture & Patterns.

---

### 2. Comment describes the problem but does not justify the solution

**Lines:** Comment block above `print_stats()`

**Issue:** The comment correctly states that "This is not safe way to exit for a real production application" and that "proper shutdown with primary/secondary requires process monitoring and rendezvous." However, the code then proceeds to implement an unsafe solution (signal handler calling `exit()` and `printf()`) rather than the safe flag-based approach that was in the original code and does not require complex inter-process monitoring.

**Recommendation:** The original flag-based approach is both safer AND simpler than the reverted version. The justification for reverting should be reconsidered -- the original code's approach (set flag in signal handler, check in loop, wait for workers, then cleanup) is the textbook pattern for handling signals in multi-threaded programs and does not require the "process monitoring and rendezvous" complexity mentioned in the comment.

---

## SUMMARY

The patch introduces multiple correctness bugs by calling unsafe functions (`exit()`, `printf()`) from a signal handler and by removing synchronization (`rte_eal_mp_wait_lcore()`) that prevents races between cleanup and active worker threads. The original code that this patch reverts was architecturally sound; this reversion introduces problems rather than solving them.

The commit message claims the original change caused crashes, but the actual root cause is not addressed here -- the solution should involve proper process coordination, not reverting to unsafe signal handling patterns.

Review-Result: ERROR