|WARNING| [v2] examples/vhost: fix hang on exit
dpdklab at iol.unh.edu
dpdklab at iol.unh.edu
Thu Oct 29 19:30:04 CET 2026
Test-Label: ai-code-review
Test-Status: WARNING
https://dpdk.org/patch/172244
_AI Code Review_
=== Patch Review: series.patch (via Claude) ===
Review date: 2026-10-29
# DPDK Patch Review
## Errors
None identified.
## Warnings
**1. Missing `rte_mb()` after atomic store may cause delayed worker notification**
Line: `rte_atomic_store_explicit(&stop_workers, true, rte_memory_order_relaxed);`
The atomic store uses `relaxed` ordering, which provides no ordering guarantees.
After unregistering the vhost drivers, the main thread sets `stop_workers = true`
and immediately calls `rte_eal_mp_wait_lcore()`, expecting workers to exit.
However, with `relaxed` ordering, the store may not be visible to other cores immediately,
causing workers to continue spinning on the old value.
The fix is to use `release` ordering on the store:
```c
rte_atomic_store_explicit(&stop_workers, true, rte_memory_order_release);
```
And correspondingly, the load in the worker loop should use `acquire` ordering:
```c
while (!rte_atomic_load_explicit(&stop_workers, rte_memory_order_acquire)) {
```
This ensures the store is visible to workers before `mp_wait_lcore()` begins waiting.
**Context**: The worker loop currently uses `relaxed`:
```c
while (!rte_atomic_load_explicit(&stop_workers, rte_memory_order_relaxed)) {
```
While this may work on x86 due to total-store-order, it is not portable
and could cause indefinite hangs on architectures with weaker memory models (ARM, POWER).
**2. Potential worker exit before unregister**
Lines: unregister_drivers() call followed by atomic store
The patch fixes the original deadlock where `destroy_device()` waited for workers
that had already exited. However, the new sequence introduces a different race:
1. Main thread calls `sigwait(&set, &sig)` and unblocks
2. Main thread unregisters drivers - `destroy_device()` may be called
3. Main thread sets `stop_workers = true`
4. If a device destroy happens between steps 2-3, and that destroy callback
spins waiting for a worker ack, the worker hasn't been told to stop yet
The safer ordering is to set `stop_workers` **before** unregistering drivers:
```c
/* Tell worker threads to prepare for shutdown */
rte_atomic_store_explicit(&stop_workers, true, rte_memory_order_release);
/* Give workers a chance to observe the flag */
rte_delay_ms(100);
/* Unregister vhost driver - destroy callbacks may now safely complete */
unregister_drivers(nb_sockets);
/* Wait for workers to fully exit */
rte_eal_mp_wait_lcore();
```
This ensures workers are already in their exit path when `destroy_device()` runs,
avoiding the hang. The delay is optional but increases robustness on loaded systems.
## Info
**Signal handling improvement acknowledged**
The conversion from `sig_atomic_t quit` with a signal handler
to `sigwait()` with blocked signals is the correct modern pattern.
`sigwait()` avoids async-signal-safety issues and allows the main thread
to cleanly orchestrate shutdown.
Review-Result: WARNING
More information about the test-report
mailing list