On Fri, 10 Jan 2025 12:17:17 +0530 Gagandeep Singh <[email protected]> wrote:
> This patch introduces a worker thread cleanup function in the EAL library, > ensuring proper termination of created pthreads and invocation of > registered pthread destructors. > This guarantees the correct cleanup of thread-specific resources, > used by drivers or applications. > > Signed-off-by: Gagandeep Singh <[email protected]> > --- This seems to have not gotten review it needs. The AI review process found several issues. Review: [PATCH] eal: add worker threads cleanup in rte_eal_cleanup() Patch 1/1 - eal: add worker threads cleanup in rte_eal_cleanup() Error: pthread_join called unconditionally after pthread_cancel failure. If pthread_cancel() fails (returns non-zero), the thread was not cancelled. Calling pthread_join() on a still-running worker thread that is blocked in read() on its pipe will block the cleanup indefinitely -- the worker is waiting for a command that will never come, and join will wait for the worker that will never exit. The join should be skipped when cancel fails, or the cancel failure should be treated as fatal for that lcore. Suggested fix: ret = pthread_cancel((pthread_t)lcore_config[lcore_id].thread_id.opaque_id); if (ret != 0) { EAL_LOG(WARNING, "Pthread cancel fails for lcore %d", lcore_id); continue; /* skip join -- thread is still running */ } ret = pthread_join(...); Error: Cleanup ordering -- worker threads cancelled after eal_bus_cleanup(). The patch inserts eal_worker_thread_cleanup() after eal_bus_cleanup(). Bus cleanup may trigger device close/release callbacks. If a worker lcore is currently executing a function dispatched via rte_eal_remote_launch() that touches bus/device resources, cancelling the thread after those resources are torn down risks use-after-free. Worker threads should be terminated first, before any subsystem teardown, to ensure no worker is mid-execution when resources are freed. Move eal_worker_thread_cleanup() to the beginning of rte_eal_cleanup(), after the run_once guard but before rte_service_finalize() / eal_bus_cleanup(). Warning: Uses raw pthread_cancel()/pthread_join() instead of DPDK thread API. AGENTS.md forbidden tokens list requires rte_thread_join() instead of pthread_join(). The existing mp_channel_cleanup code uses the same pattern (pthread_cancel + rte_thread_join), so at minimum the join should use rte_thread_join() for consistency: rte_thread_join(lcore_config[lcore_id].thread_id, NULL); There is no rte_thread_cancel() wrapper, so pthread_cancel() is acceptable here (same as rte_mp_channel_cleanup does). Warning: No pipe fd cleanup after thread cancellation. Each worker has pipe_main2worker and pipe_worker2main fds created during rte_eal_init(). After cancelling and joining the worker threads, these pipe fds are never closed. This leaks 2 pipe fds (4 file descriptors) per worker lcore. The cleanup function should close these fds after the join succeeds. Warning: Comparing opaque_id against zero to detect uninitialized threads. The check `if (!lcore_config[lcore_id].thread_id.opaque_id)` assumes that zero means "no thread was created." On Linux, pthread_t is an unsigned long and a valid thread ID could theoretically be 0 (though glibc never produces this). A more robust approach is to track which lcores had threads successfully created, or check the lcore state. The existing mp_channel code uses a similar opaque_id != 0 guard, so this is minor -- mentioning for completeness. Info: The commit message says "pthreads" and "pthread destructors" but does not explain *which* thread-specific resources motivate this change. A concrete example (e.g., a specific driver TLS destructor that leaks without this) would strengthen the justification.
