On Wed, 1 Jul 2026 11:32:57 +0530
Pushpendra Kumar <[email protected]> wrote:
> When a primary handles a pdump enable/disable request from a secondary
> (e.g. dpdk-dumpcap), it applies the callbacks locally and forwards the
> request to the other secondaries before replying. The forward used
> rte_mp_request_sync(), blocking up to MP_TIMEOUT_S for every secondary
> to acknowledge.
>
> A secondary that is attached but has not called rte_pdump_init() never
> registers the mp_pdump action and silently drops the message (logging
> only "Cannot find action: mp_pdump"). Commit c3ceb8742295 ("pdump:
> forward callback enable to secondary process"), which added this
> broadcast, appears to assume every secondary calls rte_pdump_init();
> when one does not, the primary cannot tell it from a slow or dead peer
> and blocks for the full timeout. That delay pushes the reply past the
> requester's own timeout, so dpdk-dumpcap reports "Packet dump enable
> ... failed Connection timed out".
>
> Forward the request asynchronously with rte_mp_request_async() so an
> unresponsive secondary can no longer block the reply; a callback logs a
> warning if not all secondaries acknowledge. The forward is still issued
> before the requester reply, preserving the ordering from commit
> 928f43e3f9c1 ("pdump: fix race in disabling").
>
> This slightly weakens the happens-before guarantee: the sync call was a
> full-acknowledgement barrier, whereas the async path relies on AF_UNIX
> SOCK_DGRAM FIFO ordering and the single mp_handle thread processing the
> forward before the reply that triggers teardown. This suffices for the
> disable race fixed by 928f43e3f9c1, but reviewers should confirm they
> are comfortable with the weaker guarantee.
>
> Fixes: c3ceb8742295 ("pdump: forward callback enable to secondary process")
> Cc: [email protected]
>
> Signed-off-by: Pushpendra Kumar <[email protected]>
> ---
Yes this looks like a real problem but not sure if this the right fix.
More detailed AI review says:
Review — pdump: fix request timeout on unresponsive secondary
Warning: async forward drops the acknowledgement barrier that
928f43e3f9c1 depends on.
The fix in 928f43e3f9c1 was ordering: forward to secondaries
before replying to the requester. But the safety came from
rte_mp_request_sync() being a full-acknowledgement barrier, not
from send order alone. The primary blocked until every secondary
(dumpcap included, since the broadcast walks the whole mp socket
dir) had actually processed the forward and replied — only then
was the disable response sent back to dumpcap.
rte_mp_request_async() only guarantees the forward is sent
before the reply, not processed. And the two messages reach
dumpcap on different paths handled by different threads:
the disable response wakes the thread blocked in
pdump_prepare_client_request()'s rte_mp_request_sync();
the broadcast is handled by dumpcap's separate mp_handle
thread via pdump_handle_primary_request().
Nothing forces mp_handle to run the broadcast before the blocked
thread wakes and tears down. So on disable, dumpcap can free its
ring / drop the mp_pdump action before it processes the forwarded
request — which is exactly the "Cannot find action: mp_pdump" /
missing-response window 928f43e3f9c1 closed. The commit message's
appeal to "AF_UNIX SOCK_DGRAM FIFO ordering" doesn't hold here:
the reply and the forward travel on distinct sockets to distinct
consumers, so there is no FIFO relationship between them.
For enable this is harmless (callbacks are being added, ring
already exists). It's disable where losing the barrier reopens
the race.
Info: the underlying problem is enable vs disable asymmetry.
The timeout bug you're fixing bites hardest on disable (dumpcap
^C), which is also the one path that still needs the barrier — so
"async for enable, keep sync for disable" doesn't fully solve it.
Worth deciding whether the right fix is at the mp layer instead:
the root cause per c3ceb8742295 is a secondary that attached but
never called rte_pdump_init(), so the primary can't distinguish
"didn't register" from "slow/dead." Broadcasting only to
processes that registered the action, or a disable-side handshake
that doesn't block on non-participants, would fix the timeout
without weakening teardown safety.
Info: the "reviewers should confirm they are comfortable with the
weaker guarantee" paragraph reads like an RFC note; it shouldn't
survive into the permanent commit log if this is applied as-is.
