Re: [PATCH] eal: add worker threads cleanup in rte_eal_cleanup()

[Stephen Hemminger <stephen@networkplumber.org>]

On Fri, 10 Jan 2025 12:17:17 +0530
Gagandeep Singh <g.singh@nxp.com> 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 <g.singh@nxp.com>
> ---

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.