|SUCCESS| [2/2] eal: notify secondary on primary exit

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/163795

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-05-07

Reviewing the patch series against DPDK coding standards and guidelines:

commit f96273c8e9d39e472bb07acc05e493b1e712e51b (reverted)
commit 85d6815fa6d0 (fixed)

Author: Stephen Hemminger <stephen at networkplumber.org>

PATCH 1/2: Revert "app/testpmd: stop forwarding in secondary process"

This patch reverts a testpmd-specific multi-process notification
mechanism, moving the responsibility to EAL.

PATCH 2/2: eal: notify secondary on primary exit

This patch adds primary-to-secondary notification in EAL when the primary
process exits.

> diff --git a/lib/eal/common/eal_common_proc.c b/lib/eal/common/eal_common_proc.c
> index 06f151818c..4369892e44 100644
> --- a/lib/eal/common/eal_common_proc.c
> +++ b/lib/eal/common/eal_common_proc.c

[ ... ]

> @@ -614,6 +623,25 @@ close_socket_fd(int fd)
>  		unlink(path);
>  }
>  
> +/* callback in secondary when primary has exited */
> +static int
> +mp_primary_exited(const struct rte_mp_msg *msg, const void *peer __rte_unused)
> +{
> +	const struct eal_mp_req *req;
> +
> +	if (msg->len_param != sizeof(*req)) {
> +		EAL_LOG(ERR, "invalid request from primary");
> +		return -EINVAL;
> +	}
> +
> +	req = (const struct eal_mp_req *)msg->param;
> +	if (req->type == MP_REQ_QUIT)
> +		rte_mp_channel_cleanup();

Does this code create a recursion issue? In rte_mp_channel_cleanup(), when
called from the secondary process, it calls rte_mp_action_unregister(EAL_MP)
which might still be needed if this handler is being invoked. Can the
unregistration happen while mp_primary_exited() is executing?

Additionally, does rte_mp_channel_cleanup() calling itself (indirectly
through the handler) cause any state issues with the mp_fd atomic
operations or the thread cleanup?

> +
> +	/* no reply needed */
> +	return 0;
> +}

[ ... ]

> @@ -682,12 +714,27 @@ rte_mp_channel_cleanup(void)
>  {
>  	int fd;
>  
> +	/* Primary exiting, tell all secondary processes */
> +	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
> +		struct rte_mp_msg msg = { .name = EAL_MP };
> +		struct eal_mp_req req = { .type = MP_REQ_QUIT };
> +
> +		memcpy(&msg.param, &req, sizeof(req));
> +		msg.len_param = sizeof(req);
> +
> +		rte_mp_sendmsg(&msg);

Does this code check the return value of rte_mp_sendmsg()? If the message
fails to send, should there be error handling or logging?

Also, does rte_mp_sendmsg() guarantee message delivery before proceeding
with cleanup? Can the socket be closed via close_socket_fd(fd) below
before secondaries receive the MP_REQ_QUIT message?

> +	} else {
> +		rte_mp_action_unregister(EAL_MP);
> +	}
> +
>  	fd = rte_atomic_exchange_explicit(&mp_fd, -1, rte_memory_order_relaxed);
>  	if (fd < 0)
>  		return;
>  
> -	pthread_cancel((pthread_t)mp_handle_tid.opaque_id);
> -	rte_thread_join(mp_handle_tid, NULL);
> +	if (pthread_self() != (pthread_t)mp_handle_tid.opaque_id) {
> +		pthread_cancel((pthread_t)mp_handle_tid.opaque_id);
> +		rte_thread_join(mp_handle_tid, NULL);
> +	}

Does this code handle the case where mp_handle_tid.opaque_id is
uninitialized? In rte_mp_channel_init(), if thread creation fails, can
rte_mp_channel_cleanup() be called with an invalid mp_handle_tid?

Can the pthread_self() comparison be unreliable if the thread ID hasn't
been properly initialized? Should there be a check that mp_handle_tid is
valid before comparing?

>  	close_socket_fd(fd);
>  }