* [PATCH] pdump: fix request timeout on unresponsive secondary
@ 2026-07-01 6:02 Pushpendra Kumar
2026-07-02 4:01 ` Pushpendra Kumar
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Pushpendra Kumar @ 2026-07-01 6:02 UTC (permalink / raw)
To: dev; +Cc: pushpendra.kumar, reshma.pattan, stephen, stable,
Pushpendra Kumar
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: stable@dpdk.org
Signed-off-by: Pushpendra Kumar <pushpendra1x.kumar@intel.com>
---
.mailmap | 1 +
lib/pdump/rte_pdump.c | 23 ++++++++++++++---------
2 files changed, 15 insertions(+), 9 deletions(-)
diff --git a/.mailmap b/.mailmap
index c5bc728fae..c1313c7148 100644
--- a/.mailmap
+++ b/.mailmap
@@ -1332,6 +1332,7 @@ Przemyslaw Gierszynski <przemyslaw.gierszynski@intel.com>
Przemyslaw Patynowski <przemyslawx.patynowski@intel.com>
Przemyslaw Zegan <przemyslawx.zegan@intel.com>
Pu Xu <583493798@qq.com>
+Pushpendra Kumar <pushpendra1x.kumar@intel.com>
Qi Fu <qi.fu@intel.com>
Qi Zhang <qi.z.zhang@intel.com>
Qian Hao <qi_an_hao@126.com>
diff --git a/lib/pdump/rte_pdump.c b/lib/pdump/rte_pdump.c
index ac94efe7ff..f2d5b70111 100644
--- a/lib/pdump/rte_pdump.c
+++ b/lib/pdump/rte_pdump.c
@@ -477,11 +477,21 @@ set_pdump_rxtx_cbs(const struct pdump_request *p)
return ret;
}
+/* Async reply handler; warns if any secondary did not respond. */
+static int
+pdump_secondary_reply(const struct rte_mp_msg *request __rte_unused,
+ const struct rte_mp_reply *reply)
+{
+ if (reply->nb_sent != reply->nb_received)
+ PDUMP_LOG_LINE(ERR, "not all secondary's replied (sent %u recv %u)",
+ reply->nb_sent, reply->nb_received);
+ return 0;
+}
+
static void
pdump_request_to_secondary(const struct pdump_request *req)
{
struct rte_mp_msg mp_req = { };
- struct rte_mp_reply mp_reply;
struct timespec ts = {.tv_sec = MP_TIMEOUT_S, .tv_nsec = 0};
PDUMP_LOG_LINE(DEBUG, "forward req %s to secondary", pdump_opname(req->op));
@@ -490,14 +500,9 @@ pdump_request_to_secondary(const struct pdump_request *req)
strlcpy(mp_req.name, PDUMP_MP, sizeof(mp_req.name));
mp_req.len_param = sizeof(*req);
- if (rte_mp_request_sync(&mp_req, &mp_reply, &ts) != 0)
- PDUMP_LOG_LINE(ERR, "rte_mp_request_sync failed");
-
- else if (mp_reply.nb_sent != mp_reply.nb_received)
- PDUMP_LOG_LINE(ERR, "not all secondary's replied (sent %u recv %u)",
- mp_reply.nb_sent, mp_reply.nb_received);
-
- free(mp_reply.msgs);
+ /* Forward asynchronously so an unresponsive secondary cannot block the requester reply. */
+ if (rte_mp_request_async(&mp_req, &ts, pdump_secondary_reply) != 0)
+ PDUMP_LOG_LINE(ERR, "rte_mp_request_async failed");
}
/* Allocate temporary storage for passing state to the alarm thread for deferred handling */
--
2.43.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] pdump: fix request timeout on unresponsive secondary
2026-07-01 6:02 [PATCH] pdump: fix request timeout on unresponsive secondary Pushpendra Kumar
@ 2026-07-02 4:01 ` Pushpendra Kumar
2026-07-03 15:52 ` Stephen Hemminger
2026-07-05 8:44 ` [PATCH v2] pdump: fix teardown race with opt-in MP request mode Pushpendra Kumar
2 siblings, 0 replies; 8+ messages in thread
From: Pushpendra Kumar @ 2026-07-02 4:01 UTC (permalink / raw)
To: dev; +Cc: pushpendra.kumar, reshma.pattan, stephen, stable
Hi all,
Please hold review of this patch for now.
While validating this approach, I found a race condition in ENABLE/DISABLE forwarding behavior. In the DISABLE path, a delayed forwarded secondary (S2) can crash if requester-side teardown in S1 (the secondary that initiates DISABLE) proceeds first and shared resources are released.
I reproduced this with dpdk-dumpcap by injecting delay on S2. In my setup, it appears around the 5-second timeout mark (similar to MP_TIMEOUT_S), but the issue is about ordering and lifecycle guarantees, not a specific delay value or application.
The same teardown-safety risk can also exist in the original behavior if a secondary handles DISABLE late enough that the control plane fails or times out, and requester-side teardown still proceeds.
The root issue is:
- If S2 is slow/unresponsive on DISABLE, requester-side DISABLE can fail/timeout.
- If S1 app ignores that failure and frees shared capture resources anyway, S2 may still touch stale pointers and crash.
- So the root issue is teardown safety after failed/partial DISABLE completion, not only async forwarding itself.
I am pausing this patch to investigate a cleaner lifecycle fix for the DISABLE path. I will send a v2 after a more robust solution is verified.
Suggestions and feedback are very welcome.
Best regards,
Pushpendra
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] pdump: fix request timeout on unresponsive secondary
2026-07-01 6:02 [PATCH] pdump: fix request timeout on unresponsive secondary Pushpendra Kumar
2026-07-02 4:01 ` Pushpendra Kumar
@ 2026-07-03 15:52 ` Stephen Hemminger
2026-07-05 8:44 ` [PATCH v2] pdump: fix teardown race with opt-in MP request mode Pushpendra Kumar
2 siblings, 0 replies; 8+ messages in thread
From: Stephen Hemminger @ 2026-07-03 15:52 UTC (permalink / raw)
To: Pushpendra Kumar; +Cc: dev, pushpendra.kumar, reshma.pattan, stable
On Wed, 1 Jul 2026 11:32:57 +0530
Pushpendra Kumar <pushpendra1x.kumar@intel.com> 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: stable@dpdk.org
>
> Signed-off-by: Pushpendra Kumar <pushpendra1x.kumar@intel.com>
> ---
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2] pdump: fix teardown race with opt-in MP request mode
2026-07-01 6:02 [PATCH] pdump: fix request timeout on unresponsive secondary Pushpendra Kumar
2026-07-02 4:01 ` Pushpendra Kumar
2026-07-03 15:52 ` Stephen Hemminger
@ 2026-07-05 8:44 ` Pushpendra Kumar
2026-07-05 17:07 ` [PATCH v3] " Pushpendra Kumar
2026-07-05 18:27 ` [PATCH v2] " Stephen Hemminger
2 siblings, 2 replies; 8+ messages in thread
From: Pushpendra Kumar @ 2026-07-05 8:44 UTC (permalink / raw)
To: dev
Cc: stable, thomas, reshma.pattan, stephen, anatoly.burakov,
dmitry.kozliuk, bruce.richardson, 14pwcse1224, pushpendra.kumar,
Pushpendra Kumar
This change addresses two pdump teardown failure cases:
1. A requester secondary can time out if a forwarded secondary is slow,
unresponsive, or does not implement rte_pdump_init().
2. After a DISABLE request times out, the requester secondary receives
the reply and releases shared resources, which other slow or
unresponsive secondaries may access, leading to a crash.
Add an opt-in EAL multiprocess request mode that allows a caller to
ignore peers that do not implement the requested action, while preserving
the existing rte_mp_request_sync() behavior for legacy users.
pdump uses this new mode when forwarding enable/disable requests and adds
the following behavior:
- track request generation so stale callbacks cannot act on a newer
enable/disable cycle
- wait for in-flight callbacks to drain before completing disable
- prevent shared resource teardown from racing with delayed secondary
processing
- keep pdump intent-driven without extra consumer-specific workaround
logic
The EAL change is required because the current mp request path treats
a missing action as a failure, which cannot satisfy pdump's needs without
changing behavior for all existing users. The new API is opt-in, so
existing callers of rte_mp_request_sync() keep their current semantics.
Validation:
- Baseline-vs-changes benchmark used DPDK-native test-pmd as primary and
dumpcap as secondary.
- Both runs captured 1024 packets with the same packet-count exit
condition.
- A 5-second perf stat sample on the forwarding primary showed no
meaningful regression: with changes recorded 12,402,055,320 cycles and
17,391,769,084 instructions, versus 12,450,031,900 cycles and
17,499,706,053 instructions on the parent baseline.
- The extra synchronization in the hot path did not show material
overhead in this sample.
Fixes: c3ceb8742295 ("pdump: forward callback enable to secondary process")
Signed-off-by: Pushpendra Kumar <pushpendra1x.kumar@intel.com>
Cc: stable@dpdk.org
---
This V2 patch addresses two pdump teardown failure scenarios:
- A requester secondary can time out if a forwarded secondary is slow,
unresponsive, or does not implement rte_pdump_init().
- A delayed DISABLE reply can race with requester-side teardown, which may
lead to crashes if peer processes still access shared pdump resources.
To fix this end-to-end:
- EAL adds an opt-in MP request mode to ignore peers with no matching action,
while preserving legacy rte_mp_request_sync() behavior.
- pdump uses that mode, tracks generation, and waits for in-flight callbacks
to drain before completing disable.
- dumpcap and pdump app cleanup paths are fail-closed when disable does not
complete successfully.
If this direction is accepted, I can split from the next revision into a
focused EAL patch followed by pdump and app-user updates.
.mailmap | 1 +
app/dumpcap/main.c | 23 ++-
app/pdump/main.c | 32 +++-
doc/guides/rel_notes/release_26_07.rst | 7 +
lib/eal/common/eal_common_proc.c | 37 +++-
lib/eal/include/rte_eal.h | 33 ++++
lib/eal/windows/eal_mp.c | 9 +
lib/pdump/rte_pdump.c | 241 ++++++++++++++++++++++---
lib/pdump/rte_pdump.h | 40 +++-
9 files changed, 379 insertions(+), 44 deletions(-)
diff --git a/.mailmap b/.mailmap
index c5bc728fae..c1313c7148 100644
--- a/.mailmap
+++ b/.mailmap
@@ -1332,6 +1332,7 @@ Przemyslaw Gierszynski <przemyslaw.gierszynski@intel.com>
Przemyslaw Patynowski <przemyslawx.patynowski@intel.com>
Przemyslaw Zegan <przemyslawx.zegan@intel.com>
Pu Xu <583493798@qq.com>
+Pushpendra Kumar <pushpendra1x.kumar@intel.com>
Qi Fu <qi.fu@intel.com>
Qi Zhang <qi.z.zhang@intel.com>
Qian Hao <qi_an_hao@126.com>
diff --git a/app/dumpcap/main.c b/app/dumpcap/main.c
index 46a6cb251e..f421a17034 100644
--- a/app/dumpcap/main.c
+++ b/app/dumpcap/main.c
@@ -521,14 +521,33 @@ static void
cleanup_pdump_resources(void)
{
struct interface *intf;
+ int ret;
+ bool disable_failed = false;
TAILQ_FOREACH(intf, &interfaces, next) {
- rte_pdump_disable(intf->port,
- RTE_PDUMP_ALL_QUEUES, RTE_PDUMP_FLAG_RXTX);
+ ret = rte_pdump_disable(intf->port,
+ RTE_PDUMP_ALL_QUEUES, RTE_PDUMP_FLAG_RXTX);
+ if (ret < 0) {
+ disable_failed = true;
+ fprintf(stderr,
+ "Disable pdump failed on %u:%s: %s\n",
+ intf->port, intf->name,
+ rte_strerror(rte_errno));
+ }
if (intf->opts.promisc_mode)
rte_eth_promiscuous_disable(intf->port);
}
+ if (disable_failed) {
+ /*
+ * Fail-closed: if disable did not complete, keep pdump action/state
+ * alive and do not uninit shared capture resources.
+ */
+ fprintf(stderr,
+ "Skipping pdump uninit because disable did not complete on all interfaces\n");
+ return;
+ }
+
rte_pdump_uninit();
}
diff --git a/app/pdump/main.c b/app/pdump/main.c
index 1e62c8adc1..4cfb038395 100644
--- a/app/pdump/main.c
+++ b/app/pdump/main.c
@@ -451,14 +451,17 @@ print_pdump_stats(void)
}
}
-static inline void
+static inline int
disable_pdump(struct pdump_tuples *pt)
{
if (pt->dump_by_type == DEVICE_ID)
- rte_pdump_disable_by_deviceid(pt->device_id, pt->queue,
- pt->dir);
+ return rte_pdump_disable_by_deviceid(pt->device_id, pt->queue,
+ pt->dir);
else if (pt->dump_by_type == PORT_ID)
- rte_pdump_disable(pt->port, pt->queue, pt->dir);
+ return rte_pdump_disable(pt->port, pt->queue, pt->dir);
+
+ rte_errno = EINVAL;
+ return -EINVAL;
}
static inline void
@@ -518,15 +521,30 @@ static void
cleanup_pdump_resources(void)
{
int i;
+ int ret;
+ bool disable_failed = false;
struct pdump_tuples *pt;
char name[RTE_ETH_NAME_MAX_LEN];
- /* disable pdump and free the pdump_tuple resources */
+ /* Disable all callbacks first; freeing shared objects before this is unsafe. */
for (i = 0; i < num_tuples; i++) {
pt = &pdump_t[i];
- /* remove callbacks */
- disable_pdump(pt);
+ ret = disable_pdump(pt);
+ if (ret < 0) {
+ disable_failed = true;
+ printf("pdump disable failed (tuple=%d, errno=%d: %s); "
+ "skip teardown to avoid stale callback access\n",
+ i, rte_errno, rte_strerror(rte_errno));
+ }
+ }
+
+ if (disable_failed)
+ return;
+
+ /* free the pdump_tuple resources */
+ for (i = 0; i < num_tuples; i++) {
+ pt = &pdump_t[i];
/*
* transmit rest of the enqueued packets of the rings on to
diff --git a/doc/guides/rel_notes/release_26_07.rst b/doc/guides/rel_notes/release_26_07.rst
index 4ca0a9ac77..1700a56bc7 100644
--- a/doc/guides/rel_notes/release_26_07.rst
+++ b/doc/guides/rel_notes/release_26_07.rst
@@ -177,6 +177,13 @@ New Features
Added AGENTS.md file for AI review
and supporting scripts to review patches and documentation.
+* **Hardened pdump teardown on disable failure.**
+
+ Hardened pdump request completion and application cleanup behavior so timeout
+ or disable failure does not trigger premature pdump resource teardown.
+ pdump now uses an opt-in MP request mode that ignores peers without the
+ pdump action, while preserving legacy EAL MP behavior for all other users.
+
Removed Items
-------------
diff --git a/lib/eal/common/eal_common_proc.c b/lib/eal/common/eal_common_proc.c
index 06f151818c..5a458c0417 100644
--- a/lib/eal/common/eal_common_proc.c
+++ b/lib/eal/common/eal_common_proc.c
@@ -56,6 +56,7 @@ static struct action_entry_list action_entry_list =
enum mp_type {
MP_MSG, /* Share message with peers, will not block */
MP_REQ, /* Request for information, Will block for a reply */
+ MP_REQ_IGN, /* Request where missing action should return ignore */
MP_REP, /* Response to previously-received request */
MP_IGN, /* Response telling requester to ignore this response */
};
@@ -384,11 +385,11 @@ process_msg(struct mp_msg_internal *m, struct sockaddr_un *s)
pthread_mutex_unlock(&mp_mutex_action);
if (!action) {
- if (m->type == MP_REQ && !internal_conf->init_complete) {
- /* if this is a request, and init is not yet complete,
- * and callback wasn't registered, we should tell the
- * requester to ignore our existence because we're not
- * yet ready to process this request.
+ if ((m->type == MP_REQ && !internal_conf->init_complete) ||
+ m->type == MP_REQ_IGN) {
+ /*
+ * Ask requester to ignore this peer when action is not
+ * registered either due to early init or explicit request policy.
*/
struct rte_mp_msg dummy;
@@ -936,7 +937,8 @@ mp_request_async(const char *dst, struct rte_mp_msg *req,
static int
mp_request_sync(const char *dst, struct rte_mp_msg *req,
- struct rte_mp_reply *reply, const struct timespec *ts)
+ struct rte_mp_reply *reply, const struct timespec *ts,
+ enum mp_type req_type)
{
int ret;
pthread_condattr_t attr;
@@ -959,7 +961,7 @@ mp_request_sync(const char *dst, struct rte_mp_msg *req,
return -1;
}
- ret = send_msg(dst, req, MP_REQ);
+ ret = send_msg(dst, req, req_type);
if (ret < 0) {
EAL_LOG(ERR, "Fail to send request %s:%s",
dst, req->name);
@@ -1010,11 +1012,20 @@ RTE_EXPORT_SYMBOL(rte_mp_request_sync)
int
rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
const struct timespec *ts)
+{
+ return rte_mp_request_sync_ex(req, reply, ts, 0);
+}
+
+RTE_EXPORT_SYMBOL(rte_mp_request_sync_ex)
+int
+rte_mp_request_sync_ex(struct rte_mp_msg *req, struct rte_mp_reply *reply,
+ const struct timespec *ts, uint32_t flags)
{
int dir_fd, ret = -1;
DIR *mp_dir;
struct dirent *ent;
struct timespec now, end;
+ enum mp_type req_type = MP_REQ;
const struct internal_config *internal_conf =
eal_get_internal_configuration();
@@ -1027,6 +1038,14 @@ rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
if (check_input(req) != 0)
goto end;
+ if (flags & ~RTE_MP_REQ_F_IGNORE_NO_ACTION) {
+ rte_errno = EINVAL;
+ goto end;
+ }
+
+ if (flags & RTE_MP_REQ_F_IGNORE_NO_ACTION)
+ req_type = MP_REQ_IGN;
+
if (internal_conf->no_shconf) {
EAL_LOG(DEBUG, "No shared files mode enabled, IPC is disabled");
rte_errno = ENOTSUP;
@@ -1046,7 +1065,7 @@ rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
/* for secondary process, send request to the primary process only */
if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
pthread_mutex_lock(&pending_requests.lock);
- ret = mp_request_sync(eal_mp_socket_path(), req, reply, &end);
+ ret = mp_request_sync(eal_mp_socket_path(), req, reply, &end, req_type);
pthread_mutex_unlock(&pending_requests.lock);
goto end;
}
@@ -1086,7 +1105,7 @@ rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
/* unlocks the mutex while waiting for response,
* locks on receive
*/
- if (mp_request_sync(path, req, reply, &end))
+ if (mp_request_sync(path, req, reply, &end, req_type))
goto unlock_end;
}
ret = 0;
diff --git a/lib/eal/include/rte_eal.h b/lib/eal/include/rte_eal.h
index 7241f3be5d..415b974945 100644
--- a/lib/eal/include/rte_eal.h
+++ b/lib/eal/include/rte_eal.h
@@ -173,6 +173,15 @@ struct rte_mp_reply {
struct rte_mp_msg *msgs; /* caller to free */
};
+/** Request flags for rte_mp_request_sync_ex(). */
+enum rte_mp_request_flags {
+ /**
+ * Ask peers that do not have a registered action to return MP_IGN
+ * instead of causing timeout for this request.
+ */
+ RTE_MP_REQ_F_IGNORE_NO_ACTION = 1u << 0,
+};
+
/**
* Action function typedef used by other components.
*
@@ -292,6 +301,30 @@ int
rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
const struct timespec *ts);
+/**
+ * Send a request to peer processes with explicit request flags.
+ *
+ * This API is equivalent to rte_mp_request_sync() with opt-in behavior
+ * controls provided through @p flags.
+ *
+ * @param req
+ * The req argument contains the customized request message.
+ * @param reply
+ * The reply argument will be for storing all the replied messages;
+ * the caller is responsible for free reply->msgs.
+ * @param ts
+ * The ts argument specifies how long we can wait for the peer(s) to reply.
+ * @param flags
+ * Bitmask of values from enum rte_mp_request_flags.
+ *
+ * @return
+ * - On success, return 0.
+ * - On failure, return -1, and the reason will be stored in rte_errno.
+ */
+int
+rte_mp_request_sync_ex(struct rte_mp_msg *req, struct rte_mp_reply *reply,
+ const struct timespec *ts, uint32_t flags);
+
/**
* Send a request to the peer process and expect a reply in a separate callback.
*
diff --git a/lib/eal/windows/eal_mp.c b/lib/eal/windows/eal_mp.c
index 6703355318..1066a4d2bc 100644
--- a/lib/eal/windows/eal_mp.c
+++ b/lib/eal/windows/eal_mp.c
@@ -56,10 +56,19 @@ RTE_EXPORT_SYMBOL(rte_mp_request_sync)
int
rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
const struct timespec *ts)
+{
+ return rte_mp_request_sync_ex(req, reply, ts, 0);
+}
+
+RTE_EXPORT_SYMBOL(rte_mp_request_sync_ex)
+int
+rte_mp_request_sync_ex(struct rte_mp_msg *req, struct rte_mp_reply *reply,
+ const struct timespec *ts, uint32_t flags)
{
RTE_SET_USED(req);
RTE_SET_USED(reply);
RTE_SET_USED(ts);
+ RTE_SET_USED(flags);
EAL_LOG_NOT_IMPLEMENTED();
return -1;
}
diff --git a/lib/pdump/rte_pdump.c b/lib/pdump/rte_pdump.c
index ac94efe7ff..3764aeb2d3 100644
--- a/lib/pdump/rte_pdump.c
+++ b/lib/pdump/rte_pdump.c
@@ -3,9 +3,12 @@
*/
#include <stdlib.h>
+#include <errno.h>
#include <eal_export.h>
+#include <rte_eal.h>
#include <rte_alarm.h>
+#include <rte_cycles.h>
#include <rte_mbuf.h>
#include <rte_ethdev.h>
#include <rte_lcore.h>
@@ -68,6 +71,7 @@ struct pdump_request {
const struct rte_bpf_prm *prm;
uint32_t snaplen;
+ uint32_t generation;
};
struct pdump_response {
@@ -81,6 +85,11 @@ struct pdump_bundle {
char peer[];
};
+struct pdump_shared_ctl {
+ RTE_ATOMIC(uint32_t) enabled;
+ RTE_ATOMIC(uint32_t) inflight;
+};
+
static struct pdump_rxtx_cbs {
struct rte_ring *ring;
struct rte_mempool *mp;
@@ -88,11 +97,11 @@ static struct pdump_rxtx_cbs {
const struct rte_bpf *filter;
enum pdump_version ver;
uint32_t snaplen;
+ uint32_t generation;
RTE_ATOMIC(uint32_t) use_count;
} rx_cbs[RTE_MAX_ETHPORTS][RTE_MAX_QUEUES_PER_PORT],
tx_cbs[RTE_MAX_ETHPORTS][RTE_MAX_QUEUES_PER_PORT];
-
/*
* The packet capture statistics keep track of packets
* accepted, filtered and dropped. These are per-queue
@@ -102,9 +111,59 @@ static const char MZ_RTE_PDUMP_STATS[] = "rte_pdump_stats";
static struct {
struct rte_pdump_stats rx[RTE_MAX_ETHPORTS][RTE_MAX_QUEUES_PER_PORT];
struct rte_pdump_stats tx[RTE_MAX_ETHPORTS][RTE_MAX_QUEUES_PER_PORT];
+ struct pdump_shared_ctl rx_ctl[RTE_MAX_ETHPORTS][RTE_MAX_QUEUES_PER_PORT];
+ struct pdump_shared_ctl tx_ctl[RTE_MAX_ETHPORTS][RTE_MAX_QUEUES_PER_PORT];
+ RTE_ATOMIC(uint32_t) generation;
const struct rte_memzone *mz;
} *pdump_stats;
+static int
+pdump_enter(struct pdump_shared_ctl *ctl, const struct pdump_rxtx_cbs *cbs)
+{
+ uint32_t generation = rte_atomic_load_explicit(&pdump_stats->generation,
+ rte_memory_order_relaxed);
+
+ if (rte_atomic_load_explicit(&ctl->enabled, rte_memory_order_relaxed) == 0 ||
+ cbs->generation != generation)
+ return 0;
+
+ rte_atomic_fetch_add_explicit(&ctl->inflight, 1, rte_memory_order_acquire);
+
+ /*
+ * Disable/re-enable may race with callback entry. Re-check enabled and
+ * generation after taking inflight reference so disable can wait for this
+ * callback to drain and stale callbacks become no-op.
+ */
+ generation = rte_atomic_load_explicit(&pdump_stats->generation,
+ rte_memory_order_relaxed);
+ if (rte_atomic_load_explicit(&ctl->enabled, rte_memory_order_relaxed) != 0 &&
+ cbs->generation == generation)
+ return 1;
+
+ rte_atomic_fetch_sub_explicit(&ctl->inflight, 1, rte_memory_order_release);
+ return 0;
+}
+
+static __rte_always_inline void
+pdump_exit(struct pdump_shared_ctl *ctl)
+{
+ rte_atomic_fetch_sub_explicit(&ctl->inflight, 1, rte_memory_order_release);
+}
+
+static int
+pdump_wait_inflight(struct pdump_shared_ctl *ctl)
+{
+ uint64_t end_tsc = rte_get_timer_cycles() + MP_TIMEOUT_S * rte_get_timer_hz();
+
+ while (rte_atomic_load_explicit(&ctl->inflight, rte_memory_order_acquire) != 0) {
+ if (rte_get_timer_cycles() > end_tsc)
+ return -ETIMEDOUT;
+ rte_pause();
+ }
+
+ return 0;
+}
+
static void
pdump_cb_wait(struct pdump_rxtx_cbs *cbs)
{
@@ -224,12 +283,17 @@ pdump_rx(uint16_t port, uint16_t queue,
uint16_t max_pkts __rte_unused, void *user_params)
{
struct pdump_rxtx_cbs *cbs = user_params;
+ struct pdump_shared_ctl *ctl = &pdump_stats->rx_ctl[port][queue];
struct rte_pdump_stats *stats = &pdump_stats->rx[port][queue];
+ if (!pdump_enter(ctl, cbs))
+ return nb_pkts;
+
pdump_cb_hold(cbs);
pdump_copy(port, queue, RTE_PCAPNG_DIRECTION_IN,
pkts, nb_pkts, cbs, stats);
pdump_cb_release(cbs);
+ pdump_exit(ctl);
return nb_pkts;
}
@@ -239,12 +303,17 @@ pdump_tx(uint16_t port, uint16_t queue,
struct rte_mbuf **pkts, uint16_t nb_pkts, void *user_params)
{
struct pdump_rxtx_cbs *cbs = user_params;
+ struct pdump_shared_ctl *ctl = &pdump_stats->tx_ctl[port][queue];
struct rte_pdump_stats *stats = &pdump_stats->tx[port][queue];
+ if (!pdump_enter(ctl, cbs))
+ return nb_pkts;
+
pdump_cb_hold(cbs);
pdump_copy(port, queue, RTE_PCAPNG_DIRECTION_OUT,
pkts, nb_pkts, cbs, stats);
pdump_cb_release(cbs);
+ pdump_exit(ctl);
return nb_pkts;
}
@@ -255,20 +324,43 @@ pdump_register_rx_callbacks(enum pdump_version ver,
uint16_t end_q, uint16_t port, uint16_t queue,
struct rte_ring *ring, struct rte_mempool *mp,
struct rte_bpf *filter,
- uint16_t operation, uint32_t snaplen)
+ uint16_t operation, uint32_t snaplen,
+ uint32_t generation)
{
uint16_t qid;
qid = (queue == RTE_PDUMP_ALL_QUEUES) ? 0 : queue;
for (; qid < end_q; qid++) {
struct pdump_rxtx_cbs *cbs = &rx_cbs[port][qid];
+ struct pdump_shared_ctl *ctl = &pdump_stats->rx_ctl[port][qid];
if (operation == ENABLE) {
+ int ret;
+
if (cbs->cb) {
- PDUMP_LOG_LINE(ERR,
- "rx callback for port=%d queue=%d, already exists",
- port, qid);
- return -EEXIST;
+ if (cbs->generation == generation &&
+ rte_atomic_load_explicit(&ctl->enabled,
+ rte_memory_order_relaxed) != 0) {
+ PDUMP_LOG_LINE(ERR,
+ "rx callback for port=%d queue=%d, already exists",
+ port, qid);
+ return -EEXIST;
+ }
+
+ PDUMP_LOG_LINE(DEBUG,
+ "reconciling stale rx callback for port=%d queue=%d"
+ " old_gen=%u new_gen=%u",
+ port, qid, cbs->generation, generation);
+
+ ret = rte_eth_remove_rx_callback(port, qid, cbs->cb);
+ if (ret < 0) {
+ PDUMP_LOG_LINE(ERR,
+ "failed to reconcile stale rx callback, errno=%d",
+ -ret);
+ return ret;
+ }
+ pdump_cb_wait(cbs);
+ cbs->cb = NULL;
}
cbs->use_count = 0;
cbs->ver = ver;
@@ -276,6 +368,7 @@ pdump_register_rx_callbacks(enum pdump_version ver,
cbs->mp = mp;
cbs->snaplen = snaplen;
cbs->filter = filter;
+ cbs->generation = generation;
cbs->cb = rte_eth_add_first_rx_callback(port, qid,
pdump_rx, cbs);
@@ -286,10 +379,15 @@ pdump_register_rx_callbacks(enum pdump_version ver,
return rte_errno;
}
+ rte_atomic_store_explicit(&ctl->inflight, 0, rte_memory_order_relaxed);
+ rte_atomic_store_explicit(&ctl->enabled, 1, rte_memory_order_release);
+
memset(&pdump_stats->rx[port][qid], 0, sizeof(struct rte_pdump_stats));
} else if (operation == DISABLE) {
int ret;
+ rte_atomic_store_explicit(&ctl->enabled, 0, rte_memory_order_release);
+
if (cbs->cb == NULL) {
PDUMP_LOG_LINE(ERR,
"no existing rx callback for port=%d queue=%d",
@@ -298,6 +396,11 @@ pdump_register_rx_callbacks(enum pdump_version ver,
}
ret = rte_eth_remove_rx_callback(port, qid, cbs->cb);
if (ret < 0) {
+ /* Keep state coherent: callback is still registered,
+ * so restore enabled.
+ */
+ rte_atomic_store_explicit(&ctl->enabled, 1,
+ rte_memory_order_release);
PDUMP_LOG_LINE(ERR,
"failed to remove rx callback, errno=%d",
-ret);
@@ -305,6 +408,10 @@ pdump_register_rx_callbacks(enum pdump_version ver,
}
pdump_cb_wait(cbs);
cbs->cb = NULL;
+
+ ret = pdump_wait_inflight(ctl);
+ if (ret < 0)
+ return ret;
}
}
@@ -316,7 +423,8 @@ pdump_register_tx_callbacks(enum pdump_version ver,
uint16_t end_q, uint16_t port, uint16_t queue,
struct rte_ring *ring, struct rte_mempool *mp,
struct rte_bpf *filter,
- uint16_t operation, uint32_t snaplen)
+ uint16_t operation, uint32_t snaplen,
+ uint32_t generation)
{
uint16_t qid;
@@ -324,13 +432,36 @@ pdump_register_tx_callbacks(enum pdump_version ver,
qid = (queue == RTE_PDUMP_ALL_QUEUES) ? 0 : queue;
for (; qid < end_q; qid++) {
struct pdump_rxtx_cbs *cbs = &tx_cbs[port][qid];
+ struct pdump_shared_ctl *ctl = &pdump_stats->tx_ctl[port][qid];
if (operation == ENABLE) {
+ int ret;
+
if (cbs->cb) {
- PDUMP_LOG_LINE(ERR,
- "tx callback for port=%d queue=%d, already exists",
- port, qid);
- return -EEXIST;
+ if (cbs->generation == generation &&
+ rte_atomic_load_explicit(&ctl->enabled,
+ rte_memory_order_relaxed) != 0) {
+ PDUMP_LOG_LINE(ERR,
+ "tx callback for port=%d queue=%d, already exists",
+ port, qid);
+ return -EEXIST;
+ }
+
+ PDUMP_LOG_LINE(DEBUG,
+ "reconciling stale tx callback for port=%d queue=%d"
+ " old_gen=%u new_gen=%u",
+ port, qid, cbs->generation, generation);
+
+ ret = rte_eth_remove_tx_callback(port, qid, cbs->cb);
+ if (ret < 0) {
+ PDUMP_LOG_LINE(ERR,
+ "failed to reconcile stale tx callback, errno=%d",
+ -ret);
+ return ret;
+ }
+
+ pdump_cb_wait(cbs);
+ cbs->cb = NULL;
}
cbs->use_count = 0;
cbs->ver = ver;
@@ -338,6 +469,7 @@ pdump_register_tx_callbacks(enum pdump_version ver,
cbs->mp = mp;
cbs->snaplen = snaplen;
cbs->filter = filter;
+ cbs->generation = generation;
cbs->cb = rte_eth_add_tx_callback(port, qid, pdump_tx,
cbs);
@@ -347,10 +479,15 @@ pdump_register_tx_callbacks(enum pdump_version ver,
rte_errno);
return rte_errno;
}
+
+ rte_atomic_store_explicit(&ctl->inflight, 0, rte_memory_order_relaxed);
+ rte_atomic_store_explicit(&ctl->enabled, 1, rte_memory_order_release);
memset(&pdump_stats->tx[port][qid], 0, sizeof(struct rte_pdump_stats));
} else if (operation == DISABLE) {
int ret;
+ rte_atomic_store_explicit(&ctl->enabled, 0, rte_memory_order_release);
+
if (cbs->cb == NULL) {
PDUMP_LOG_LINE(ERR,
"no existing tx callback for port=%d queue=%d",
@@ -359,6 +496,11 @@ pdump_register_tx_callbacks(enum pdump_version ver,
}
ret = rte_eth_remove_tx_callback(port, qid, cbs->cb);
if (ret < 0) {
+ /* Keep state coherent: callback is still registered,
+ * so restore enabled.
+ */
+ rte_atomic_store_explicit(&ctl->enabled, 1,
+ rte_memory_order_release);
PDUMP_LOG_LINE(ERR,
"failed to remove tx callback, errno=%d",
-ret);
@@ -367,6 +509,10 @@ pdump_register_tx_callbacks(enum pdump_version ver,
pdump_cb_wait(cbs);
cbs->cb = NULL;
+
+ ret = pdump_wait_inflight(ctl);
+ if (ret < 0)
+ return ret;
}
}
@@ -459,7 +605,8 @@ set_pdump_rxtx_cbs(const struct pdump_request *p)
end_q = (queue == RTE_PDUMP_ALL_QUEUES) ? nb_rx_q : queue + 1;
ret = pdump_register_rx_callbacks(p->ver, end_q, port, queue,
ring, mp, filter,
- operation, p->snaplen);
+ operation, p->snaplen,
+ p->generation);
if (ret < 0)
return ret;
}
@@ -469,7 +616,8 @@ set_pdump_rxtx_cbs(const struct pdump_request *p)
end_q = (queue == RTE_PDUMP_ALL_QUEUES) ? nb_tx_q : queue + 1;
ret = pdump_register_tx_callbacks(p->ver, end_q, port, queue,
ring, mp, filter,
- operation, p->snaplen);
+ operation, p->snaplen,
+ p->generation);
if (ret < 0)
return ret;
}
@@ -477,12 +625,14 @@ set_pdump_rxtx_cbs(const struct pdump_request *p)
return ret;
}
-static void
-pdump_request_to_secondary(const struct pdump_request *req)
+static int
+pdump_request_to_secondary_sync(const struct pdump_request *req)
{
struct rte_mp_msg mp_req = { };
struct rte_mp_reply mp_reply;
struct timespec ts = {.tv_sec = MP_TIMEOUT_S, .tv_nsec = 0};
+ int ret = 0;
+ uint16_t i;
PDUMP_LOG_LINE(DEBUG, "forward req %s to secondary", pdump_opname(req->op));
@@ -490,14 +640,41 @@ pdump_request_to_secondary(const struct pdump_request *req)
strlcpy(mp_req.name, PDUMP_MP, sizeof(mp_req.name));
mp_req.len_param = sizeof(*req);
- if (rte_mp_request_sync(&mp_req, &mp_reply, &ts) != 0)
- PDUMP_LOG_LINE(ERR, "rte_mp_request_sync failed");
+ if (rte_mp_request_sync_ex(&mp_req, &mp_reply, &ts,
+ RTE_MP_REQ_F_IGNORE_NO_ACTION) != 0) {
+ PDUMP_LOG_LINE(ERR, "rte_mp_request_sync failed: %s",
+ strerror(rte_errno));
+ return -rte_errno;
+ }
- else if (mp_reply.nb_sent != mp_reply.nb_received)
+ if (mp_reply.nb_sent != mp_reply.nb_received) {
PDUMP_LOG_LINE(ERR, "not all secondary's replied (sent %u recv %u)",
mp_reply.nb_sent, mp_reply.nb_received);
+ ret = -ETIMEDOUT;
+ }
+
+ for (i = 0; i < mp_reply.nb_received; i++) {
+ struct rte_mp_msg *mp_rep = &mp_reply.msgs[i];
+ const struct pdump_response *resp;
+
+ if (mp_rep->len_param != sizeof(*resp)) {
+ PDUMP_LOG_LINE(ERR, "invalid secondary reply size %u", mp_rep->len_param);
+ if (ret == 0)
+ ret = -EINVAL;
+ continue;
+ }
+
+ resp = (const struct pdump_response *)mp_rep->param;
+ if (resp->err_value != 0) {
+ PDUMP_LOG_LINE(ERR, "secondary reply failed: op=%u err=%d",
+ resp->res_op, resp->err_value);
+ if (ret == 0)
+ ret = resp->err_value;
+ }
+ }
free(mp_reply.msgs);
+ return ret;
}
/* Allocate temporary storage for passing state to the alarm thread for deferred handling */
@@ -556,7 +733,10 @@ pdump_handle_primary_request(const struct rte_mp_msg *mp_msg, const void *peer)
PDUMP_LOG_LINE(DEBUG, "secondary pdump %s", pdump_opname(req->op));
/* Can just do it now, no need for interrupt thread */
- ret = set_pdump_rxtx_cbs(req);
+ if (req->op == ENABLE || req->op == DISABLE)
+ ret = set_pdump_rxtx_cbs(req);
+ else
+ ret = -EINVAL;
}
return pdump_send_response(req, ret, peer);
@@ -569,17 +749,29 @@ __pdump_request(void *param)
{
struct pdump_bundle *bundle = param;
struct rte_mp_msg *msg = &bundle->msg;
- const struct pdump_request *req =
- (const struct pdump_request *)msg->param;
+ struct pdump_request *req =
+ (struct pdump_request *)msg->param;
int ret;
PDUMP_LOG_LINE(DEBUG, "primary pdump %s", pdump_opname(req->op));
+ if (req->op == DISABLE) {
+ req->generation = rte_atomic_fetch_add_explicit(&pdump_stats->generation, 1,
+ rte_memory_order_acq_rel) + 1;
+ } else if (req->op == ENABLE) {
+ req->generation = rte_atomic_load_explicit(&pdump_stats->generation,
+ rte_memory_order_acquire);
+ }
+
ret = set_pdump_rxtx_cbs(req);
- /* Primary process is responsible for broadcasting request to all secondaries */
+ /*
+ * Primary process is responsible for broadcasting the request to all
+ * secondaries. The sync request uses opt-in ignore-missing-action mode so
+ * pdump does not depend on unrelated secondary processes.
+ */
if (ret == 0)
- pdump_request_to_secondary(req);
+ ret = pdump_request_to_secondary_sync(req);
pdump_send_response(req, ret, bundle->peer);
free(bundle);
@@ -659,6 +851,9 @@ rte_pdump_init(void)
pdump_stats = mz->addr;
pdump_stats->mz = mz;
+ if (rte_eal_process_type() == RTE_PROC_PRIMARY)
+ rte_atomic_store_explicit(&pdump_stats->generation, 1,
+ rte_memory_order_release);
return 0;
}
diff --git a/lib/pdump/rte_pdump.h b/lib/pdump/rte_pdump.h
index 1e32d46097..77e5914420 100644
--- a/lib/pdump/rte_pdump.h
+++ b/lib/pdump/rte_pdump.h
@@ -64,7 +64,7 @@ rte_pdump_uninit(void);
* queues of a given port.
* @param flags
* flags specifies RTE_PDUMP_FLAG_RX/RTE_PDUMP_FLAG_TX/RTE_PDUMP_FLAG_RXTX
- * on which packet capturing should be enabled for a given port and queue.
+ * on which packet capturing should be disabled for a given port and queue.
* @param ring
* ring on which captured packets will be enqueued for user.
* @param mp
@@ -72,6 +72,11 @@ rte_pdump_uninit(void);
* @param filter
* Unused should be NULL.
*
+ * @note
+ * In applications that enable capture on multiple interfaces, enable may be
+ * partially applied before an error is returned. Callers should explicitly
+ * unwind partial enable state.
+ *
* @return
* 0 on success, -1 on error, rte_errno is set accordingly.
*/
@@ -103,6 +108,11 @@ rte_pdump_enable(uint16_t port, uint16_t queue, uint32_t flags,
* @param prm
* Use BPF program to run to filter packes (can be NULL)
*
+ * @note
+ * In applications that enable capture on multiple interfaces, enable may be
+ * partially applied before an error is returned. Callers should explicitly
+ * unwind partial enable state.
+ *
* @return
* 0 on success, -1 on error, rte_errno is set accordingly.
*/
@@ -124,7 +134,14 @@ rte_pdump_enable_bpf(uint16_t port_id, uint16_t queue,
* queues of a given port.
* @param flags
* flags specifies RTE_PDUMP_FLAG_RX/RTE_PDUMP_FLAG_TX/RTE_PDUMP_FLAG_RXTX
- * on which packet capturing should be enabled for a given port and queue.
+ * on which packet capturing should be disabled for a given port and queue.
+ *
+ * @note
+ * A disable failure (including timeout/no response) means teardown is not
+ * complete across pdump-enabled peers. The caller must not release shared
+ * pdump resources and must not uninitialize pdump for that capture session
+ * until disable succeeds. Releasing resources on disable failure can lead to
+ * crashes in peer processes still accessing those resources.
*
* @return
* 0 on success, -1 on error, rte_errno is set accordingly.
@@ -153,6 +170,11 @@ rte_pdump_disable(uint16_t port, uint16_t queue, uint32_t flags);
* @param filter
* unused should be NULL
*
+ * @note
+ * In applications that enable capture on multiple interfaces, enable may be
+ * partially applied before an error is returned. Callers should explicitly
+ * unwind partial enable state.
+ *
* @return
* 0 on success, -1 on error, rte_errno is set accordingly.
*/
@@ -186,6 +208,11 @@ rte_pdump_enable_by_deviceid(char *device_id, uint16_t queue,
* @param filter
* Use BPF program to run to filter packes (can be NULL)
*
+ * @note
+ * In applications that enable capture on multiple interfaces, enable may be
+ * partially applied before an error is returned. Callers should explicitly
+ * unwind partial enable state.
+ *
* @return
* 0 on success, -1 on error, rte_errno is set accordingly.
*/
@@ -210,7 +237,14 @@ rte_pdump_enable_bpf_by_deviceid(const char *device_id, uint16_t queue,
* queues of a given device id.
* @param flags
* flags specifies RTE_PDUMP_FLAG_RX/RTE_PDUMP_FLAG_TX/RTE_PDUMP_FLAG_RXTX
- * on which packet capturing should be enabled for a given port and queue.
+ * on which packet capturing should be disabled for a given port and queue.
+ *
+ * @note
+ * A disable failure (including timeout/no response) means teardown is not
+ * complete across pdump-enabled peers. The caller must not release shared
+ * pdump resources and must not uninitialize pdump for that capture session
+ * until disable succeeds. Releasing resources on disable failure can lead to
+ * crashes in peer processes still accessing those resources.
*
* @return
* 0 on success, -1 on error, rte_errno is set accordingly.
--
2.43.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v3] pdump: fix teardown race with opt-in MP request mode
2026-07-05 8:44 ` [PATCH v2] pdump: fix teardown race with opt-in MP request mode Pushpendra Kumar
@ 2026-07-05 17:07 ` Pushpendra Kumar
2026-07-05 18:27 ` [PATCH v2] " Stephen Hemminger
1 sibling, 0 replies; 8+ messages in thread
From: Pushpendra Kumar @ 2026-07-05 17:07 UTC (permalink / raw)
To: dev
Cc: stable, reshma.pattan, stephen, anatoly.burakov, dmitry.kozliuk,
thomas, Pushpendra Kumar
This change addresses two pdump teardown failure cases:
1. A requester secondary can time out if a forwarded secondary is slow,
unresponsive, or does not implement rte_pdump_init().
2. After a DISABLE request times out, the requester secondary receives
the reply and releases shared resources, which other slow or
unresponsive secondaries may access, leading to a crash.
Add an opt-in EAL multiprocess request mode that allows a caller to
ignore peers that do not implement the requested action, while preserving
the existing rte_mp_request_sync() behavior for legacy users.
pdump uses this new mode when forwarding enable/disable requests and adds
the following behavior:
- track request generation so stale callbacks cannot act on a newer
enable/disable cycle
- wait for in-flight callbacks to drain before completing disable
- prevent shared resource teardown from racing with delayed secondary
processing
- keep pdump intent-driven without extra consumer-specific workaround
logic
The EAL change is required because the current mp request path treats
a missing action as a failure, which cannot satisfy pdump's needs without
changing behavior for all existing users. The new API is opt-in, so
existing callers of rte_mp_request_sync() keep their current semantics.
Validation:
- Baseline-vs-changes benchmark used DPDK-native test-pmd as primary and
dumpcap as secondary.
- Both runs captured 1024 packets with the same packet-count exit
condition.
- A 5-second perf stat sample on the forwarding primary showed no
meaningful regression: with changes recorded 12,402,055,320 cycles and
17,391,769,084 instructions, versus 12,450,031,900 cycles and
17,499,706,053 instructions on the parent baseline.
- This validates that the primary forwarding path is not materially
affected.
- Secondary callback-path overhead for pdump_enter/pdump_exit was not
separately microbenchmarked in this patch.
Fixes: c3ceb8742295 ("pdump: forward callback enable to secondary process")
Signed-off-by: Pushpendra Kumar <pushpendra1x.kumar@intel.com>
Cc: stable@dpdk.org
---
v3:
- strengthen `pdump_enter()` generation loads to use acquire ordering
- clarify why `pdump_enter()` re-checks generation after taking `inflight`
- document the distinct roles of `use_count` and `inflight`
- document bounded disable timeout and retry expectation
- document first-error return policy in `pdump_request_to_secondary_sync()`
- document that unknown `rte_mp_request_sync_ex()` flags are rejected intentionally
- document why generation starts at 1 on primary init
- clarify fail-closed cleanup behavior and same-process recovery limitation
- reword release notes to emphasize opt-in EAL API/flag semantics and unchanged legacy behavior
- clarify commit-message validation scope: perf numbers cover the forwarding primary path only
Not changed:
- no automatic RX/TX rollback on partial enable failure
Current pdump enable intentionally remains partial-apply as documented in the public API. Adding rollback would change semantics toward transactional enable and introduce additional failure handling if rollback itself fails.
- no non-NULL `cb` after successful callback removal on timeout
After successful `rte_eth_remove_*x_callback()`, `cb` must continue to reflect actual ethdev registration state. Leaving it non-NULL would make local state inconsistent and could trigger incorrect later remove/reconcile behavior.
- no RX/TX helper refactor in this correctness patch
This area is concurrency-sensitive and RX/TX callback APIs are not identical, so keeping the logic local reduces regression risk.
If you want a shorter version for the email intro instead of the `---` block, use:
v3 updates mainly tighten documentation and clarify intent around the accepted fixes:
acquire ordering in `pdump_enter()`, explicit comments for generation re-check, `use_count` vs `inflight`, bounded timeout/retry, first-error aggregation, unknown-flag rejection, and generation initialization; fail-closed cleanup/recovery notes; release-note wording for the new opt-in EAL API; and commit-message clarification that current perf numbers cover the forwarding primary path only.
.mailmap | 1 +
app/dumpcap/main.c | 24 ++-
app/pdump/main.c | 37 +++-
doc/guides/rel_notes/release_26_07.rst | 9 +
lib/eal/common/eal_common_proc.c | 41 +++-
lib/eal/include/rte_eal.h | 33 +++
lib/eal/windows/eal_mp.c | 9 +
lib/pdump/rte_pdump.c | 266 ++++++++++++++++++++++---
lib/pdump/rte_pdump.h | 42 +++-
9 files changed, 417 insertions(+), 45 deletions(-)
diff --git a/.mailmap b/.mailmap
index 05a55c0bd6..3bfe0c4df0 100644
--- a/.mailmap
+++ b/.mailmap
@@ -1338,6 +1338,7 @@ Przemyslaw Gierszynski <przemyslaw.gierszynski@intel.com>
Przemyslaw Patynowski <przemyslawx.patynowski@intel.com>
Przemyslaw Zegan <przemyslawx.zegan@intel.com>
Pu Xu <583493798@qq.com>
+Pushpendra Kumar <pushpendra1x.kumar@intel.com>
Qi Fu <qi.fu@intel.com>
Qi Zhang <qi.z.zhang@intel.com>
Qian Hao <qi_an_hao@126.com>
diff --git a/app/dumpcap/main.c b/app/dumpcap/main.c
index 46a6cb251e..c6ae916075 100644
--- a/app/dumpcap/main.c
+++ b/app/dumpcap/main.c
@@ -521,14 +521,34 @@ static void
cleanup_pdump_resources(void)
{
struct interface *intf;
+ int ret;
+ bool disable_failed = false;
TAILQ_FOREACH(intf, &interfaces, next) {
- rte_pdump_disable(intf->port,
- RTE_PDUMP_ALL_QUEUES, RTE_PDUMP_FLAG_RXTX);
+ ret = rte_pdump_disable(intf->port,
+ RTE_PDUMP_ALL_QUEUES, RTE_PDUMP_FLAG_RXTX);
+ if (ret < 0) {
+ disable_failed = true;
+ fprintf(stderr,
+ "Disable pdump failed on %u:%s: %s\n",
+ intf->port, intf->name,
+ rte_strerror(rte_errno));
+ }
if (intf->opts.promisc_mode)
rte_eth_promiscuous_disable(intf->port);
}
+ if (disable_failed) {
+ /*
+ * Fail-closed: if disable did not complete, keep pdump action/state
+ * alive and do not uninit shared capture resources. Recovery in the
+ * same process instance is not guaranteed until disable succeeds.
+ */
+ fprintf(stderr,
+ "Skipping pdump uninit because disable did not complete on all interfaces\n");
+ return;
+ }
+
rte_pdump_uninit();
}
diff --git a/app/pdump/main.c b/app/pdump/main.c
index 1e62c8adc1..8f33d2f603 100644
--- a/app/pdump/main.c
+++ b/app/pdump/main.c
@@ -451,14 +451,17 @@ print_pdump_stats(void)
}
}
-static inline void
+static inline int
disable_pdump(struct pdump_tuples *pt)
{
if (pt->dump_by_type == DEVICE_ID)
- rte_pdump_disable_by_deviceid(pt->device_id, pt->queue,
- pt->dir);
+ return rte_pdump_disable_by_deviceid(pt->device_id, pt->queue,
+ pt->dir);
else if (pt->dump_by_type == PORT_ID)
- rte_pdump_disable(pt->port, pt->queue, pt->dir);
+ return rte_pdump_disable(pt->port, pt->queue, pt->dir);
+
+ rte_errno = EINVAL;
+ return -EINVAL;
}
static inline void
@@ -518,15 +521,35 @@ static void
cleanup_pdump_resources(void)
{
int i;
+ int ret;
+ bool disable_failed = false;
struct pdump_tuples *pt;
char name[RTE_ETH_NAME_MAX_LEN];
- /* disable pdump and free the pdump_tuple resources */
+ /* Disable all callbacks first; freeing shared objects before this is unsafe. */
for (i = 0; i < num_tuples; i++) {
pt = &pdump_t[i];
- /* remove callbacks */
- disable_pdump(pt);
+ ret = disable_pdump(pt);
+ if (ret < 0) {
+ disable_failed = true;
+ printf("pdump disable failed (tuple=%d, errno=%d: %s); "
+ "skip teardown to avoid stale callback access\n",
+ i, rte_errno, rte_strerror(rte_errno));
+ }
+ }
+
+ /*
+ * Fail-closed: do not uninit/free shared pdump resources after disable
+ * failure. Recovery in the same process instance is not guaranteed until
+ * disable succeeds.
+ */
+ if (disable_failed)
+ return;
+
+ /* free the pdump_tuple resources */
+ for (i = 0; i < num_tuples; i++) {
+ pt = &pdump_t[i];
/*
* transmit rest of the enqueued packets of the rings on to
diff --git a/doc/guides/rel_notes/release_26_07.rst b/doc/guides/rel_notes/release_26_07.rst
index 8b1bdada1a..51fc74e7ef 100644
--- a/doc/guides/rel_notes/release_26_07.rst
+++ b/doc/guides/rel_notes/release_26_07.rst
@@ -250,6 +250,15 @@ New Features
Added AGENTS.md file for AI review
and supporting scripts to review patches and documentation.
+* **Hardened pdump teardown on disable failure.**
+
+ Hardened pdump request completion and application cleanup behavior so timeout
+ or disable failure does not trigger premature pdump resource teardown.
+ EAL now provides ``rte_mp_request_sync_ex()`` with
+ ``RTE_MP_REQ_F_IGNORE_NO_ACTION`` to ignore peers that do not implement
+ the requested action. pdump uses this opt-in mode, while legacy
+ ``rte_mp_request_sync()`` behavior remains unchanged.
+
Removed Items
-------------
diff --git a/lib/eal/common/eal_common_proc.c b/lib/eal/common/eal_common_proc.c
index 5133eaaa47..fbfecd82d3 100644
--- a/lib/eal/common/eal_common_proc.c
+++ b/lib/eal/common/eal_common_proc.c
@@ -56,6 +56,7 @@ static struct action_entry_list action_entry_list =
enum mp_type {
MP_MSG, /* Share message with peers, will not block */
MP_REQ, /* Request for information, Will block for a reply */
+ MP_REQ_IGN, /* Request where missing action should return ignore */
MP_REP, /* Response to previously-received request */
MP_IGN, /* Response telling requester to ignore this response */
};
@@ -399,11 +400,11 @@ process_msg(struct mp_msg_internal *m, struct sockaddr_un *s)
pthread_mutex_unlock(&mp_mutex_action);
if (!action) {
- if (m->type == MP_REQ && !internal_conf->init_complete) {
- /* if this is a request, and init is not yet complete,
- * and callback wasn't registered, we should tell the
- * requester to ignore our existence because we're not
- * yet ready to process this request.
+ if ((m->type == MP_REQ && !internal_conf->init_complete) ||
+ m->type == MP_REQ_IGN) {
+ /*
+ * Ask requester to ignore this peer when action is not
+ * registered either due to early init or explicit request policy.
*/
struct rte_mp_msg dummy;
@@ -947,7 +948,8 @@ mp_request_async(const char *dst, struct rte_mp_msg *req,
static int
mp_request_sync(const char *dst, struct rte_mp_msg *req,
- struct rte_mp_reply *reply, const struct timespec *ts)
+ struct rte_mp_reply *reply, const struct timespec *ts,
+ enum mp_type req_type)
{
int ret;
pthread_condattr_t attr;
@@ -970,7 +972,7 @@ mp_request_sync(const char *dst, struct rte_mp_msg *req,
return -1;
}
- ret = send_msg(dst, req, MP_REQ);
+ ret = send_msg(dst, req, req_type);
if (ret < 0) {
EAL_LOG(ERR, "Fail to send request %s:%s",
dst, req->name);
@@ -1021,11 +1023,20 @@ RTE_EXPORT_SYMBOL(rte_mp_request_sync)
int
rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
const struct timespec *ts)
+{
+ return rte_mp_request_sync_ex(req, reply, ts, 0);
+}
+
+RTE_EXPORT_SYMBOL(rte_mp_request_sync_ex)
+int
+rte_mp_request_sync_ex(struct rte_mp_msg *req, struct rte_mp_reply *reply,
+ const struct timespec *ts, uint32_t flags)
{
int dir_fd, ret = -1;
DIR *mp_dir;
struct dirent *ent;
struct timespec now, end;
+ enum mp_type req_type = MP_REQ;
const struct internal_config *internal_conf =
eal_get_internal_configuration();
@@ -1038,6 +1049,18 @@ rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
if (check_input(req) != 0)
goto end;
+ /*
+ * Reject unknown flags for safety instead of silently ignoring them.
+ * Future flags require corresponding support in this implementation.
+ */
+ if (flags & ~RTE_MP_REQ_F_IGNORE_NO_ACTION) {
+ rte_errno = EINVAL;
+ goto end;
+ }
+
+ if (flags & RTE_MP_REQ_F_IGNORE_NO_ACTION)
+ req_type = MP_REQ_IGN;
+
if (internal_conf->no_shconf) {
EAL_LOG(DEBUG, "No shared files mode enabled, IPC is disabled");
rte_errno = ENOTSUP;
@@ -1057,7 +1080,7 @@ rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
/* for secondary process, send request to the primary process only */
if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
pthread_mutex_lock(&pending_requests.lock);
- ret = mp_request_sync(eal_mp_socket_path(), req, reply, &end);
+ ret = mp_request_sync(eal_mp_socket_path(), req, reply, &end, req_type);
pthread_mutex_unlock(&pending_requests.lock);
goto end;
}
@@ -1097,7 +1120,7 @@ rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
/* unlocks the mutex while waiting for response,
* locks on receive
*/
- if (mp_request_sync(path, req, reply, &end))
+ if (mp_request_sync(path, req, reply, &end, req_type))
goto unlock_end;
}
ret = 0;
diff --git a/lib/eal/include/rte_eal.h b/lib/eal/include/rte_eal.h
index 7241f3be5d..415b974945 100644
--- a/lib/eal/include/rte_eal.h
+++ b/lib/eal/include/rte_eal.h
@@ -173,6 +173,15 @@ struct rte_mp_reply {
struct rte_mp_msg *msgs; /* caller to free */
};
+/** Request flags for rte_mp_request_sync_ex(). */
+enum rte_mp_request_flags {
+ /**
+ * Ask peers that do not have a registered action to return MP_IGN
+ * instead of causing timeout for this request.
+ */
+ RTE_MP_REQ_F_IGNORE_NO_ACTION = 1u << 0,
+};
+
/**
* Action function typedef used by other components.
*
@@ -292,6 +301,30 @@ int
rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
const struct timespec *ts);
+/**
+ * Send a request to peer processes with explicit request flags.
+ *
+ * This API is equivalent to rte_mp_request_sync() with opt-in behavior
+ * controls provided through @p flags.
+ *
+ * @param req
+ * The req argument contains the customized request message.
+ * @param reply
+ * The reply argument will be for storing all the replied messages;
+ * the caller is responsible for free reply->msgs.
+ * @param ts
+ * The ts argument specifies how long we can wait for the peer(s) to reply.
+ * @param flags
+ * Bitmask of values from enum rte_mp_request_flags.
+ *
+ * @return
+ * - On success, return 0.
+ * - On failure, return -1, and the reason will be stored in rte_errno.
+ */
+int
+rte_mp_request_sync_ex(struct rte_mp_msg *req, struct rte_mp_reply *reply,
+ const struct timespec *ts, uint32_t flags);
+
/**
* Send a request to the peer process and expect a reply in a separate callback.
*
diff --git a/lib/eal/windows/eal_mp.c b/lib/eal/windows/eal_mp.c
index 6703355318..1066a4d2bc 100644
--- a/lib/eal/windows/eal_mp.c
+++ b/lib/eal/windows/eal_mp.c
@@ -56,10 +56,19 @@ RTE_EXPORT_SYMBOL(rte_mp_request_sync)
int
rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
const struct timespec *ts)
+{
+ return rte_mp_request_sync_ex(req, reply, ts, 0);
+}
+
+RTE_EXPORT_SYMBOL(rte_mp_request_sync_ex)
+int
+rte_mp_request_sync_ex(struct rte_mp_msg *req, struct rte_mp_reply *reply,
+ const struct timespec *ts, uint32_t flags)
{
RTE_SET_USED(req);
RTE_SET_USED(reply);
RTE_SET_USED(ts);
+ RTE_SET_USED(flags);
EAL_LOG_NOT_IMPLEMENTED();
return -1;
}
diff --git a/lib/pdump/rte_pdump.c b/lib/pdump/rte_pdump.c
index ac94efe7ff..f55f43774c 100644
--- a/lib/pdump/rte_pdump.c
+++ b/lib/pdump/rte_pdump.c
@@ -3,9 +3,12 @@
*/
#include <stdlib.h>
+#include <errno.h>
#include <eal_export.h>
+#include <rte_eal.h>
#include <rte_alarm.h>
+#include <rte_cycles.h>
#include <rte_mbuf.h>
#include <rte_ethdev.h>
#include <rte_lcore.h>
@@ -29,7 +32,10 @@ RTE_LOG_REGISTER_DEFAULT(pdump_logtype, NOTICE);
#define PDUMP_BURST_SIZE 32u
-/* Overly generous timeout for secondary to respond */
+/*
+ * Shared timeout budget for both MP request/reply and inflight callback drain
+ * during disable. Kept fixed for now to avoid API/config scope changes.
+ */
#define MP_TIMEOUT_S 5
enum pdump_operation {
@@ -68,6 +74,7 @@ struct pdump_request {
const struct rte_bpf_prm *prm;
uint32_t snaplen;
+ uint32_t generation;
};
struct pdump_response {
@@ -81,6 +88,11 @@ struct pdump_bundle {
char peer[];
};
+struct pdump_shared_ctl {
+ RTE_ATOMIC(uint32_t) enabled;
+ RTE_ATOMIC(uint32_t) inflight;
+};
+
static struct pdump_rxtx_cbs {
struct rte_ring *ring;
struct rte_mempool *mp;
@@ -88,11 +100,11 @@ static struct pdump_rxtx_cbs {
const struct rte_bpf *filter;
enum pdump_version ver;
uint32_t snaplen;
+ uint32_t generation;
RTE_ATOMIC(uint32_t) use_count;
} rx_cbs[RTE_MAX_ETHPORTS][RTE_MAX_QUEUES_PER_PORT],
tx_cbs[RTE_MAX_ETHPORTS][RTE_MAX_QUEUES_PER_PORT];
-
/*
* The packet capture statistics keep track of packets
* accepted, filtered and dropped. These are per-queue
@@ -102,12 +114,73 @@ static const char MZ_RTE_PDUMP_STATS[] = "rte_pdump_stats";
static struct {
struct rte_pdump_stats rx[RTE_MAX_ETHPORTS][RTE_MAX_QUEUES_PER_PORT];
struct rte_pdump_stats tx[RTE_MAX_ETHPORTS][RTE_MAX_QUEUES_PER_PORT];
+ struct pdump_shared_ctl rx_ctl[RTE_MAX_ETHPORTS][RTE_MAX_QUEUES_PER_PORT];
+ struct pdump_shared_ctl tx_ctl[RTE_MAX_ETHPORTS][RTE_MAX_QUEUES_PER_PORT];
+ RTE_ATOMIC(uint32_t) generation;
const struct rte_memzone *mz;
} *pdump_stats;
+static int
+pdump_enter(struct pdump_shared_ctl *ctl, const struct pdump_rxtx_cbs *cbs)
+{
+ uint32_t generation = rte_atomic_load_explicit(&pdump_stats->generation,
+ rte_memory_order_acquire);
+
+ if (rte_atomic_load_explicit(&ctl->enabled, rte_memory_order_relaxed) == 0 ||
+ cbs->generation != generation)
+ return 0;
+
+ rte_atomic_fetch_add_explicit(&ctl->inflight, 1, rte_memory_order_acquire);
+
+ /*
+ * Disable/re-enable may race with callback entry. Re-check enabled and
+ * generation after taking inflight reference so disable can wait for this
+ * callback to drain and stale callbacks become no-op. The second generation
+ * load is intentional: it detects an epoch change that happens between the
+ * first check and inflight increment.
+ */
+ generation = rte_atomic_load_explicit(&pdump_stats->generation,
+ rte_memory_order_acquire);
+ if (rte_atomic_load_explicit(&ctl->enabled, rte_memory_order_relaxed) != 0 &&
+ cbs->generation == generation)
+ return 1;
+
+ rte_atomic_fetch_sub_explicit(&ctl->inflight, 1, rte_memory_order_release);
+ return 0;
+}
+
+static __rte_always_inline void
+pdump_exit(struct pdump_shared_ctl *ctl)
+{
+ rte_atomic_fetch_sub_explicit(&ctl->inflight, 1, rte_memory_order_release);
+}
+
+static int
+pdump_wait_inflight(struct pdump_shared_ctl *ctl)
+{
+ uint64_t end_tsc = rte_get_timer_cycles() + MP_TIMEOUT_S * rte_get_timer_hz();
+
+ /*
+ * Use the same timeout budget as MP sync requests. Under heavy load this
+ * can time out and fail closed, requiring disable retry by the caller.
+ */
+
+ while (rte_atomic_load_explicit(&ctl->inflight, rte_memory_order_acquire) != 0) {
+ if (rte_get_timer_cycles() > end_tsc)
+ return -ETIMEDOUT;
+ rte_pause();
+ }
+
+ return 0;
+}
+
static void
pdump_cb_wait(struct pdump_rxtx_cbs *cbs)
{
+ /*
+ * use_count protects callback-owned data (for example filter lifetime),
+ * while ctl->inflight protects callback entry/teardown synchronization.
+ */
/* make sure the data loads happens before the use count load */
rte_atomic_thread_fence(rte_memory_order_acquire);
@@ -224,12 +297,17 @@ pdump_rx(uint16_t port, uint16_t queue,
uint16_t max_pkts __rte_unused, void *user_params)
{
struct pdump_rxtx_cbs *cbs = user_params;
+ struct pdump_shared_ctl *ctl = &pdump_stats->rx_ctl[port][queue];
struct rte_pdump_stats *stats = &pdump_stats->rx[port][queue];
+ if (!pdump_enter(ctl, cbs))
+ return nb_pkts;
+
pdump_cb_hold(cbs);
pdump_copy(port, queue, RTE_PCAPNG_DIRECTION_IN,
pkts, nb_pkts, cbs, stats);
pdump_cb_release(cbs);
+ pdump_exit(ctl);
return nb_pkts;
}
@@ -239,12 +317,17 @@ pdump_tx(uint16_t port, uint16_t queue,
struct rte_mbuf **pkts, uint16_t nb_pkts, void *user_params)
{
struct pdump_rxtx_cbs *cbs = user_params;
+ struct pdump_shared_ctl *ctl = &pdump_stats->tx_ctl[port][queue];
struct rte_pdump_stats *stats = &pdump_stats->tx[port][queue];
+ if (!pdump_enter(ctl, cbs))
+ return nb_pkts;
+
pdump_cb_hold(cbs);
pdump_copy(port, queue, RTE_PCAPNG_DIRECTION_OUT,
pkts, nb_pkts, cbs, stats);
pdump_cb_release(cbs);
+ pdump_exit(ctl);
return nb_pkts;
}
@@ -255,20 +338,43 @@ pdump_register_rx_callbacks(enum pdump_version ver,
uint16_t end_q, uint16_t port, uint16_t queue,
struct rte_ring *ring, struct rte_mempool *mp,
struct rte_bpf *filter,
- uint16_t operation, uint32_t snaplen)
+ uint16_t operation, uint32_t snaplen,
+ uint32_t generation)
{
uint16_t qid;
qid = (queue == RTE_PDUMP_ALL_QUEUES) ? 0 : queue;
for (; qid < end_q; qid++) {
struct pdump_rxtx_cbs *cbs = &rx_cbs[port][qid];
+ struct pdump_shared_ctl *ctl = &pdump_stats->rx_ctl[port][qid];
if (operation == ENABLE) {
+ int ret;
+
if (cbs->cb) {
- PDUMP_LOG_LINE(ERR,
- "rx callback for port=%d queue=%d, already exists",
- port, qid);
- return -EEXIST;
+ if (cbs->generation == generation &&
+ rte_atomic_load_explicit(&ctl->enabled,
+ rte_memory_order_relaxed) != 0) {
+ PDUMP_LOG_LINE(ERR,
+ "rx callback for port=%d queue=%d, already exists",
+ port, qid);
+ return -EEXIST;
+ }
+
+ PDUMP_LOG_LINE(DEBUG,
+ "reconciling stale rx callback for port=%d queue=%d"
+ " old_gen=%u new_gen=%u",
+ port, qid, cbs->generation, generation);
+
+ ret = rte_eth_remove_rx_callback(port, qid, cbs->cb);
+ if (ret < 0) {
+ PDUMP_LOG_LINE(ERR,
+ "failed to reconcile stale rx callback, errno=%d",
+ -ret);
+ return ret;
+ }
+ pdump_cb_wait(cbs);
+ cbs->cb = NULL;
}
cbs->use_count = 0;
cbs->ver = ver;
@@ -276,6 +382,7 @@ pdump_register_rx_callbacks(enum pdump_version ver,
cbs->mp = mp;
cbs->snaplen = snaplen;
cbs->filter = filter;
+ cbs->generation = generation;
cbs->cb = rte_eth_add_first_rx_callback(port, qid,
pdump_rx, cbs);
@@ -286,10 +393,15 @@ pdump_register_rx_callbacks(enum pdump_version ver,
return rte_errno;
}
+ rte_atomic_store_explicit(&ctl->inflight, 0, rte_memory_order_relaxed);
+ rte_atomic_store_explicit(&ctl->enabled, 1, rte_memory_order_release);
+
memset(&pdump_stats->rx[port][qid], 0, sizeof(struct rte_pdump_stats));
} else if (operation == DISABLE) {
int ret;
+ rte_atomic_store_explicit(&ctl->enabled, 0, rte_memory_order_release);
+
if (cbs->cb == NULL) {
PDUMP_LOG_LINE(ERR,
"no existing rx callback for port=%d queue=%d",
@@ -298,6 +410,11 @@ pdump_register_rx_callbacks(enum pdump_version ver,
}
ret = rte_eth_remove_rx_callback(port, qid, cbs->cb);
if (ret < 0) {
+ /* Keep state coherent: callback is still registered,
+ * so restore enabled.
+ */
+ rte_atomic_store_explicit(&ctl->enabled, 1,
+ rte_memory_order_release);
PDUMP_LOG_LINE(ERR,
"failed to remove rx callback, errno=%d",
-ret);
@@ -305,6 +422,10 @@ pdump_register_rx_callbacks(enum pdump_version ver,
}
pdump_cb_wait(cbs);
cbs->cb = NULL;
+
+ ret = pdump_wait_inflight(ctl);
+ if (ret < 0)
+ return ret;
}
}
@@ -316,7 +437,8 @@ pdump_register_tx_callbacks(enum pdump_version ver,
uint16_t end_q, uint16_t port, uint16_t queue,
struct rte_ring *ring, struct rte_mempool *mp,
struct rte_bpf *filter,
- uint16_t operation, uint32_t snaplen)
+ uint16_t operation, uint32_t snaplen,
+ uint32_t generation)
{
uint16_t qid;
@@ -324,13 +446,36 @@ pdump_register_tx_callbacks(enum pdump_version ver,
qid = (queue == RTE_PDUMP_ALL_QUEUES) ? 0 : queue;
for (; qid < end_q; qid++) {
struct pdump_rxtx_cbs *cbs = &tx_cbs[port][qid];
+ struct pdump_shared_ctl *ctl = &pdump_stats->tx_ctl[port][qid];
if (operation == ENABLE) {
+ int ret;
+
if (cbs->cb) {
- PDUMP_LOG_LINE(ERR,
- "tx callback for port=%d queue=%d, already exists",
- port, qid);
- return -EEXIST;
+ if (cbs->generation == generation &&
+ rte_atomic_load_explicit(&ctl->enabled,
+ rte_memory_order_relaxed) != 0) {
+ PDUMP_LOG_LINE(ERR,
+ "tx callback for port=%d queue=%d, already exists",
+ port, qid);
+ return -EEXIST;
+ }
+
+ PDUMP_LOG_LINE(DEBUG,
+ "reconciling stale tx callback for port=%d queue=%d"
+ " old_gen=%u new_gen=%u",
+ port, qid, cbs->generation, generation);
+
+ ret = rte_eth_remove_tx_callback(port, qid, cbs->cb);
+ if (ret < 0) {
+ PDUMP_LOG_LINE(ERR,
+ "failed to reconcile stale tx callback, errno=%d",
+ -ret);
+ return ret;
+ }
+
+ pdump_cb_wait(cbs);
+ cbs->cb = NULL;
}
cbs->use_count = 0;
cbs->ver = ver;
@@ -338,6 +483,7 @@ pdump_register_tx_callbacks(enum pdump_version ver,
cbs->mp = mp;
cbs->snaplen = snaplen;
cbs->filter = filter;
+ cbs->generation = generation;
cbs->cb = rte_eth_add_tx_callback(port, qid, pdump_tx,
cbs);
@@ -347,10 +493,15 @@ pdump_register_tx_callbacks(enum pdump_version ver,
rte_errno);
return rte_errno;
}
+
+ rte_atomic_store_explicit(&ctl->inflight, 0, rte_memory_order_relaxed);
+ rte_atomic_store_explicit(&ctl->enabled, 1, rte_memory_order_release);
memset(&pdump_stats->tx[port][qid], 0, sizeof(struct rte_pdump_stats));
} else if (operation == DISABLE) {
int ret;
+ rte_atomic_store_explicit(&ctl->enabled, 0, rte_memory_order_release);
+
if (cbs->cb == NULL) {
PDUMP_LOG_LINE(ERR,
"no existing tx callback for port=%d queue=%d",
@@ -359,6 +510,11 @@ pdump_register_tx_callbacks(enum pdump_version ver,
}
ret = rte_eth_remove_tx_callback(port, qid, cbs->cb);
if (ret < 0) {
+ /* Keep state coherent: callback is still registered,
+ * so restore enabled.
+ */
+ rte_atomic_store_explicit(&ctl->enabled, 1,
+ rte_memory_order_release);
PDUMP_LOG_LINE(ERR,
"failed to remove tx callback, errno=%d",
-ret);
@@ -367,6 +523,10 @@ pdump_register_tx_callbacks(enum pdump_version ver,
pdump_cb_wait(cbs);
cbs->cb = NULL;
+
+ ret = pdump_wait_inflight(ctl);
+ if (ret < 0)
+ return ret;
}
}
@@ -459,17 +619,23 @@ set_pdump_rxtx_cbs(const struct pdump_request *p)
end_q = (queue == RTE_PDUMP_ALL_QUEUES) ? nb_rx_q : queue + 1;
ret = pdump_register_rx_callbacks(p->ver, end_q, port, queue,
ring, mp, filter,
- operation, p->snaplen);
+ operation, p->snaplen,
+ p->generation);
if (ret < 0)
return ret;
}
+ /*
+ * Intentional partial-apply behavior: if RX succeeds and TX fails,
+ * RX remains enabled. API notes require caller-side unwind.
+ */
/* register TX callback */
if (flags & RTE_PDUMP_FLAG_TX) {
end_q = (queue == RTE_PDUMP_ALL_QUEUES) ? nb_tx_q : queue + 1;
ret = pdump_register_tx_callbacks(p->ver, end_q, port, queue,
ring, mp, filter,
- operation, p->snaplen);
+ operation, p->snaplen,
+ p->generation);
if (ret < 0)
return ret;
}
@@ -477,12 +643,14 @@ set_pdump_rxtx_cbs(const struct pdump_request *p)
return ret;
}
-static void
-pdump_request_to_secondary(const struct pdump_request *req)
+static int
+pdump_request_to_secondary_sync(const struct pdump_request *req)
{
struct rte_mp_msg mp_req = { };
struct rte_mp_reply mp_reply;
struct timespec ts = {.tv_sec = MP_TIMEOUT_S, .tv_nsec = 0};
+ int ret = 0;
+ uint16_t i;
PDUMP_LOG_LINE(DEBUG, "forward req %s to secondary", pdump_opname(req->op));
@@ -490,14 +658,45 @@ pdump_request_to_secondary(const struct pdump_request *req)
strlcpy(mp_req.name, PDUMP_MP, sizeof(mp_req.name));
mp_req.len_param = sizeof(*req);
- if (rte_mp_request_sync(&mp_req, &mp_reply, &ts) != 0)
- PDUMP_LOG_LINE(ERR, "rte_mp_request_sync failed");
+ if (rte_mp_request_sync_ex(&mp_req, &mp_reply, &ts,
+ RTE_MP_REQ_F_IGNORE_NO_ACTION) != 0) {
+ PDUMP_LOG_LINE(ERR, "rte_mp_request_sync failed: %s",
+ strerror(rte_errno));
+ return -rte_errno;
+ }
- else if (mp_reply.nb_sent != mp_reply.nb_received)
+ if (mp_reply.nb_sent != mp_reply.nb_received) {
PDUMP_LOG_LINE(ERR, "not all secondary's replied (sent %u recv %u)",
mp_reply.nb_sent, mp_reply.nb_received);
+ ret = -ETIMEDOUT;
+ }
+
+ for (i = 0; i < mp_reply.nb_received; i++) {
+ struct rte_mp_msg *mp_rep = &mp_reply.msgs[i];
+ const struct pdump_response *resp;
+
+ if (mp_rep->len_param != sizeof(*resp)) {
+ PDUMP_LOG_LINE(ERR, "invalid secondary reply size %u", mp_rep->len_param);
+ if (ret == 0)
+ ret = -EINVAL;
+ continue;
+ }
+
+ resp = (const struct pdump_response *)mp_rep->param;
+ if (resp->err_value != 0) {
+ PDUMP_LOG_LINE(ERR, "secondary reply failed: op=%u err=%d",
+ resp->res_op, resp->err_value);
+ /*
+ * Return the first failure to keep a single representative
+ * status; all per-secondary errors are still logged above.
+ */
+ if (ret == 0)
+ ret = resp->err_value;
+ }
+ }
free(mp_reply.msgs);
+ return ret;
}
/* Allocate temporary storage for passing state to the alarm thread for deferred handling */
@@ -556,7 +755,10 @@ pdump_handle_primary_request(const struct rte_mp_msg *mp_msg, const void *peer)
PDUMP_LOG_LINE(DEBUG, "secondary pdump %s", pdump_opname(req->op));
/* Can just do it now, no need for interrupt thread */
- ret = set_pdump_rxtx_cbs(req);
+ if (req->op == ENABLE || req->op == DISABLE)
+ ret = set_pdump_rxtx_cbs(req);
+ else
+ ret = -EINVAL;
}
return pdump_send_response(req, ret, peer);
@@ -569,17 +771,29 @@ __pdump_request(void *param)
{
struct pdump_bundle *bundle = param;
struct rte_mp_msg *msg = &bundle->msg;
- const struct pdump_request *req =
- (const struct pdump_request *)msg->param;
+ struct pdump_request *req =
+ (struct pdump_request *)msg->param;
int ret;
PDUMP_LOG_LINE(DEBUG, "primary pdump %s", pdump_opname(req->op));
+ if (req->op == DISABLE) {
+ req->generation = rte_atomic_fetch_add_explicit(&pdump_stats->generation, 1,
+ rte_memory_order_acq_rel) + 1;
+ } else if (req->op == ENABLE) {
+ req->generation = rte_atomic_load_explicit(&pdump_stats->generation,
+ rte_memory_order_acquire);
+ }
+
ret = set_pdump_rxtx_cbs(req);
- /* Primary process is responsible for broadcasting request to all secondaries */
+ /*
+ * Primary process is responsible for broadcasting the request to all
+ * secondaries. The sync request uses opt-in ignore-missing-action mode so
+ * pdump does not depend on unrelated secondary processes.
+ */
if (ret == 0)
- pdump_request_to_secondary(req);
+ ret = pdump_request_to_secondary_sync(req);
pdump_send_response(req, ret, bundle->peer);
free(bundle);
@@ -659,6 +873,10 @@ rte_pdump_init(void)
pdump_stats = mz->addr;
pdump_stats->mz = mz;
+ /* Start at generation 1 so zero remains the default pre-init value. */
+ if (rte_eal_process_type() == RTE_PROC_PRIMARY)
+ rte_atomic_store_explicit(&pdump_stats->generation, 1,
+ rte_memory_order_release);
return 0;
}
diff --git a/lib/pdump/rte_pdump.h b/lib/pdump/rte_pdump.h
index 1e32d46097..52d3c0ca93 100644
--- a/lib/pdump/rte_pdump.h
+++ b/lib/pdump/rte_pdump.h
@@ -64,7 +64,7 @@ rte_pdump_uninit(void);
* queues of a given port.
* @param flags
* flags specifies RTE_PDUMP_FLAG_RX/RTE_PDUMP_FLAG_TX/RTE_PDUMP_FLAG_RXTX
- * on which packet capturing should be enabled for a given port and queue.
+ * on which packet capturing should be disabled for a given port and queue.
* @param ring
* ring on which captured packets will be enqueued for user.
* @param mp
@@ -72,6 +72,11 @@ rte_pdump_uninit(void);
* @param filter
* Unused should be NULL.
*
+ * @note
+ * In applications that enable capture on multiple interfaces, enable may be
+ * partially applied before an error is returned. Callers should explicitly
+ * unwind partial enable state.
+ *
* @return
* 0 on success, -1 on error, rte_errno is set accordingly.
*/
@@ -103,6 +108,11 @@ rte_pdump_enable(uint16_t port, uint16_t queue, uint32_t flags,
* @param prm
* Use BPF program to run to filter packes (can be NULL)
*
+ * @note
+ * In applications that enable capture on multiple interfaces, enable may be
+ * partially applied before an error is returned. Callers should explicitly
+ * unwind partial enable state.
+ *
* @return
* 0 on success, -1 on error, rte_errno is set accordingly.
*/
@@ -124,7 +134,15 @@ rte_pdump_enable_bpf(uint16_t port_id, uint16_t queue,
* queues of a given port.
* @param flags
* flags specifies RTE_PDUMP_FLAG_RX/RTE_PDUMP_FLAG_TX/RTE_PDUMP_FLAG_RXTX
- * on which packet capturing should be enabled for a given port and queue.
+ * on which packet capturing should be disabled for a given port and queue.
+ *
+ * @note
+ * A disable failure (including timeout/no response) means teardown is not
+ * complete across pdump-enabled peers. The caller must not release shared
+ * pdump resources and must not uninitialize pdump for that capture session
+ * until disable succeeds. Releasing resources on disable failure can lead to
+ * crashes in peer processes still accessing those resources. Timeout is
+ * bounded; callers should retry disable before attempting teardown.
*
* @return
* 0 on success, -1 on error, rte_errno is set accordingly.
@@ -153,6 +171,11 @@ rte_pdump_disable(uint16_t port, uint16_t queue, uint32_t flags);
* @param filter
* unused should be NULL
*
+ * @note
+ * In applications that enable capture on multiple interfaces, enable may be
+ * partially applied before an error is returned. Callers should explicitly
+ * unwind partial enable state.
+ *
* @return
* 0 on success, -1 on error, rte_errno is set accordingly.
*/
@@ -186,6 +209,11 @@ rte_pdump_enable_by_deviceid(char *device_id, uint16_t queue,
* @param filter
* Use BPF program to run to filter packes (can be NULL)
*
+ * @note
+ * In applications that enable capture on multiple interfaces, enable may be
+ * partially applied before an error is returned. Callers should explicitly
+ * unwind partial enable state.
+ *
* @return
* 0 on success, -1 on error, rte_errno is set accordingly.
*/
@@ -210,7 +238,15 @@ rte_pdump_enable_bpf_by_deviceid(const char *device_id, uint16_t queue,
* queues of a given device id.
* @param flags
* flags specifies RTE_PDUMP_FLAG_RX/RTE_PDUMP_FLAG_TX/RTE_PDUMP_FLAG_RXTX
- * on which packet capturing should be enabled for a given port and queue.
+ * on which packet capturing should be disabled for a given port and queue.
+ *
+ * @note
+ * A disable failure (including timeout/no response) means teardown is not
+ * complete across pdump-enabled peers. The caller must not release shared
+ * pdump resources and must not uninitialize pdump for that capture session
+ * until disable succeeds. Releasing resources on disable failure can lead to
+ * crashes in peer processes still accessing those resources. Timeout is
+ * bounded; callers should retry disable before attempting teardown.
*
* @return
* 0 on success, -1 on error, rte_errno is set accordingly.
--
2.43.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2] pdump: fix teardown race with opt-in MP request mode
2026-07-05 8:44 ` [PATCH v2] pdump: fix teardown race with opt-in MP request mode Pushpendra Kumar
2026-07-05 17:07 ` [PATCH v3] " Pushpendra Kumar
@ 2026-07-05 18:27 ` Stephen Hemminger
2026-07-05 21:08 ` Kumar, Pushpendra1X
1 sibling, 1 reply; 8+ messages in thread
From: Stephen Hemminger @ 2026-07-05 18:27 UTC (permalink / raw)
To: Pushpendra Kumar
Cc: dev, stable, Thomas Monjalon, Reshma Pattan, Anatoly Burakov,
Dmitry Kozlyuk, Bruce Richardson, 14pwcse1224, pushpendra.kumar
[-- Attachment #1: Type: text/plain, Size: 41652 bytes --]
Root cause is not calling rte_pdump_init in secondary. This should be
detected or fixed automatically by EAL. Working around with async adds
unnecessary complexity
On Sun, Jul 5, 2026, 01:48 Pushpendra Kumar <pushpendra1x.kumar@intel.com>
wrote:
> This change addresses two pdump teardown failure cases:
>
> 1. A requester secondary can time out if a forwarded secondary is slow,
> unresponsive, or does not implement rte_pdump_init().
> 2. After a DISABLE request times out, the requester secondary receives
> the reply and releases shared resources, which other slow or
> unresponsive secondaries may access, leading to a crash.
>
> Add an opt-in EAL multiprocess request mode that allows a caller to
> ignore peers that do not implement the requested action, while preserving
> the existing rte_mp_request_sync() behavior for legacy users.
>
> pdump uses this new mode when forwarding enable/disable requests and adds
> the following behavior:
> - track request generation so stale callbacks cannot act on a newer
> enable/disable cycle
> - wait for in-flight callbacks to drain before completing disable
> - prevent shared resource teardown from racing with delayed secondary
> processing
> - keep pdump intent-driven without extra consumer-specific workaround
> logic
>
> The EAL change is required because the current mp request path treats
> a missing action as a failure, which cannot satisfy pdump's needs without
> changing behavior for all existing users. The new API is opt-in, so
> existing callers of rte_mp_request_sync() keep their current semantics.
>
> Validation:
> - Baseline-vs-changes benchmark used DPDK-native test-pmd as primary and
> dumpcap as secondary.
> - Both runs captured 1024 packets with the same packet-count exit
> condition.
> - A 5-second perf stat sample on the forwarding primary showed no
> meaningful regression: with changes recorded 12,402,055,320 cycles and
> 17,391,769,084 instructions, versus 12,450,031,900 cycles and
> 17,499,706,053 instructions on the parent baseline.
> - The extra synchronization in the hot path did not show material
> overhead in this sample.
>
> Fixes: c3ceb8742295 ("pdump: forward callback enable to secondary process")
> Signed-off-by: Pushpendra Kumar <pushpendra1x.kumar@intel.com>
>
> Cc: stable@dpdk.org
> ---
> This V2 patch addresses two pdump teardown failure scenarios:
>
> - A requester secondary can time out if a forwarded secondary is slow,
> unresponsive, or does not implement rte_pdump_init().
> - A delayed DISABLE reply can race with requester-side teardown, which may
> lead to crashes if peer processes still access shared pdump resources.
>
> To fix this end-to-end:
> - EAL adds an opt-in MP request mode to ignore peers with no matching
> action,
> while preserving legacy rte_mp_request_sync() behavior.
> - pdump uses that mode, tracks generation, and waits for in-flight
> callbacks
> to drain before completing disable.
> - dumpcap and pdump app cleanup paths are fail-closed when disable does not
> complete successfully.
>
> If this direction is accepted, I can split from the next revision into a
> focused EAL patch followed by pdump and app-user updates.
>
> .mailmap | 1 +
> app/dumpcap/main.c | 23 ++-
> app/pdump/main.c | 32 +++-
> doc/guides/rel_notes/release_26_07.rst | 7 +
> lib/eal/common/eal_common_proc.c | 37 +++-
> lib/eal/include/rte_eal.h | 33 ++++
> lib/eal/windows/eal_mp.c | 9 +
> lib/pdump/rte_pdump.c | 241 ++++++++++++++++++++++---
> lib/pdump/rte_pdump.h | 40 +++-
> 9 files changed, 379 insertions(+), 44 deletions(-)
>
> diff --git a/.mailmap b/.mailmap
> index c5bc728fae..c1313c7148 100644
> --- a/.mailmap
> +++ b/.mailmap
> @@ -1332,6 +1332,7 @@ Przemyslaw Gierszynski <
> przemyslaw.gierszynski@intel.com>
> Przemyslaw Patynowski <przemyslawx.patynowski@intel.com>
> Przemyslaw Zegan <przemyslawx.zegan@intel.com>
> Pu Xu <583493798@qq.com>
> +Pushpendra Kumar <pushpendra1x.kumar@intel.com>
> Qi Fu <qi.fu@intel.com>
> Qi Zhang <qi.z.zhang@intel.com>
> Qian Hao <qi_an_hao@126.com>
> diff --git a/app/dumpcap/main.c b/app/dumpcap/main.c
> index 46a6cb251e..f421a17034 100644
> --- a/app/dumpcap/main.c
> +++ b/app/dumpcap/main.c
> @@ -521,14 +521,33 @@ static void
> cleanup_pdump_resources(void)
> {
> struct interface *intf;
> + int ret;
> + bool disable_failed = false;
>
> TAILQ_FOREACH(intf, &interfaces, next) {
> - rte_pdump_disable(intf->port,
> - RTE_PDUMP_ALL_QUEUES,
> RTE_PDUMP_FLAG_RXTX);
> + ret = rte_pdump_disable(intf->port,
> + RTE_PDUMP_ALL_QUEUES,
> RTE_PDUMP_FLAG_RXTX);
> + if (ret < 0) {
> + disable_failed = true;
> + fprintf(stderr,
> + "Disable pdump failed on %u:%s: %s\n",
> + intf->port, intf->name,
> + rte_strerror(rte_errno));
> + }
> if (intf->opts.promisc_mode)
> rte_eth_promiscuous_disable(intf->port);
> }
>
> + if (disable_failed) {
> + /*
> + * Fail-closed: if disable did not complete, keep pdump
> action/state
> + * alive and do not uninit shared capture resources.
> + */
> + fprintf(stderr,
> + "Skipping pdump uninit because disable did not
> complete on all interfaces\n");
> + return;
> + }
> +
> rte_pdump_uninit();
> }
>
> diff --git a/app/pdump/main.c b/app/pdump/main.c
> index 1e62c8adc1..4cfb038395 100644
> --- a/app/pdump/main.c
> +++ b/app/pdump/main.c
> @@ -451,14 +451,17 @@ print_pdump_stats(void)
> }
> }
>
> -static inline void
> +static inline int
> disable_pdump(struct pdump_tuples *pt)
> {
> if (pt->dump_by_type == DEVICE_ID)
> - rte_pdump_disable_by_deviceid(pt->device_id, pt->queue,
> - pt->dir);
> + return rte_pdump_disable_by_deviceid(pt->device_id,
> pt->queue,
> + pt->dir);
> else if (pt->dump_by_type == PORT_ID)
> - rte_pdump_disable(pt->port, pt->queue, pt->dir);
> + return rte_pdump_disable(pt->port, pt->queue, pt->dir);
> +
> + rte_errno = EINVAL;
> + return -EINVAL;
> }
>
> static inline void
> @@ -518,15 +521,30 @@ static void
> cleanup_pdump_resources(void)
> {
> int i;
> + int ret;
> + bool disable_failed = false;
> struct pdump_tuples *pt;
> char name[RTE_ETH_NAME_MAX_LEN];
>
> - /* disable pdump and free the pdump_tuple resources */
> + /* Disable all callbacks first; freeing shared objects before this
> is unsafe. */
> for (i = 0; i < num_tuples; i++) {
> pt = &pdump_t[i];
>
> - /* remove callbacks */
> - disable_pdump(pt);
> + ret = disable_pdump(pt);
> + if (ret < 0) {
> + disable_failed = true;
> + printf("pdump disable failed (tuple=%d, errno=%d:
> %s); "
> + "skip teardown to avoid stale callback
> access\n",
> + i, rte_errno, rte_strerror(rte_errno));
> + }
> + }
> +
> + if (disable_failed)
> + return;
> +
> + /* free the pdump_tuple resources */
> + for (i = 0; i < num_tuples; i++) {
> + pt = &pdump_t[i];
>
> /*
> * transmit rest of the enqueued packets of the rings on to
> diff --git a/doc/guides/rel_notes/release_26_07.rst
> b/doc/guides/rel_notes/release_26_07.rst
> index 4ca0a9ac77..1700a56bc7 100644
> --- a/doc/guides/rel_notes/release_26_07.rst
> +++ b/doc/guides/rel_notes/release_26_07.rst
> @@ -177,6 +177,13 @@ New Features
> Added AGENTS.md file for AI review
> and supporting scripts to review patches and documentation.
>
> +* **Hardened pdump teardown on disable failure.**
> +
> + Hardened pdump request completion and application cleanup behavior so
> timeout
> + or disable failure does not trigger premature pdump resource teardown.
> + pdump now uses an opt-in MP request mode that ignores peers without the
> + pdump action, while preserving legacy EAL MP behavior for all other
> users.
> +
>
> Removed Items
> -------------
> diff --git a/lib/eal/common/eal_common_proc.c
> b/lib/eal/common/eal_common_proc.c
> index 06f151818c..5a458c0417 100644
> --- a/lib/eal/common/eal_common_proc.c
> +++ b/lib/eal/common/eal_common_proc.c
> @@ -56,6 +56,7 @@ static struct action_entry_list action_entry_list =
> enum mp_type {
> MP_MSG, /* Share message with peers, will not block */
> MP_REQ, /* Request for information, Will block for a reply */
> + MP_REQ_IGN, /* Request where missing action should return ignore */
> MP_REP, /* Response to previously-received request */
> MP_IGN, /* Response telling requester to ignore this response */
> };
> @@ -384,11 +385,11 @@ process_msg(struct mp_msg_internal *m, struct
> sockaddr_un *s)
> pthread_mutex_unlock(&mp_mutex_action);
>
> if (!action) {
> - if (m->type == MP_REQ && !internal_conf->init_complete) {
> - /* if this is a request, and init is not yet
> complete,
> - * and callback wasn't registered, we should tell
> the
> - * requester to ignore our existence because we're
> not
> - * yet ready to process this request.
> + if ((m->type == MP_REQ && !internal_conf->init_complete) ||
> + m->type == MP_REQ_IGN) {
> + /*
> + * Ask requester to ignore this peer when action
> is not
> + * registered either due to early init or explicit
> request policy.
> */
> struct rte_mp_msg dummy;
>
> @@ -936,7 +937,8 @@ mp_request_async(const char *dst, struct rte_mp_msg
> *req,
>
> static int
> mp_request_sync(const char *dst, struct rte_mp_msg *req,
> - struct rte_mp_reply *reply, const struct timespec *ts)
> + struct rte_mp_reply *reply, const struct timespec *ts,
> + enum mp_type req_type)
> {
> int ret;
> pthread_condattr_t attr;
> @@ -959,7 +961,7 @@ mp_request_sync(const char *dst, struct rte_mp_msg
> *req,
> return -1;
> }
>
> - ret = send_msg(dst, req, MP_REQ);
> + ret = send_msg(dst, req, req_type);
> if (ret < 0) {
> EAL_LOG(ERR, "Fail to send request %s:%s",
> dst, req->name);
> @@ -1010,11 +1012,20 @@ RTE_EXPORT_SYMBOL(rte_mp_request_sync)
> int
> rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
> const struct timespec *ts)
> +{
> + return rte_mp_request_sync_ex(req, reply, ts, 0);
> +}
> +
> +RTE_EXPORT_SYMBOL(rte_mp_request_sync_ex)
> +int
> +rte_mp_request_sync_ex(struct rte_mp_msg *req, struct rte_mp_reply *reply,
> + const struct timespec *ts, uint32_t flags)
> {
> int dir_fd, ret = -1;
> DIR *mp_dir;
> struct dirent *ent;
> struct timespec now, end;
> + enum mp_type req_type = MP_REQ;
> const struct internal_config *internal_conf =
> eal_get_internal_configuration();
>
> @@ -1027,6 +1038,14 @@ rte_mp_request_sync(struct rte_mp_msg *req, struct
> rte_mp_reply *reply,
> if (check_input(req) != 0)
> goto end;
>
> + if (flags & ~RTE_MP_REQ_F_IGNORE_NO_ACTION) {
> + rte_errno = EINVAL;
> + goto end;
> + }
> +
> + if (flags & RTE_MP_REQ_F_IGNORE_NO_ACTION)
> + req_type = MP_REQ_IGN;
> +
> if (internal_conf->no_shconf) {
> EAL_LOG(DEBUG, "No shared files mode enabled, IPC is
> disabled");
> rte_errno = ENOTSUP;
> @@ -1046,7 +1065,7 @@ rte_mp_request_sync(struct rte_mp_msg *req, struct
> rte_mp_reply *reply,
> /* for secondary process, send request to the primary process only
> */
> if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
> pthread_mutex_lock(&pending_requests.lock);
> - ret = mp_request_sync(eal_mp_socket_path(), req, reply,
> &end);
> + ret = mp_request_sync(eal_mp_socket_path(), req, reply,
> &end, req_type);
> pthread_mutex_unlock(&pending_requests.lock);
> goto end;
> }
> @@ -1086,7 +1105,7 @@ rte_mp_request_sync(struct rte_mp_msg *req, struct
> rte_mp_reply *reply,
> /* unlocks the mutex while waiting for response,
> * locks on receive
> */
> - if (mp_request_sync(path, req, reply, &end))
> + if (mp_request_sync(path, req, reply, &end, req_type))
> goto unlock_end;
> }
> ret = 0;
> diff --git a/lib/eal/include/rte_eal.h b/lib/eal/include/rte_eal.h
> index 7241f3be5d..415b974945 100644
> --- a/lib/eal/include/rte_eal.h
> +++ b/lib/eal/include/rte_eal.h
> @@ -173,6 +173,15 @@ struct rte_mp_reply {
> struct rte_mp_msg *msgs; /* caller to free */
> };
>
> +/** Request flags for rte_mp_request_sync_ex(). */
> +enum rte_mp_request_flags {
> + /**
> + * Ask peers that do not have a registered action to return MP_IGN
> + * instead of causing timeout for this request.
> + */
> + RTE_MP_REQ_F_IGNORE_NO_ACTION = 1u << 0,
> +};
> +
> /**
> * Action function typedef used by other components.
> *
> @@ -292,6 +301,30 @@ int
> rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
> const struct timespec *ts);
>
> +/**
> + * Send a request to peer processes with explicit request flags.
> + *
> + * This API is equivalent to rte_mp_request_sync() with opt-in behavior
> + * controls provided through @p flags.
> + *
> + * @param req
> + * The req argument contains the customized request message.
> + * @param reply
> + * The reply argument will be for storing all the replied messages;
> + * the caller is responsible for free reply->msgs.
> + * @param ts
> + * The ts argument specifies how long we can wait for the peer(s) to
> reply.
> + * @param flags
> + * Bitmask of values from enum rte_mp_request_flags.
> + *
> + * @return
> + * - On success, return 0.
> + * - On failure, return -1, and the reason will be stored in rte_errno.
> + */
> +int
> +rte_mp_request_sync_ex(struct rte_mp_msg *req, struct rte_mp_reply *reply,
> + const struct timespec *ts, uint32_t flags);
> +
> /**
> * Send a request to the peer process and expect a reply in a separate
> callback.
> *
> diff --git a/lib/eal/windows/eal_mp.c b/lib/eal/windows/eal_mp.c
> index 6703355318..1066a4d2bc 100644
> --- a/lib/eal/windows/eal_mp.c
> +++ b/lib/eal/windows/eal_mp.c
> @@ -56,10 +56,19 @@ RTE_EXPORT_SYMBOL(rte_mp_request_sync)
> int
> rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
> const struct timespec *ts)
> +{
> + return rte_mp_request_sync_ex(req, reply, ts, 0);
> +}
> +
> +RTE_EXPORT_SYMBOL(rte_mp_request_sync_ex)
> +int
> +rte_mp_request_sync_ex(struct rte_mp_msg *req, struct rte_mp_reply *reply,
> + const struct timespec *ts, uint32_t flags)
> {
> RTE_SET_USED(req);
> RTE_SET_USED(reply);
> RTE_SET_USED(ts);
> + RTE_SET_USED(flags);
> EAL_LOG_NOT_IMPLEMENTED();
> return -1;
> }
> diff --git a/lib/pdump/rte_pdump.c b/lib/pdump/rte_pdump.c
> index ac94efe7ff..3764aeb2d3 100644
> --- a/lib/pdump/rte_pdump.c
> +++ b/lib/pdump/rte_pdump.c
> @@ -3,9 +3,12 @@
> */
>
> #include <stdlib.h>
> +#include <errno.h>
>
> #include <eal_export.h>
> +#include <rte_eal.h>
> #include <rte_alarm.h>
> +#include <rte_cycles.h>
> #include <rte_mbuf.h>
> #include <rte_ethdev.h>
> #include <rte_lcore.h>
> @@ -68,6 +71,7 @@ struct pdump_request {
>
> const struct rte_bpf_prm *prm;
> uint32_t snaplen;
> + uint32_t generation;
> };
>
> struct pdump_response {
> @@ -81,6 +85,11 @@ struct pdump_bundle {
> char peer[];
> };
>
> +struct pdump_shared_ctl {
> + RTE_ATOMIC(uint32_t) enabled;
> + RTE_ATOMIC(uint32_t) inflight;
> +};
> +
> static struct pdump_rxtx_cbs {
> struct rte_ring *ring;
> struct rte_mempool *mp;
> @@ -88,11 +97,11 @@ static struct pdump_rxtx_cbs {
> const struct rte_bpf *filter;
> enum pdump_version ver;
> uint32_t snaplen;
> + uint32_t generation;
> RTE_ATOMIC(uint32_t) use_count;
> } rx_cbs[RTE_MAX_ETHPORTS][RTE_MAX_QUEUES_PER_PORT],
> tx_cbs[RTE_MAX_ETHPORTS][RTE_MAX_QUEUES_PER_PORT];
>
> -
> /*
> * The packet capture statistics keep track of packets
> * accepted, filtered and dropped. These are per-queue
> @@ -102,9 +111,59 @@ static const char MZ_RTE_PDUMP_STATS[] =
> "rte_pdump_stats";
> static struct {
> struct rte_pdump_stats
> rx[RTE_MAX_ETHPORTS][RTE_MAX_QUEUES_PER_PORT];
> struct rte_pdump_stats
> tx[RTE_MAX_ETHPORTS][RTE_MAX_QUEUES_PER_PORT];
> + struct pdump_shared_ctl
> rx_ctl[RTE_MAX_ETHPORTS][RTE_MAX_QUEUES_PER_PORT];
> + struct pdump_shared_ctl
> tx_ctl[RTE_MAX_ETHPORTS][RTE_MAX_QUEUES_PER_PORT];
> + RTE_ATOMIC(uint32_t) generation;
> const struct rte_memzone *mz;
> } *pdump_stats;
>
> +static int
> +pdump_enter(struct pdump_shared_ctl *ctl, const struct pdump_rxtx_cbs
> *cbs)
> +{
> + uint32_t generation =
> rte_atomic_load_explicit(&pdump_stats->generation,
> +
> rte_memory_order_relaxed);
> +
> + if (rte_atomic_load_explicit(&ctl->enabled,
> rte_memory_order_relaxed) == 0 ||
> + cbs->generation != generation)
> + return 0;
> +
> + rte_atomic_fetch_add_explicit(&ctl->inflight, 1,
> rte_memory_order_acquire);
> +
> + /*
> + * Disable/re-enable may race with callback entry. Re-check
> enabled and
> + * generation after taking inflight reference so disable can wait
> for this
> + * callback to drain and stale callbacks become no-op.
> + */
> + generation = rte_atomic_load_explicit(&pdump_stats->generation,
> + rte_memory_order_relaxed);
> + if (rte_atomic_load_explicit(&ctl->enabled,
> rte_memory_order_relaxed) != 0 &&
> + cbs->generation == generation)
> + return 1;
> +
> + rte_atomic_fetch_sub_explicit(&ctl->inflight, 1,
> rte_memory_order_release);
> + return 0;
> +}
> +
> +static __rte_always_inline void
> +pdump_exit(struct pdump_shared_ctl *ctl)
> +{
> + rte_atomic_fetch_sub_explicit(&ctl->inflight, 1,
> rte_memory_order_release);
> +}
> +
> +static int
> +pdump_wait_inflight(struct pdump_shared_ctl *ctl)
> +{
> + uint64_t end_tsc = rte_get_timer_cycles() + MP_TIMEOUT_S *
> rte_get_timer_hz();
> +
> + while (rte_atomic_load_explicit(&ctl->inflight,
> rte_memory_order_acquire) != 0) {
> + if (rte_get_timer_cycles() > end_tsc)
> + return -ETIMEDOUT;
> + rte_pause();
> + }
> +
> + return 0;
> +}
> +
> static void
> pdump_cb_wait(struct pdump_rxtx_cbs *cbs)
> {
> @@ -224,12 +283,17 @@ pdump_rx(uint16_t port, uint16_t queue,
> uint16_t max_pkts __rte_unused, void *user_params)
> {
> struct pdump_rxtx_cbs *cbs = user_params;
> + struct pdump_shared_ctl *ctl = &pdump_stats->rx_ctl[port][queue];
> struct rte_pdump_stats *stats = &pdump_stats->rx[port][queue];
>
> + if (!pdump_enter(ctl, cbs))
> + return nb_pkts;
> +
> pdump_cb_hold(cbs);
> pdump_copy(port, queue, RTE_PCAPNG_DIRECTION_IN,
> pkts, nb_pkts, cbs, stats);
> pdump_cb_release(cbs);
> + pdump_exit(ctl);
>
> return nb_pkts;
> }
> @@ -239,12 +303,17 @@ pdump_tx(uint16_t port, uint16_t queue,
> struct rte_mbuf **pkts, uint16_t nb_pkts, void
> *user_params)
> {
> struct pdump_rxtx_cbs *cbs = user_params;
> + struct pdump_shared_ctl *ctl = &pdump_stats->tx_ctl[port][queue];
> struct rte_pdump_stats *stats = &pdump_stats->tx[port][queue];
>
> + if (!pdump_enter(ctl, cbs))
> + return nb_pkts;
> +
> pdump_cb_hold(cbs);
> pdump_copy(port, queue, RTE_PCAPNG_DIRECTION_OUT,
> pkts, nb_pkts, cbs, stats);
> pdump_cb_release(cbs);
> + pdump_exit(ctl);
>
> return nb_pkts;
> }
> @@ -255,20 +324,43 @@ pdump_register_rx_callbacks(enum pdump_version ver,
> uint16_t end_q, uint16_t port, uint16_t queue,
> struct rte_ring *ring, struct rte_mempool *mp,
> struct rte_bpf *filter,
> - uint16_t operation, uint32_t snaplen)
> + uint16_t operation, uint32_t snaplen,
> + uint32_t generation)
> {
> uint16_t qid;
>
> qid = (queue == RTE_PDUMP_ALL_QUEUES) ? 0 : queue;
> for (; qid < end_q; qid++) {
> struct pdump_rxtx_cbs *cbs = &rx_cbs[port][qid];
> + struct pdump_shared_ctl *ctl =
> &pdump_stats->rx_ctl[port][qid];
>
> if (operation == ENABLE) {
> + int ret;
> +
> if (cbs->cb) {
> - PDUMP_LOG_LINE(ERR,
> - "rx callback for port=%d queue=%d,
> already exists",
> - port, qid);
> - return -EEXIST;
> + if (cbs->generation == generation &&
> + rte_atomic_load_explicit(&ctl->enabled,
> + rte_memory_order_relaxed) !=
> 0) {
> + PDUMP_LOG_LINE(ERR,
> + "rx callback for port=%d
> queue=%d, already exists",
> + port, qid);
> + return -EEXIST;
> + }
> +
> + PDUMP_LOG_LINE(DEBUG,
> + "reconciling stale rx callback for
> port=%d queue=%d"
> + " old_gen=%u new_gen=%u",
> + port, qid, cbs->generation,
> generation);
> +
> + ret = rte_eth_remove_rx_callback(port,
> qid, cbs->cb);
> + if (ret < 0) {
> + PDUMP_LOG_LINE(ERR,
> + "failed to reconcile stale
> rx callback, errno=%d",
> + -ret);
> + return ret;
> + }
> + pdump_cb_wait(cbs);
> + cbs->cb = NULL;
> }
> cbs->use_count = 0;
> cbs->ver = ver;
> @@ -276,6 +368,7 @@ pdump_register_rx_callbacks(enum pdump_version ver,
> cbs->mp = mp;
> cbs->snaplen = snaplen;
> cbs->filter = filter;
> + cbs->generation = generation;
>
> cbs->cb = rte_eth_add_first_rx_callback(port, qid,
> pdump_rx,
> cbs);
> @@ -286,10 +379,15 @@ pdump_register_rx_callbacks(enum pdump_version ver,
> return rte_errno;
> }
>
> + rte_atomic_store_explicit(&ctl->inflight, 0,
> rte_memory_order_relaxed);
> + rte_atomic_store_explicit(&ctl->enabled, 1,
> rte_memory_order_release);
> +
> memset(&pdump_stats->rx[port][qid], 0,
> sizeof(struct rte_pdump_stats));
> } else if (operation == DISABLE) {
> int ret;
>
> + rte_atomic_store_explicit(&ctl->enabled, 0,
> rte_memory_order_release);
> +
> if (cbs->cb == NULL) {
> PDUMP_LOG_LINE(ERR,
> "no existing rx callback for
> port=%d queue=%d",
> @@ -298,6 +396,11 @@ pdump_register_rx_callbacks(enum pdump_version ver,
> }
> ret = rte_eth_remove_rx_callback(port, qid,
> cbs->cb);
> if (ret < 0) {
> + /* Keep state coherent: callback is still
> registered,
> + * so restore enabled.
> + */
> + rte_atomic_store_explicit(&ctl->enabled, 1,
> + rte_memory_order_release);
> PDUMP_LOG_LINE(ERR,
> "failed to remove rx callback,
> errno=%d",
> -ret);
> @@ -305,6 +408,10 @@ pdump_register_rx_callbacks(enum pdump_version ver,
> }
> pdump_cb_wait(cbs);
> cbs->cb = NULL;
> +
> + ret = pdump_wait_inflight(ctl);
> + if (ret < 0)
> + return ret;
> }
> }
>
> @@ -316,7 +423,8 @@ pdump_register_tx_callbacks(enum pdump_version ver,
> uint16_t end_q, uint16_t port, uint16_t queue,
> struct rte_ring *ring, struct rte_mempool *mp,
> struct rte_bpf *filter,
> - uint16_t operation, uint32_t snaplen)
> + uint16_t operation, uint32_t snaplen,
> + uint32_t generation)
> {
>
> uint16_t qid;
> @@ -324,13 +432,36 @@ pdump_register_tx_callbacks(enum pdump_version ver,
> qid = (queue == RTE_PDUMP_ALL_QUEUES) ? 0 : queue;
> for (; qid < end_q; qid++) {
> struct pdump_rxtx_cbs *cbs = &tx_cbs[port][qid];
> + struct pdump_shared_ctl *ctl =
> &pdump_stats->tx_ctl[port][qid];
>
> if (operation == ENABLE) {
> + int ret;
> +
> if (cbs->cb) {
> - PDUMP_LOG_LINE(ERR,
> - "tx callback for port=%d queue=%d,
> already exists",
> - port, qid);
> - return -EEXIST;
> + if (cbs->generation == generation &&
> + rte_atomic_load_explicit(&ctl->enabled,
> + rte_memory_order_relaxed) !=
> 0) {
> + PDUMP_LOG_LINE(ERR,
> + "tx callback for port=%d
> queue=%d, already exists",
> + port, qid);
> + return -EEXIST;
> + }
> +
> + PDUMP_LOG_LINE(DEBUG,
> + "reconciling stale tx callback for
> port=%d queue=%d"
> + " old_gen=%u new_gen=%u",
> + port, qid, cbs->generation,
> generation);
> +
> + ret = rte_eth_remove_tx_callback(port,
> qid, cbs->cb);
> + if (ret < 0) {
> + PDUMP_LOG_LINE(ERR,
> + "failed to reconcile stale
> tx callback, errno=%d",
> + -ret);
> + return ret;
> + }
> +
> + pdump_cb_wait(cbs);
> + cbs->cb = NULL;
> }
> cbs->use_count = 0;
> cbs->ver = ver;
> @@ -338,6 +469,7 @@ pdump_register_tx_callbacks(enum pdump_version ver,
> cbs->mp = mp;
> cbs->snaplen = snaplen;
> cbs->filter = filter;
> + cbs->generation = generation;
>
> cbs->cb = rte_eth_add_tx_callback(port, qid,
> pdump_tx,
> cbs);
> @@ -347,10 +479,15 @@ pdump_register_tx_callbacks(enum pdump_version ver,
> rte_errno);
> return rte_errno;
> }
> +
> + rte_atomic_store_explicit(&ctl->inflight, 0,
> rte_memory_order_relaxed);
> + rte_atomic_store_explicit(&ctl->enabled, 1,
> rte_memory_order_release);
> memset(&pdump_stats->tx[port][qid], 0,
> sizeof(struct rte_pdump_stats));
> } else if (operation == DISABLE) {
> int ret;
>
> + rte_atomic_store_explicit(&ctl->enabled, 0,
> rte_memory_order_release);
> +
> if (cbs->cb == NULL) {
> PDUMP_LOG_LINE(ERR,
> "no existing tx callback for
> port=%d queue=%d",
> @@ -359,6 +496,11 @@ pdump_register_tx_callbacks(enum pdump_version ver,
> }
> ret = rte_eth_remove_tx_callback(port, qid,
> cbs->cb);
> if (ret < 0) {
> + /* Keep state coherent: callback is still
> registered,
> + * so restore enabled.
> + */
> + rte_atomic_store_explicit(&ctl->enabled, 1,
> + rte_memory_order_release);
> PDUMP_LOG_LINE(ERR,
> "failed to remove tx callback,
> errno=%d",
> -ret);
> @@ -367,6 +509,10 @@ pdump_register_tx_callbacks(enum pdump_version ver,
>
> pdump_cb_wait(cbs);
> cbs->cb = NULL;
> +
> + ret = pdump_wait_inflight(ctl);
> + if (ret < 0)
> + return ret;
> }
> }
>
> @@ -459,7 +605,8 @@ set_pdump_rxtx_cbs(const struct pdump_request *p)
> end_q = (queue == RTE_PDUMP_ALL_QUEUES) ? nb_rx_q : queue
> + 1;
> ret = pdump_register_rx_callbacks(p->ver, end_q, port,
> queue,
> ring, mp, filter,
> - operation, p->snaplen);
> + operation, p->snaplen,
> + p->generation);
> if (ret < 0)
> return ret;
> }
> @@ -469,7 +616,8 @@ set_pdump_rxtx_cbs(const struct pdump_request *p)
> end_q = (queue == RTE_PDUMP_ALL_QUEUES) ? nb_tx_q : queue
> + 1;
> ret = pdump_register_tx_callbacks(p->ver, end_q, port,
> queue,
> ring, mp, filter,
> - operation, p->snaplen);
> + operation, p->snaplen,
> + p->generation);
> if (ret < 0)
> return ret;
> }
> @@ -477,12 +625,14 @@ set_pdump_rxtx_cbs(const struct pdump_request *p)
> return ret;
> }
>
> -static void
> -pdump_request_to_secondary(const struct pdump_request *req)
> +static int
> +pdump_request_to_secondary_sync(const struct pdump_request *req)
> {
> struct rte_mp_msg mp_req = { };
> struct rte_mp_reply mp_reply;
> struct timespec ts = {.tv_sec = MP_TIMEOUT_S, .tv_nsec = 0};
> + int ret = 0;
> + uint16_t i;
>
> PDUMP_LOG_LINE(DEBUG, "forward req %s to secondary",
> pdump_opname(req->op));
>
> @@ -490,14 +640,41 @@ pdump_request_to_secondary(const struct
> pdump_request *req)
> strlcpy(mp_req.name, PDUMP_MP, sizeof(mp_req.name));
> mp_req.len_param = sizeof(*req);
>
> - if (rte_mp_request_sync(&mp_req, &mp_reply, &ts) != 0)
> - PDUMP_LOG_LINE(ERR, "rte_mp_request_sync failed");
> + if (rte_mp_request_sync_ex(&mp_req, &mp_reply, &ts,
> + RTE_MP_REQ_F_IGNORE_NO_ACTION) != 0) {
> + PDUMP_LOG_LINE(ERR, "rte_mp_request_sync failed: %s",
> + strerror(rte_errno));
> + return -rte_errno;
> + }
>
> - else if (mp_reply.nb_sent != mp_reply.nb_received)
> + if (mp_reply.nb_sent != mp_reply.nb_received) {
> PDUMP_LOG_LINE(ERR, "not all secondary's replied (sent %u
> recv %u)",
> mp_reply.nb_sent, mp_reply.nb_received);
> + ret = -ETIMEDOUT;
> + }
> +
> + for (i = 0; i < mp_reply.nb_received; i++) {
> + struct rte_mp_msg *mp_rep = &mp_reply.msgs[i];
> + const struct pdump_response *resp;
> +
> + if (mp_rep->len_param != sizeof(*resp)) {
> + PDUMP_LOG_LINE(ERR, "invalid secondary reply size
> %u", mp_rep->len_param);
> + if (ret == 0)
> + ret = -EINVAL;
> + continue;
> + }
> +
> + resp = (const struct pdump_response *)mp_rep->param;
> + if (resp->err_value != 0) {
> + PDUMP_LOG_LINE(ERR, "secondary reply failed: op=%u
> err=%d",
> + resp->res_op, resp->err_value);
> + if (ret == 0)
> + ret = resp->err_value;
> + }
> + }
>
> free(mp_reply.msgs);
> + return ret;
> }
>
> /* Allocate temporary storage for passing state to the alarm thread for
> deferred handling */
> @@ -556,7 +733,10 @@ pdump_handle_primary_request(const struct rte_mp_msg
> *mp_msg, const void *peer)
> PDUMP_LOG_LINE(DEBUG, "secondary pdump %s",
> pdump_opname(req->op));
>
> /* Can just do it now, no need for interrupt thread */
> - ret = set_pdump_rxtx_cbs(req);
> + if (req->op == ENABLE || req->op == DISABLE)
> + ret = set_pdump_rxtx_cbs(req);
> + else
> + ret = -EINVAL;
> }
>
> return pdump_send_response(req, ret, peer);
> @@ -569,17 +749,29 @@ __pdump_request(void *param)
> {
> struct pdump_bundle *bundle = param;
> struct rte_mp_msg *msg = &bundle->msg;
> - const struct pdump_request *req =
> - (const struct pdump_request *)msg->param;
> + struct pdump_request *req =
> + (struct pdump_request *)msg->param;
> int ret;
>
> PDUMP_LOG_LINE(DEBUG, "primary pdump %s", pdump_opname(req->op));
>
> + if (req->op == DISABLE) {
> + req->generation =
> rte_atomic_fetch_add_explicit(&pdump_stats->generation, 1,
> +
> rte_memory_order_acq_rel) + 1;
> + } else if (req->op == ENABLE) {
> + req->generation =
> rte_atomic_load_explicit(&pdump_stats->generation,
> +
> rte_memory_order_acquire);
> + }
> +
> ret = set_pdump_rxtx_cbs(req);
>
> - /* Primary process is responsible for broadcasting request to all
> secondaries */
> + /*
> + * Primary process is responsible for broadcasting the request to
> all
> + * secondaries. The sync request uses opt-in ignore-missing-action
> mode so
> + * pdump does not depend on unrelated secondary processes.
> + */
> if (ret == 0)
> - pdump_request_to_secondary(req);
> + ret = pdump_request_to_secondary_sync(req);
>
> pdump_send_response(req, ret, bundle->peer);
> free(bundle);
> @@ -659,6 +851,9 @@ rte_pdump_init(void)
>
> pdump_stats = mz->addr;
> pdump_stats->mz = mz;
> + if (rte_eal_process_type() == RTE_PROC_PRIMARY)
> + rte_atomic_store_explicit(&pdump_stats->generation, 1,
> + rte_memory_order_release);
>
> return 0;
> }
> diff --git a/lib/pdump/rte_pdump.h b/lib/pdump/rte_pdump.h
> index 1e32d46097..77e5914420 100644
> --- a/lib/pdump/rte_pdump.h
> +++ b/lib/pdump/rte_pdump.h
> @@ -64,7 +64,7 @@ rte_pdump_uninit(void);
> * queues of a given port.
> * @param flags
> * flags specifies
> RTE_PDUMP_FLAG_RX/RTE_PDUMP_FLAG_TX/RTE_PDUMP_FLAG_RXTX
> - * on which packet capturing should be enabled for a given port and
> queue.
> + * on which packet capturing should be disabled for a given port and
> queue.
> * @param ring
> * ring on which captured packets will be enqueued for user.
> * @param mp
> @@ -72,6 +72,11 @@ rte_pdump_uninit(void);
> * @param filter
> * Unused should be NULL.
> *
> + * @note
> + * In applications that enable capture on multiple interfaces, enable
> may be
> + * partially applied before an error is returned. Callers should
> explicitly
> + * unwind partial enable state.
> + *
> * @return
> * 0 on success, -1 on error, rte_errno is set accordingly.
> */
> @@ -103,6 +108,11 @@ rte_pdump_enable(uint16_t port, uint16_t queue,
> uint32_t flags,
> * @param prm
> * Use BPF program to run to filter packes (can be NULL)
> *
> + * @note
> + * In applications that enable capture on multiple interfaces, enable
> may be
> + * partially applied before an error is returned. Callers should
> explicitly
> + * unwind partial enable state.
> + *
> * @return
> * 0 on success, -1 on error, rte_errno is set accordingly.
> */
> @@ -124,7 +134,14 @@ rte_pdump_enable_bpf(uint16_t port_id, uint16_t queue,
> * queues of a given port.
> * @param flags
> * flags specifies
> RTE_PDUMP_FLAG_RX/RTE_PDUMP_FLAG_TX/RTE_PDUMP_FLAG_RXTX
> - * on which packet capturing should be enabled for a given port and
> queue.
> + * on which packet capturing should be disabled for a given port and
> queue.
> + *
> + * @note
> + * A disable failure (including timeout/no response) means teardown is
> not
> + * complete across pdump-enabled peers. The caller must not release
> shared
> + * pdump resources and must not uninitialize pdump for that capture
> session
> + * until disable succeeds. Releasing resources on disable failure can
> lead to
> + * crashes in peer processes still accessing those resources.
> *
> * @return
> * 0 on success, -1 on error, rte_errno is set accordingly.
> @@ -153,6 +170,11 @@ rte_pdump_disable(uint16_t port, uint16_t queue,
> uint32_t flags);
> * @param filter
> * unused should be NULL
> *
> + * @note
> + * In applications that enable capture on multiple interfaces, enable
> may be
> + * partially applied before an error is returned. Callers should
> explicitly
> + * unwind partial enable state.
> + *
> * @return
> * 0 on success, -1 on error, rte_errno is set accordingly.
> */
> @@ -186,6 +208,11 @@ rte_pdump_enable_by_deviceid(char *device_id,
> uint16_t queue,
> * @param filter
> * Use BPF program to run to filter packes (can be NULL)
> *
> + * @note
> + * In applications that enable capture on multiple interfaces, enable
> may be
> + * partially applied before an error is returned. Callers should
> explicitly
> + * unwind partial enable state.
> + *
> * @return
> * 0 on success, -1 on error, rte_errno is set accordingly.
> */
> @@ -210,7 +237,14 @@ rte_pdump_enable_bpf_by_deviceid(const char
> *device_id, uint16_t queue,
> * queues of a given device id.
> * @param flags
> * flags specifies
> RTE_PDUMP_FLAG_RX/RTE_PDUMP_FLAG_TX/RTE_PDUMP_FLAG_RXTX
> - * on which packet capturing should be enabled for a given port and
> queue.
> + * on which packet capturing should be disabled for a given port and
> queue.
> + *
> + * @note
> + * A disable failure (including timeout/no response) means teardown is
> not
> + * complete across pdump-enabled peers. The caller must not release
> shared
> + * pdump resources and must not uninitialize pdump for that capture
> session
> + * until disable succeeds. Releasing resources on disable failure can
> lead to
> + * crashes in peer processes still accessing those resources.
> *
> * @return
> * 0 on success, -1 on error, rte_errno is set accordingly.
> --
> 2.43.0
>
>
[-- Attachment #2: Type: text/html, Size: 51805 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH v2] pdump: fix teardown race with opt-in MP request mode
2026-07-05 18:27 ` [PATCH v2] " Stephen Hemminger
@ 2026-07-05 21:08 ` Kumar, Pushpendra1X
2026-07-06 8:25 ` Stephen Hemminger
0 siblings, 1 reply; 8+ messages in thread
From: Kumar, Pushpendra1X @ 2026-07-05 21:08 UTC (permalink / raw)
To: Stephen Hemminger
Cc: dev, stable, Thomas Monjalon, Pattan, Reshma, Burakov, Anatoly,
Dmitry Kozlyuk, Richardson, Bruce, 14pwcse1224@uetpeshawar.edu.pk,
pushpendra.kumar@tieto.com
[-- Attachment #1: Type: text/plain, Size: 42925 bytes --]
Hi Stephen,
Thanks for taking time to review.
Just to clarify, the async ENABLE/DISABLE approach was part of v1 and I
dropped that direction in v2.
The current proposal still uses synchronous request handling. The EAL
change is only intended to allow pdump to ignore peers that do not
register the pdump action, while preserving existing
rte_mp_request_sync() semantics for current users.
The generation tracking and inflight synchronization added in pdump are
there to prevent teardown races from delayed processing after a DISABLE
request, not to make enable/disable asynchronous. The intent is simply
to avoid releasing shared resources while another secondary may still be
processing a previously forwarded request.
I may be being overly cautious here, but I'd appreciate your thoughts on
whether you consider this protection unnecessary.
My concern is that the forwarding model currently assumes all
secondaries participate in pdump, while EAL does not enforce or track
that assumption.
On the other hand, if EAL were to handle this generically, it would need
a mechanism to determine which secondary processes have registered a
given action before forwarding requests. The proposed API remains
generic, while pdump is simply its first user.
Thanks,
Pushpendra
From: Stephen Hemminger <stephen@networkplumber.org>
Sent: Sunday, July 5, 2026 11:58 PM
To: Kumar, Pushpendra1X <pushpendra1x.kumar@intel.com>
Cc: dev <dev@dpdk.org>; stable <stable@dpdk.org>; Thomas Monjalon <thomas@monjalon.net>; Pattan, Reshma <reshma.pattan@intel.com>; Burakov, Anatoly <anatoly.burakov@intel.com>; Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>; Richardson, Bruce <bruce.richardson@intel.com>; 14pwcse1224@uetpeshawar.edu.pk; pushpendra.kumar@tieto.com
Subject: Re: [PATCH v2] pdump: fix teardown race with opt-in MP request mode
Root cause is not calling rte_pdump_init in secondary. This should be detected or fixed automatically by EAL. Working around with async adds unnecessary complexity
On Sun, Jul 5, 2026, 01:48 Pushpendra Kumar <pushpendra1x.kumar@intel.com<mailto:pushpendra1x.kumar@intel.com>> wrote:
This change addresses two pdump teardown failure cases:
1. A requester secondary can time out if a forwarded secondary is slow,
unresponsive, or does not implement rte_pdump_init().
2. After a DISABLE request times out, the requester secondary receives
the reply and releases shared resources, which other slow or
unresponsive secondaries may access, leading to a crash.
Add an opt-in EAL multiprocess request mode that allows a caller to
ignore peers that do not implement the requested action, while preserving
the existing rte_mp_request_sync() behavior for legacy users.
pdump uses this new mode when forwarding enable/disable requests and adds
the following behavior:
- track request generation so stale callbacks cannot act on a newer
enable/disable cycle
- wait for in-flight callbacks to drain before completing disable
- prevent shared resource teardown from racing with delayed secondary
processing
- keep pdump intent-driven without extra consumer-specific workaround
logic
The EAL change is required because the current mp request path treats
a missing action as a failure, which cannot satisfy pdump's needs without
changing behavior for all existing users. The new API is opt-in, so
existing callers of rte_mp_request_sync() keep their current semantics.
Validation:
- Baseline-vs-changes benchmark used DPDK-native test-pmd as primary and
dumpcap as secondary.
- Both runs captured 1024 packets with the same packet-count exit
condition.
- A 5-second perf stat sample on the forwarding primary showed no
meaningful regression: with changes recorded 12,402,055,320 cycles and
17,391,769,084 instructions, versus 12,450,031,900 cycles and
17,499,706,053 instructions on the parent baseline.
- The extra synchronization in the hot path did not show material
overhead in this sample.
Fixes: c3ceb8742295 ("pdump: forward callback enable to secondary process")
Signed-off-by: Pushpendra Kumar <pushpendra1x.kumar@intel.com<mailto:pushpendra1x.kumar@intel.com>>
Cc: stable@dpdk.org<mailto:stable@dpdk.org>
---
This V2 patch addresses two pdump teardown failure scenarios:
- A requester secondary can time out if a forwarded secondary is slow,
unresponsive, or does not implement rte_pdump_init().
- A delayed DISABLE reply can race with requester-side teardown, which may
lead to crashes if peer processes still access shared pdump resources.
To fix this end-to-end:
- EAL adds an opt-in MP request mode to ignore peers with no matching action,
while preserving legacy rte_mp_request_sync() behavior.
- pdump uses that mode, tracks generation, and waits for in-flight callbacks
to drain before completing disable.
- dumpcap and pdump app cleanup paths are fail-closed when disable does not
complete successfully.
If this direction is accepted, I can split from the next revision into a
focused EAL patch followed by pdump and app-user updates.
.mailmap | 1 +
app/dumpcap/main.c | 23 ++-
app/pdump/main.c | 32 +++-
doc/guides/rel_notes/release_26_07.rst | 7 +
lib/eal/common/eal_common_proc.c | 37 +++-
lib/eal/include/rte_eal.h | 33 ++++
lib/eal/windows/eal_mp.c | 9 +
lib/pdump/rte_pdump.c | 241 ++++++++++++++++++++++---
lib/pdump/rte_pdump.h | 40 +++-
9 files changed, 379 insertions(+), 44 deletions(-)
diff --git a/.mailmap b/.mailmap
index c5bc728fae..c1313c7148 100644
--- a/.mailmap
+++ b/.mailmap
@@ -1332,6 +1332,7 @@ Przemyslaw Gierszynski <przemyslaw.gierszynski@intel.com<mailto:przemyslaw.gierszynski@intel.com>>
Przemyslaw Patynowski <przemyslawx.patynowski@intel.com<mailto:przemyslawx.patynowski@intel.com>>
Przemyslaw Zegan <przemyslawx.zegan@intel.com<mailto:przemyslawx.zegan@intel.com>>
Pu Xu <583493798@qq.com<mailto:583493798@qq.com>>
+Pushpendra Kumar <pushpendra1x.kumar@intel.com<mailto:pushpendra1x.kumar@intel.com>>
Qi Fu <qi.fu@intel.com<mailto:qi.fu@intel.com>>
Qi Zhang <qi.z.zhang@intel.com<mailto:qi.z.zhang@intel.com>>
Qian Hao <qi_an_hao@126.com<mailto:qi_an_hao@126.com>>
diff --git a/app/dumpcap/main.c b/app/dumpcap/main.c
index 46a6cb251e..f421a17034 100644
--- a/app/dumpcap/main.c
+++ b/app/dumpcap/main.c
@@ -521,14 +521,33 @@ static void
cleanup_pdump_resources(void)
{
struct interface *intf;
+ int ret;
+ bool disable_failed = false;
TAILQ_FOREACH(intf, &interfaces, next) {
- rte_pdump_disable(intf->port,
- RTE_PDUMP_ALL_QUEUES, RTE_PDUMP_FLAG_RXTX);
+ ret = rte_pdump_disable(intf->port,
+ RTE_PDUMP_ALL_QUEUES, RTE_PDUMP_FLAG_RXTX);
+ if (ret < 0) {
+ disable_failed = true;
+ fprintf(stderr,
+ "Disable pdump failed on %u:%s: %s\n",
+ intf->port, intf->name,
+ rte_strerror(rte_errno));
+ }
if (intf->opts.promisc_mode)
rte_eth_promiscuous_disable(intf->port);
}
+ if (disable_failed) {
+ /*
+ * Fail-closed: if disable did not complete, keep pdump action/state
+ * alive and do not uninit shared capture resources.
+ */
+ fprintf(stderr,
+ "Skipping pdump uninit because disable did not complete on all interfaces\n");
+ return;
+ }
+
rte_pdump_uninit();
}
diff --git a/app/pdump/main.c b/app/pdump/main.c
index 1e62c8adc1..4cfb038395 100644
--- a/app/pdump/main.c
+++ b/app/pdump/main.c
@@ -451,14 +451,17 @@ print_pdump_stats(void)
}
}
-static inline void
+static inline int
disable_pdump(struct pdump_tuples *pt)
{
if (pt->dump_by_type == DEVICE_ID)
- rte_pdump_disable_by_deviceid(pt->device_id, pt->queue,
- pt->dir);
+ return rte_pdump_disable_by_deviceid(pt->device_id, pt->queue,
+ pt->dir);
else if (pt->dump_by_type == PORT_ID)
- rte_pdump_disable(pt->port, pt->queue, pt->dir);
+ return rte_pdump_disable(pt->port, pt->queue, pt->dir);
+
+ rte_errno = EINVAL;
+ return -EINVAL;
}
static inline void
@@ -518,15 +521,30 @@ static void
cleanup_pdump_resources(void)
{
int i;
+ int ret;
+ bool disable_failed = false;
struct pdump_tuples *pt;
char name[RTE_ETH_NAME_MAX_LEN];
- /* disable pdump and free the pdump_tuple resources */
+ /* Disable all callbacks first; freeing shared objects before this is unsafe. */
for (i = 0; i < num_tuples; i++) {
pt = &pdump_t[i];
- /* remove callbacks */
- disable_pdump(pt);
+ ret = disable_pdump(pt);
+ if (ret < 0) {
+ disable_failed = true;
+ printf("pdump disable failed (tuple=%d, errno=%d: %s); "
+ "skip teardown to avoid stale callback access\n",
+ i, rte_errno, rte_strerror(rte_errno));
+ }
+ }
+
+ if (disable_failed)
+ return;
+
+ /* free the pdump_tuple resources */
+ for (i = 0; i < num_tuples; i++) {
+ pt = &pdump_t[i];
/*
* transmit rest of the enqueued packets of the rings on to
diff --git a/doc/guides/rel_notes/release_26_07.rst b/doc/guides/rel_notes/release_26_07.rst
index 4ca0a9ac77..1700a56bc7 100644
--- a/doc/guides/rel_notes/release_26_07.rst
+++ b/doc/guides/rel_notes/release_26_07.rst
@@ -177,6 +177,13 @@ New Features
Added AGENTS.md file for AI review
and supporting scripts to review patches and documentation.
+* **Hardened pdump teardown on disable failure.**
+
+ Hardened pdump request completion and application cleanup behavior so timeout
+ or disable failure does not trigger premature pdump resource teardown.
+ pdump now uses an opt-in MP request mode that ignores peers without the
+ pdump action, while preserving legacy EAL MP behavior for all other users.
+
Removed Items
-------------
diff --git a/lib/eal/common/eal_common_proc.c b/lib/eal/common/eal_common_proc.c
index 06f151818c..5a458c0417 100644
--- a/lib/eal/common/eal_common_proc.c
+++ b/lib/eal/common/eal_common_proc.c
@@ -56,6 +56,7 @@ static struct action_entry_list action_entry_list =
enum mp_type {
MP_MSG, /* Share message with peers, will not block */
MP_REQ, /* Request for information, Will block for a reply */
+ MP_REQ_IGN, /* Request where missing action should return ignore */
MP_REP, /* Response to previously-received request */
MP_IGN, /* Response telling requester to ignore this response */
};
@@ -384,11 +385,11 @@ process_msg(struct mp_msg_internal *m, struct sockaddr_un *s)
pthread_mutex_unlock(&mp_mutex_action);
if (!action) {
- if (m->type == MP_REQ && !internal_conf->init_complete) {
- /* if this is a request, and init is not yet complete,
- * and callback wasn't registered, we should tell the
- * requester to ignore our existence because we're not
- * yet ready to process this request.
+ if ((m->type == MP_REQ && !internal_conf->init_complete) ||
+ m->type == MP_REQ_IGN) {
+ /*
+ * Ask requester to ignore this peer when action is not
+ * registered either due to early init or explicit request policy.
*/
struct rte_mp_msg dummy;
@@ -936,7 +937,8 @@ mp_request_async(const char *dst, struct rte_mp_msg *req,
static int
mp_request_sync(const char *dst, struct rte_mp_msg *req,
- struct rte_mp_reply *reply, const struct timespec *ts)
+ struct rte_mp_reply *reply, const struct timespec *ts,
+ enum mp_type req_type)
{
int ret;
pthread_condattr_t attr;
@@ -959,7 +961,7 @@ mp_request_sync(const char *dst, struct rte_mp_msg *req,
return -1;
}
- ret = send_msg(dst, req, MP_REQ);
+ ret = send_msg(dst, req, req_type);
if (ret < 0) {
EAL_LOG(ERR, "Fail to send request %s:%s",
dst, req->name);
@@ -1010,11 +1012,20 @@ RTE_EXPORT_SYMBOL(rte_mp_request_sync)
int
rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
const struct timespec *ts)
+{
+ return rte_mp_request_sync_ex(req, reply, ts, 0);
+}
+
+RTE_EXPORT_SYMBOL(rte_mp_request_sync_ex)
+int
+rte_mp_request_sync_ex(struct rte_mp_msg *req, struct rte_mp_reply *reply,
+ const struct timespec *ts, uint32_t flags)
{
int dir_fd, ret = -1;
DIR *mp_dir;
struct dirent *ent;
struct timespec now, end;
+ enum mp_type req_type = MP_REQ;
const struct internal_config *internal_conf =
eal_get_internal_configuration();
@@ -1027,6 +1038,14 @@ rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
if (check_input(req) != 0)
goto end;
+ if (flags & ~RTE_MP_REQ_F_IGNORE_NO_ACTION) {
+ rte_errno = EINVAL;
+ goto end;
+ }
+
+ if (flags & RTE_MP_REQ_F_IGNORE_NO_ACTION)
+ req_type = MP_REQ_IGN;
+
if (internal_conf->no_shconf) {
EAL_LOG(DEBUG, "No shared files mode enabled, IPC is disabled");
rte_errno = ENOTSUP;
@@ -1046,7 +1065,7 @@ rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
/* for secondary process, send request to the primary process only */
if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
pthread_mutex_lock(&pending_requests.lock);
- ret = mp_request_sync(eal_mp_socket_path(), req, reply, &end);
+ ret = mp_request_sync(eal_mp_socket_path(), req, reply, &end, req_type);
pthread_mutex_unlock(&pending_requests.lock);
goto end;
}
@@ -1086,7 +1105,7 @@ rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
/* unlocks the mutex while waiting for response,
* locks on receive
*/
- if (mp_request_sync(path, req, reply, &end))
+ if (mp_request_sync(path, req, reply, &end, req_type))
goto unlock_end;
}
ret = 0;
diff --git a/lib/eal/include/rte_eal.h b/lib/eal/include/rte_eal.h
index 7241f3be5d..415b974945 100644
--- a/lib/eal/include/rte_eal.h
+++ b/lib/eal/include/rte_eal.h
@@ -173,6 +173,15 @@ struct rte_mp_reply {
struct rte_mp_msg *msgs; /* caller to free */
};
+/** Request flags for rte_mp_request_sync_ex(). */
+enum rte_mp_request_flags {
+ /**
+ * Ask peers that do not have a registered action to return MP_IGN
+ * instead of causing timeout for this request.
+ */
+ RTE_MP_REQ_F_IGNORE_NO_ACTION = 1u << 0,
+};
+
/**
* Action function typedef used by other components.
*
@@ -292,6 +301,30 @@ int
rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
const struct timespec *ts);
+/**
+ * Send a request to peer processes with explicit request flags.
+ *
+ * This API is equivalent to rte_mp_request_sync() with opt-in behavior
+ * controls provided through @p flags.
+ *
+ * @param req
+ * The req argument contains the customized request message.
+ * @param reply
+ * The reply argument will be for storing all the replied messages;
+ * the caller is responsible for free reply->msgs.
+ * @param ts
+ * The ts argument specifies how long we can wait for the peer(s) to reply.
+ * @param flags
+ * Bitmask of values from enum rte_mp_request_flags.
+ *
+ * @return
+ * - On success, return 0.
+ * - On failure, return -1, and the reason will be stored in rte_errno.
+ */
+int
+rte_mp_request_sync_ex(struct rte_mp_msg *req, struct rte_mp_reply *reply,
+ const struct timespec *ts, uint32_t flags);
+
/**
* Send a request to the peer process and expect a reply in a separate callback.
*
diff --git a/lib/eal/windows/eal_mp.c b/lib/eal/windows/eal_mp.c
index 6703355318..1066a4d2bc 100644
--- a/lib/eal/windows/eal_mp.c
+++ b/lib/eal/windows/eal_mp.c
@@ -56,10 +56,19 @@ RTE_EXPORT_SYMBOL(rte_mp_request_sync)
int
rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
const struct timespec *ts)
+{
+ return rte_mp_request_sync_ex(req, reply, ts, 0);
+}
+
+RTE_EXPORT_SYMBOL(rte_mp_request_sync_ex)
+int
+rte_mp_request_sync_ex(struct rte_mp_msg *req, struct rte_mp_reply *reply,
+ const struct timespec *ts, uint32_t flags)
{
RTE_SET_USED(req);
RTE_SET_USED(reply);
RTE_SET_USED(ts);
+ RTE_SET_USED(flags);
EAL_LOG_NOT_IMPLEMENTED();
return -1;
}
diff --git a/lib/pdump/rte_pdump.c b/lib/pdump/rte_pdump.c
index ac94efe7ff..3764aeb2d3 100644
--- a/lib/pdump/rte_pdump.c
+++ b/lib/pdump/rte_pdump.c
@@ -3,9 +3,12 @@
*/
#include <stdlib.h>
+#include <errno.h>
#include <eal_export.h>
+#include <rte_eal.h>
#include <rte_alarm.h>
+#include <rte_cycles.h>
#include <rte_mbuf.h>
#include <rte_ethdev.h>
#include <rte_lcore.h>
@@ -68,6 +71,7 @@ struct pdump_request {
const struct rte_bpf_prm *prm;
uint32_t snaplen;
+ uint32_t generation;
};
struct pdump_response {
@@ -81,6 +85,11 @@ struct pdump_bundle {
char peer[];
};
+struct pdump_shared_ctl {
+ RTE_ATOMIC(uint32_t) enabled;
+ RTE_ATOMIC(uint32_t) inflight;
+};
+
static struct pdump_rxtx_cbs {
struct rte_ring *ring;
struct rte_mempool *mp;
@@ -88,11 +97,11 @@ static struct pdump_rxtx_cbs {
const struct rte_bpf *filter;
enum pdump_version ver;
uint32_t snaplen;
+ uint32_t generation;
RTE_ATOMIC(uint32_t) use_count;
} rx_cbs[RTE_MAX_ETHPORTS][RTE_MAX_QUEUES_PER_PORT],
tx_cbs[RTE_MAX_ETHPORTS][RTE_MAX_QUEUES_PER_PORT];
-
/*
* The packet capture statistics keep track of packets
* accepted, filtered and dropped. These are per-queue
@@ -102,9 +111,59 @@ static const char MZ_RTE_PDUMP_STATS[] = "rte_pdump_stats";
static struct {
struct rte_pdump_stats rx[RTE_MAX_ETHPORTS][RTE_MAX_QUEUES_PER_PORT];
struct rte_pdump_stats tx[RTE_MAX_ETHPORTS][RTE_MAX_QUEUES_PER_PORT];
+ struct pdump_shared_ctl rx_ctl[RTE_MAX_ETHPORTS][RTE_MAX_QUEUES_PER_PORT];
+ struct pdump_shared_ctl tx_ctl[RTE_MAX_ETHPORTS][RTE_MAX_QUEUES_PER_PORT];
+ RTE_ATOMIC(uint32_t) generation;
const struct rte_memzone *mz;
} *pdump_stats;
+static int
+pdump_enter(struct pdump_shared_ctl *ctl, const struct pdump_rxtx_cbs *cbs)
+{
+ uint32_t generation = rte_atomic_load_explicit(&pdump_stats->generation,
+ rte_memory_order_relaxed);
+
+ if (rte_atomic_load_explicit(&ctl->enabled, rte_memory_order_relaxed) == 0 ||
+ cbs->generation != generation)
+ return 0;
+
+ rte_atomic_fetch_add_explicit(&ctl->inflight, 1, rte_memory_order_acquire);
+
+ /*
+ * Disable/re-enable may race with callback entry. Re-check enabled and
+ * generation after taking inflight reference so disable can wait for this
+ * callback to drain and stale callbacks become no-op.
+ */
+ generation = rte_atomic_load_explicit(&pdump_stats->generation,
+ rte_memory_order_relaxed);
+ if (rte_atomic_load_explicit(&ctl->enabled, rte_memory_order_relaxed) != 0 &&
+ cbs->generation == generation)
+ return 1;
+
+ rte_atomic_fetch_sub_explicit(&ctl->inflight, 1, rte_memory_order_release);
+ return 0;
+}
+
+static __rte_always_inline void
+pdump_exit(struct pdump_shared_ctl *ctl)
+{
+ rte_atomic_fetch_sub_explicit(&ctl->inflight, 1, rte_memory_order_release);
+}
+
+static int
+pdump_wait_inflight(struct pdump_shared_ctl *ctl)
+{
+ uint64_t end_tsc = rte_get_timer_cycles() + MP_TIMEOUT_S * rte_get_timer_hz();
+
+ while (rte_atomic_load_explicit(&ctl->inflight, rte_memory_order_acquire) != 0) {
+ if (rte_get_timer_cycles() > end_tsc)
+ return -ETIMEDOUT;
+ rte_pause();
+ }
+
+ return 0;
+}
+
static void
pdump_cb_wait(struct pdump_rxtx_cbs *cbs)
{
@@ -224,12 +283,17 @@ pdump_rx(uint16_t port, uint16_t queue,
uint16_t max_pkts __rte_unused, void *user_params)
{
struct pdump_rxtx_cbs *cbs = user_params;
+ struct pdump_shared_ctl *ctl = &pdump_stats->rx_ctl[port][queue];
struct rte_pdump_stats *stats = &pdump_stats->rx[port][queue];
+ if (!pdump_enter(ctl, cbs))
+ return nb_pkts;
+
pdump_cb_hold(cbs);
pdump_copy(port, queue, RTE_PCAPNG_DIRECTION_IN,
pkts, nb_pkts, cbs, stats);
pdump_cb_release(cbs);
+ pdump_exit(ctl);
return nb_pkts;
}
@@ -239,12 +303,17 @@ pdump_tx(uint16_t port, uint16_t queue,
struct rte_mbuf **pkts, uint16_t nb_pkts, void *user_params)
{
struct pdump_rxtx_cbs *cbs = user_params;
+ struct pdump_shared_ctl *ctl = &pdump_stats->tx_ctl[port][queue];
struct rte_pdump_stats *stats = &pdump_stats->tx[port][queue];
+ if (!pdump_enter(ctl, cbs))
+ return nb_pkts;
+
pdump_cb_hold(cbs);
pdump_copy(port, queue, RTE_PCAPNG_DIRECTION_OUT,
pkts, nb_pkts, cbs, stats);
pdump_cb_release(cbs);
+ pdump_exit(ctl);
return nb_pkts;
}
@@ -255,20 +324,43 @@ pdump_register_rx_callbacks(enum pdump_version ver,
uint16_t end_q, uint16_t port, uint16_t queue,
struct rte_ring *ring, struct rte_mempool *mp,
struct rte_bpf *filter,
- uint16_t operation, uint32_t snaplen)
+ uint16_t operation, uint32_t snaplen,
+ uint32_t generation)
{
uint16_t qid;
qid = (queue == RTE_PDUMP_ALL_QUEUES) ? 0 : queue;
for (; qid < end_q; qid++) {
struct pdump_rxtx_cbs *cbs = &rx_cbs[port][qid];
+ struct pdump_shared_ctl *ctl = &pdump_stats->rx_ctl[port][qid];
if (operation == ENABLE) {
+ int ret;
+
if (cbs->cb) {
- PDUMP_LOG_LINE(ERR,
- "rx callback for port=%d queue=%d, already exists",
- port, qid);
- return -EEXIST;
+ if (cbs->generation == generation &&
+ rte_atomic_load_explicit(&ctl->enabled,
+ rte_memory_order_relaxed) != 0) {
+ PDUMP_LOG_LINE(ERR,
+ "rx callback for port=%d queue=%d, already exists",
+ port, qid);
+ return -EEXIST;
+ }
+
+ PDUMP_LOG_LINE(DEBUG,
+ "reconciling stale rx callback for port=%d queue=%d"
+ " old_gen=%u new_gen=%u",
+ port, qid, cbs->generation, generation);
+
+ ret = rte_eth_remove_rx_callback(port, qid, cbs->cb);
+ if (ret < 0) {
+ PDUMP_LOG_LINE(ERR,
+ "failed to reconcile stale rx callback, errno=%d",
+ -ret);
+ return ret;
+ }
+ pdump_cb_wait(cbs);
+ cbs->cb = NULL;
}
cbs->use_count = 0;
cbs->ver = ver;
@@ -276,6 +368,7 @@ pdump_register_rx_callbacks(enum pdump_version ver,
cbs->mp = mp;
cbs->snaplen = snaplen;
cbs->filter = filter;
+ cbs->generation = generation;
cbs->cb = rte_eth_add_first_rx_callback(port, qid,
pdump_rx, cbs);
@@ -286,10 +379,15 @@ pdump_register_rx_callbacks(enum pdump_version ver,
return rte_errno;
}
+ rte_atomic_store_explicit(&ctl->inflight, 0, rte_memory_order_relaxed);
+ rte_atomic_store_explicit(&ctl->enabled, 1, rte_memory_order_release);
+
memset(&pdump_stats->rx[port][qid], 0, sizeof(struct rte_pdump_stats));
} else if (operation == DISABLE) {
int ret;
+ rte_atomic_store_explicit(&ctl->enabled, 0, rte_memory_order_release);
+
if (cbs->cb == NULL) {
PDUMP_LOG_LINE(ERR,
"no existing rx callback for port=%d queue=%d",
@@ -298,6 +396,11 @@ pdump_register_rx_callbacks(enum pdump_version ver,
}
ret = rte_eth_remove_rx_callback(port, qid, cbs->cb);
if (ret < 0) {
+ /* Keep state coherent: callback is still registered,
+ * so restore enabled.
+ */
+ rte_atomic_store_explicit(&ctl->enabled, 1,
+ rte_memory_order_release);
PDUMP_LOG_LINE(ERR,
"failed to remove rx callback, errno=%d",
-ret);
@@ -305,6 +408,10 @@ pdump_register_rx_callbacks(enum pdump_version ver,
}
pdump_cb_wait(cbs);
cbs->cb = NULL;
+
+ ret = pdump_wait_inflight(ctl);
+ if (ret < 0)
+ return ret;
}
}
@@ -316,7 +423,8 @@ pdump_register_tx_callbacks(enum pdump_version ver,
uint16_t end_q, uint16_t port, uint16_t queue,
struct rte_ring *ring, struct rte_mempool *mp,
struct rte_bpf *filter,
- uint16_t operation, uint32_t snaplen)
+ uint16_t operation, uint32_t snaplen,
+ uint32_t generation)
{
uint16_t qid;
@@ -324,13 +432,36 @@ pdump_register_tx_callbacks(enum pdump_version ver,
qid = (queue == RTE_PDUMP_ALL_QUEUES) ? 0 : queue;
for (; qid < end_q; qid++) {
struct pdump_rxtx_cbs *cbs = &tx_cbs[port][qid];
+ struct pdump_shared_ctl *ctl = &pdump_stats->tx_ctl[port][qid];
if (operation == ENABLE) {
+ int ret;
+
if (cbs->cb) {
- PDUMP_LOG_LINE(ERR,
- "tx callback for port=%d queue=%d, already exists",
- port, qid);
- return -EEXIST;
+ if (cbs->generation == generation &&
+ rte_atomic_load_explicit(&ctl->enabled,
+ rte_memory_order_relaxed) != 0) {
+ PDUMP_LOG_LINE(ERR,
+ "tx callback for port=%d queue=%d, already exists",
+ port, qid);
+ return -EEXIST;
+ }
+
+ PDUMP_LOG_LINE(DEBUG,
+ "reconciling stale tx callback for port=%d queue=%d"
+ " old_gen=%u new_gen=%u",
+ port, qid, cbs->generation, generation);
+
+ ret = rte_eth_remove_tx_callback(port, qid, cbs->cb);
+ if (ret < 0) {
+ PDUMP_LOG_LINE(ERR,
+ "failed to reconcile stale tx callback, errno=%d",
+ -ret);
+ return ret;
+ }
+
+ pdump_cb_wait(cbs);
+ cbs->cb = NULL;
}
cbs->use_count = 0;
cbs->ver = ver;
@@ -338,6 +469,7 @@ pdump_register_tx_callbacks(enum pdump_version ver,
cbs->mp = mp;
cbs->snaplen = snaplen;
cbs->filter = filter;
+ cbs->generation = generation;
cbs->cb = rte_eth_add_tx_callback(port, qid, pdump_tx,
cbs);
@@ -347,10 +479,15 @@ pdump_register_tx_callbacks(enum pdump_version ver,
rte_errno);
return rte_errno;
}
+
+ rte_atomic_store_explicit(&ctl->inflight, 0, rte_memory_order_relaxed);
+ rte_atomic_store_explicit(&ctl->enabled, 1, rte_memory_order_release);
memset(&pdump_stats->tx[port][qid], 0, sizeof(struct rte_pdump_stats));
} else if (operation == DISABLE) {
int ret;
+ rte_atomic_store_explicit(&ctl->enabled, 0, rte_memory_order_release);
+
if (cbs->cb == NULL) {
PDUMP_LOG_LINE(ERR,
"no existing tx callback for port=%d queue=%d",
@@ -359,6 +496,11 @@ pdump_register_tx_callbacks(enum pdump_version ver,
}
ret = rte_eth_remove_tx_callback(port, qid, cbs->cb);
if (ret < 0) {
+ /* Keep state coherent: callback is still registered,
+ * so restore enabled.
+ */
+ rte_atomic_store_explicit(&ctl->enabled, 1,
+ rte_memory_order_release);
PDUMP_LOG_LINE(ERR,
"failed to remove tx callback, errno=%d",
-ret);
@@ -367,6 +509,10 @@ pdump_register_tx_callbacks(enum pdump_version ver,
pdump_cb_wait(cbs);
cbs->cb = NULL;
+
+ ret = pdump_wait_inflight(ctl);
+ if (ret < 0)
+ return ret;
}
}
@@ -459,7 +605,8 @@ set_pdump_rxtx_cbs(const struct pdump_request *p)
end_q = (queue == RTE_PDUMP_ALL_QUEUES) ? nb_rx_q : queue + 1;
ret = pdump_register_rx_callbacks(p->ver, end_q, port, queue,
ring, mp, filter,
- operation, p->snaplen);
+ operation, p->snaplen,
+ p->generation);
if (ret < 0)
return ret;
}
@@ -469,7 +616,8 @@ set_pdump_rxtx_cbs(const struct pdump_request *p)
end_q = (queue == RTE_PDUMP_ALL_QUEUES) ? nb_tx_q : queue + 1;
ret = pdump_register_tx_callbacks(p->ver, end_q, port, queue,
ring, mp, filter,
- operation, p->snaplen);
+ operation, p->snaplen,
+ p->generation);
if (ret < 0)
return ret;
}
@@ -477,12 +625,14 @@ set_pdump_rxtx_cbs(const struct pdump_request *p)
return ret;
}
-static void
-pdump_request_to_secondary(const struct pdump_request *req)
+static int
+pdump_request_to_secondary_sync(const struct pdump_request *req)
{
struct rte_mp_msg mp_req = { };
struct rte_mp_reply mp_reply;
struct timespec ts = {.tv_sec = MP_TIMEOUT_S, .tv_nsec = 0};
+ int ret = 0;
+ uint16_t i;
PDUMP_LOG_LINE(DEBUG, "forward req %s to secondary", pdump_opname(req->op));
@@ -490,14 +640,41 @@ pdump_request_to_secondary(const struct pdump_request *req)
strlcpy(mp_req.name<http://mp_req.name>, PDUMP_MP, sizeof(mp_req.name<http://mp_req.name>));
mp_req.len_param = sizeof(*req);
- if (rte_mp_request_sync(&mp_req, &mp_reply, &ts) != 0)
- PDUMP_LOG_LINE(ERR, "rte_mp_request_sync failed");
+ if (rte_mp_request_sync_ex(&mp_req, &mp_reply, &ts,
+ RTE_MP_REQ_F_IGNORE_NO_ACTION) != 0) {
+ PDUMP_LOG_LINE(ERR, "rte_mp_request_sync failed: %s",
+ strerror(rte_errno));
+ return -rte_errno;
+ }
- else if (mp_reply.nb_sent != mp_reply.nb_received)
+ if (mp_reply.nb_sent != mp_reply.nb_received) {
PDUMP_LOG_LINE(ERR, "not all secondary's replied (sent %u recv %u)",
mp_reply.nb_sent, mp_reply.nb_received);
+ ret = -ETIMEDOUT;
+ }
+
+ for (i = 0; i < mp_reply.nb_received; i++) {
+ struct rte_mp_msg *mp_rep = &mp_reply.msgs[i];
+ const struct pdump_response *resp;
+
+ if (mp_rep->len_param != sizeof(*resp)) {
+ PDUMP_LOG_LINE(ERR, "invalid secondary reply size %u", mp_rep->len_param);
+ if (ret == 0)
+ ret = -EINVAL;
+ continue;
+ }
+
+ resp = (const struct pdump_response *)mp_rep->param;
+ if (resp->err_value != 0) {
+ PDUMP_LOG_LINE(ERR, "secondary reply failed: op=%u err=%d",
+ resp->res_op, resp->err_value);
+ if (ret == 0)
+ ret = resp->err_value;
+ }
+ }
free(mp_reply.msgs);
+ return ret;
}
/* Allocate temporary storage for passing state to the alarm thread for deferred handling */
@@ -556,7 +733,10 @@ pdump_handle_primary_request(const struct rte_mp_msg *mp_msg, const void *peer)
PDUMP_LOG_LINE(DEBUG, "secondary pdump %s", pdump_opname(req->op));
/* Can just do it now, no need for interrupt thread */
- ret = set_pdump_rxtx_cbs(req);
+ if (req->op == ENABLE || req->op == DISABLE)
+ ret = set_pdump_rxtx_cbs(req);
+ else
+ ret = -EINVAL;
}
return pdump_send_response(req, ret, peer);
@@ -569,17 +749,29 @@ __pdump_request(void *param)
{
struct pdump_bundle *bundle = param;
struct rte_mp_msg *msg = &bundle->msg;
- const struct pdump_request *req =
- (const struct pdump_request *)msg->param;
+ struct pdump_request *req =
+ (struct pdump_request *)msg->param;
int ret;
PDUMP_LOG_LINE(DEBUG, "primary pdump %s", pdump_opname(req->op));
+ if (req->op == DISABLE) {
+ req->generation = rte_atomic_fetch_add_explicit(&pdump_stats->generation, 1,
+ rte_memory_order_acq_rel) + 1;
+ } else if (req->op == ENABLE) {
+ req->generation = rte_atomic_load_explicit(&pdump_stats->generation,
+ rte_memory_order_acquire);
+ }
+
ret = set_pdump_rxtx_cbs(req);
- /* Primary process is responsible for broadcasting request to all secondaries */
+ /*
+ * Primary process is responsible for broadcasting the request to all
+ * secondaries. The sync request uses opt-in ignore-missing-action mode so
+ * pdump does not depend on unrelated secondary processes.
+ */
if (ret == 0)
- pdump_request_to_secondary(req);
+ ret = pdump_request_to_secondary_sync(req);
pdump_send_response(req, ret, bundle->peer);
free(bundle);
@@ -659,6 +851,9 @@ rte_pdump_init(void)
pdump_stats = mz->addr;
pdump_stats->mz = mz;
+ if (rte_eal_process_type() == RTE_PROC_PRIMARY)
+ rte_atomic_store_explicit(&pdump_stats->generation, 1,
+ rte_memory_order_release);
return 0;
}
diff --git a/lib/pdump/rte_pdump.h b/lib/pdump/rte_pdump.h
index 1e32d46097..77e5914420 100644
--- a/lib/pdump/rte_pdump.h
+++ b/lib/pdump/rte_pdump.h
@@ -64,7 +64,7 @@ rte_pdump_uninit(void);
* queues of a given port.
* @param flags
* flags specifies RTE_PDUMP_FLAG_RX/RTE_PDUMP_FLAG_TX/RTE_PDUMP_FLAG_RXTX
- * on which packet capturing should be enabled for a given port and queue.
+ * on which packet capturing should be disabled for a given port and queue.
* @param ring
* ring on which captured packets will be enqueued for user.
* @param mp
@@ -72,6 +72,11 @@ rte_pdump_uninit(void);
* @param filter
* Unused should be NULL.
*
+ * @note
+ * In applications that enable capture on multiple interfaces, enable may be
+ * partially applied before an error is returned. Callers should explicitly
+ * unwind partial enable state.
+ *
* @return
* 0 on success, -1 on error, rte_errno is set accordingly.
*/
@@ -103,6 +108,11 @@ rte_pdump_enable(uint16_t port, uint16_t queue, uint32_t flags,
* @param prm
* Use BPF program to run to filter packes (can be NULL)
*
+ * @note
+ * In applications that enable capture on multiple interfaces, enable may be
+ * partially applied before an error is returned. Callers should explicitly
+ * unwind partial enable state.
+ *
* @return
* 0 on success, -1 on error, rte_errno is set accordingly.
*/
@@ -124,7 +134,14 @@ rte_pdump_enable_bpf(uint16_t port_id, uint16_t queue,
* queues of a given port.
* @param flags
* flags specifies RTE_PDUMP_FLAG_RX/RTE_PDUMP_FLAG_TX/RTE_PDUMP_FLAG_RXTX
- * on which packet capturing should be enabled for a given port and queue.
+ * on which packet capturing should be disabled for a given port and queue.
+ *
+ * @note
+ * A disable failure (including timeout/no response) means teardown is not
+ * complete across pdump-enabled peers. The caller must not release shared
+ * pdump resources and must not uninitialize pdump for that capture session
+ * until disable succeeds. Releasing resources on disable failure can lead to
+ * crashes in peer processes still accessing those resources.
*
* @return
* 0 on success, -1 on error, rte_errno is set accordingly.
@@ -153,6 +170,11 @@ rte_pdump_disable(uint16_t port, uint16_t queue, uint32_t flags);
* @param filter
* unused should be NULL
*
+ * @note
+ * In applications that enable capture on multiple interfaces, enable may be
+ * partially applied before an error is returned. Callers should explicitly
+ * unwind partial enable state.
+ *
* @return
* 0 on success, -1 on error, rte_errno is set accordingly.
*/
@@ -186,6 +208,11 @@ rte_pdump_enable_by_deviceid(char *device_id, uint16_t queue,
* @param filter
* Use BPF program to run to filter packes (can be NULL)
*
+ * @note
+ * In applications that enable capture on multiple interfaces, enable may be
+ * partially applied before an error is returned. Callers should explicitly
+ * unwind partial enable state.
+ *
* @return
* 0 on success, -1 on error, rte_errno is set accordingly.
*/
@@ -210,7 +237,14 @@ rte_pdump_enable_bpf_by_deviceid(const char *device_id, uint16_t queue,
* queues of a given device id.
* @param flags
* flags specifies RTE_PDUMP_FLAG_RX/RTE_PDUMP_FLAG_TX/RTE_PDUMP_FLAG_RXTX
- * on which packet capturing should be enabled for a given port and queue.
+ * on which packet capturing should be disabled for a given port and queue.
+ *
+ * @note
+ * A disable failure (including timeout/no response) means teardown is not
+ * complete across pdump-enabled peers. The caller must not release shared
+ * pdump resources and must not uninitialize pdump for that capture session
+ * until disable succeeds. Releasing resources on disable failure can lead to
+ * crashes in peer processes still accessing those resources.
*
* @return
* 0 on success, -1 on error, rte_errno is set accordingly.
--
2.43.0
[-- Attachment #2: Type: text/html, Size: 78222 bytes --]
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2] pdump: fix teardown race with opt-in MP request mode
2026-07-05 21:08 ` Kumar, Pushpendra1X
@ 2026-07-06 8:25 ` Stephen Hemminger
0 siblings, 0 replies; 8+ messages in thread
From: Stephen Hemminger @ 2026-07-06 8:25 UTC (permalink / raw)
To: Kumar, Pushpendra1X
Cc: dev, stable, Thomas Monjalon, Pattan, Reshma, Burakov, Anatoly,
Dmitry Kozlyuk, Richardson, Bruce, 14pwcse1224, pushpendra.kumar
[-- Attachment #1: Type: text/plain, Size: 43837 bytes --]
Not registering pdump in secondary means there is no capture in secondary.
Still think that is misuse
On Sun, Jul 5, 2026, 23:08 Kumar, Pushpendra1X <pushpendra1x.kumar@intel.com>
wrote:
> Hi Stephen,
>
>
>
> Thanks for taking time to review.
>
>
>
> Just to clarify, the async ENABLE/DISABLE approach was part of v1 and I
>
> dropped that direction in v2.
>
>
>
> The current proposal still uses synchronous request handling. The EAL
>
> change is only intended to allow pdump to ignore peers that do not
>
> register the pdump action, while preserving existing
>
> rte_mp_request_sync() semantics for current users.
>
>
>
> The generation tracking and inflight synchronization added in pdump are
>
> there to prevent teardown races from delayed processing after a DISABLE
>
> request, not to make enable/disable asynchronous. The intent is simply
>
> to avoid releasing shared resources while another secondary may still be
>
> processing a previously forwarded request.
>
> I may be being overly cautious here, but I'd appreciate your thoughts on
>
> whether you consider this protection unnecessary.
>
>
>
> My concern is that the forwarding model currently assumes all
>
> secondaries participate in pdump, while EAL does not enforce or track
>
> that assumption.
>
>
>
> On the other hand, if EAL were to handle this generically, it would need
>
> a mechanism to determine which secondary processes have registered a
>
> given action before forwarding requests. The proposed API remains
>
> generic, while pdump is simply its first user.
>
>
>
> Thanks,
>
> Pushpendra
>
>
>
> *From:* Stephen Hemminger <stephen@networkplumber.org>
> *Sent:* Sunday, July 5, 2026 11:58 PM
> *To:* Kumar, Pushpendra1X <pushpendra1x.kumar@intel.com>
> *Cc:* dev <dev@dpdk.org>; stable <stable@dpdk.org>; Thomas Monjalon <
> thomas@monjalon.net>; Pattan, Reshma <reshma.pattan@intel.com>; Burakov,
> Anatoly <anatoly.burakov@intel.com>; Dmitry Kozlyuk <
> dmitry.kozliuk@gmail.com>; Richardson, Bruce <bruce.richardson@intel.com>;
> 14pwcse1224@uetpeshawar.edu.pk; pushpendra.kumar@tieto.com
> *Subject:* Re: [PATCH v2] pdump: fix teardown race with opt-in MP request
> mode
>
>
>
> Root cause is not calling rte_pdump_init in secondary. This should be
> detected or fixed automatically by EAL. Working around with async adds
> unnecessary complexity
>
>
>
> On Sun, Jul 5, 2026, 01:48 Pushpendra Kumar <pushpendra1x.kumar@intel.com>
> wrote:
>
> This change addresses two pdump teardown failure cases:
>
> 1. A requester secondary can time out if a forwarded secondary is slow,
> unresponsive, or does not implement rte_pdump_init().
> 2. After a DISABLE request times out, the requester secondary receives
> the reply and releases shared resources, which other slow or
> unresponsive secondaries may access, leading to a crash.
>
> Add an opt-in EAL multiprocess request mode that allows a caller to
> ignore peers that do not implement the requested action, while preserving
> the existing rte_mp_request_sync() behavior for legacy users.
>
> pdump uses this new mode when forwarding enable/disable requests and adds
> the following behavior:
> - track request generation so stale callbacks cannot act on a newer
> enable/disable cycle
> - wait for in-flight callbacks to drain before completing disable
> - prevent shared resource teardown from racing with delayed secondary
> processing
> - keep pdump intent-driven without extra consumer-specific workaround
> logic
>
> The EAL change is required because the current mp request path treats
> a missing action as a failure, which cannot satisfy pdump's needs without
> changing behavior for all existing users. The new API is opt-in, so
> existing callers of rte_mp_request_sync() keep their current semantics.
>
> Validation:
> - Baseline-vs-changes benchmark used DPDK-native test-pmd as primary and
> dumpcap as secondary.
> - Both runs captured 1024 packets with the same packet-count exit
> condition.
> - A 5-second perf stat sample on the forwarding primary showed no
> meaningful regression: with changes recorded 12,402,055,320 cycles and
> 17,391,769,084 instructions, versus 12,450,031,900 cycles and
> 17,499,706,053 instructions on the parent baseline.
> - The extra synchronization in the hot path did not show material
> overhead in this sample.
>
> Fixes: c3ceb8742295 ("pdump: forward callback enable to secondary process")
> Signed-off-by: Pushpendra Kumar <pushpendra1x.kumar@intel.com>
>
> Cc: stable@dpdk.org
> ---
> This V2 patch addresses two pdump teardown failure scenarios:
>
> - A requester secondary can time out if a forwarded secondary is slow,
> unresponsive, or does not implement rte_pdump_init().
> - A delayed DISABLE reply can race with requester-side teardown, which may
> lead to crashes if peer processes still access shared pdump resources.
>
> To fix this end-to-end:
> - EAL adds an opt-in MP request mode to ignore peers with no matching
> action,
> while preserving legacy rte_mp_request_sync() behavior.
> - pdump uses that mode, tracks generation, and waits for in-flight
> callbacks
> to drain before completing disable.
> - dumpcap and pdump app cleanup paths are fail-closed when disable does not
> complete successfully.
>
> If this direction is accepted, I can split from the next revision into a
> focused EAL patch followed by pdump and app-user updates.
>
> .mailmap | 1 +
> app/dumpcap/main.c | 23 ++-
> app/pdump/main.c | 32 +++-
> doc/guides/rel_notes/release_26_07.rst | 7 +
> lib/eal/common/eal_common_proc.c | 37 +++-
> lib/eal/include/rte_eal.h | 33 ++++
> lib/eal/windows/eal_mp.c | 9 +
> lib/pdump/rte_pdump.c | 241 ++++++++++++++++++++++---
> lib/pdump/rte_pdump.h | 40 +++-
> 9 files changed, 379 insertions(+), 44 deletions(-)
>
> diff --git a/.mailmap b/.mailmap
> index c5bc728fae..c1313c7148 100644
> --- a/.mailmap
> +++ b/.mailmap
> @@ -1332,6 +1332,7 @@ Przemyslaw Gierszynski <
> przemyslaw.gierszynski@intel.com>
> Przemyslaw Patynowski <przemyslawx.patynowski@intel.com>
> Przemyslaw Zegan <przemyslawx.zegan@intel.com>
> Pu Xu <583493798@qq.com>
> +Pushpendra Kumar <pushpendra1x.kumar@intel.com>
> Qi Fu <qi.fu@intel.com>
> Qi Zhang <qi.z.zhang@intel.com>
> Qian Hao <qi_an_hao@126.com>
> diff --git a/app/dumpcap/main.c b/app/dumpcap/main.c
> index 46a6cb251e..f421a17034 100644
> --- a/app/dumpcap/main.c
> +++ b/app/dumpcap/main.c
> @@ -521,14 +521,33 @@ static void
> cleanup_pdump_resources(void)
> {
> struct interface *intf;
> + int ret;
> + bool disable_failed = false;
>
> TAILQ_FOREACH(intf, &interfaces, next) {
> - rte_pdump_disable(intf->port,
> - RTE_PDUMP_ALL_QUEUES,
> RTE_PDUMP_FLAG_RXTX);
> + ret = rte_pdump_disable(intf->port,
> + RTE_PDUMP_ALL_QUEUES,
> RTE_PDUMP_FLAG_RXTX);
> + if (ret < 0) {
> + disable_failed = true;
> + fprintf(stderr,
> + "Disable pdump failed on %u:%s: %s\n",
> + intf->port, intf->name,
> + rte_strerror(rte_errno));
> + }
> if (intf->opts.promisc_mode)
> rte_eth_promiscuous_disable(intf->port);
> }
>
> + if (disable_failed) {
> + /*
> + * Fail-closed: if disable did not complete, keep pdump
> action/state
> + * alive and do not uninit shared capture resources.
> + */
> + fprintf(stderr,
> + "Skipping pdump uninit because disable did not
> complete on all interfaces\n");
> + return;
> + }
> +
> rte_pdump_uninit();
> }
>
> diff --git a/app/pdump/main.c b/app/pdump/main.c
> index 1e62c8adc1..4cfb038395 100644
> --- a/app/pdump/main.c
> +++ b/app/pdump/main.c
> @@ -451,14 +451,17 @@ print_pdump_stats(void)
> }
> }
>
> -static inline void
> +static inline int
> disable_pdump(struct pdump_tuples *pt)
> {
> if (pt->dump_by_type == DEVICE_ID)
> - rte_pdump_disable_by_deviceid(pt->device_id, pt->queue,
> - pt->dir);
> + return rte_pdump_disable_by_deviceid(pt->device_id,
> pt->queue,
> + pt->dir);
> else if (pt->dump_by_type == PORT_ID)
> - rte_pdump_disable(pt->port, pt->queue, pt->dir);
> + return rte_pdump_disable(pt->port, pt->queue, pt->dir);
> +
> + rte_errno = EINVAL;
> + return -EINVAL;
> }
>
> static inline void
> @@ -518,15 +521,30 @@ static void
> cleanup_pdump_resources(void)
> {
> int i;
> + int ret;
> + bool disable_failed = false;
> struct pdump_tuples *pt;
> char name[RTE_ETH_NAME_MAX_LEN];
>
> - /* disable pdump and free the pdump_tuple resources */
> + /* Disable all callbacks first; freeing shared objects before this
> is unsafe. */
> for (i = 0; i < num_tuples; i++) {
> pt = &pdump_t[i];
>
> - /* remove callbacks */
> - disable_pdump(pt);
> + ret = disable_pdump(pt);
> + if (ret < 0) {
> + disable_failed = true;
> + printf("pdump disable failed (tuple=%d, errno=%d:
> %s); "
> + "skip teardown to avoid stale callback
> access\n",
> + i, rte_errno, rte_strerror(rte_errno));
> + }
> + }
> +
> + if (disable_failed)
> + return;
> +
> + /* free the pdump_tuple resources */
> + for (i = 0; i < num_tuples; i++) {
> + pt = &pdump_t[i];
>
> /*
> * transmit rest of the enqueued packets of the rings on to
> diff --git a/doc/guides/rel_notes/release_26_07.rst
> b/doc/guides/rel_notes/release_26_07.rst
> index 4ca0a9ac77..1700a56bc7 100644
> --- a/doc/guides/rel_notes/release_26_07.rst
> +++ b/doc/guides/rel_notes/release_26_07.rst
> @@ -177,6 +177,13 @@ New Features
> Added AGENTS.md file for AI review
> and supporting scripts to review patches and documentation.
>
> +* **Hardened pdump teardown on disable failure.**
> +
> + Hardened pdump request completion and application cleanup behavior so
> timeout
> + or disable failure does not trigger premature pdump resource teardown.
> + pdump now uses an opt-in MP request mode that ignores peers without the
> + pdump action, while preserving legacy EAL MP behavior for all other
> users.
> +
>
> Removed Items
> -------------
> diff --git a/lib/eal/common/eal_common_proc.c
> b/lib/eal/common/eal_common_proc.c
> index 06f151818c..5a458c0417 100644
> --- a/lib/eal/common/eal_common_proc.c
> +++ b/lib/eal/common/eal_common_proc.c
> @@ -56,6 +56,7 @@ static struct action_entry_list action_entry_list =
> enum mp_type {
> MP_MSG, /* Share message with peers, will not block */
> MP_REQ, /* Request for information, Will block for a reply */
> + MP_REQ_IGN, /* Request where missing action should return ignore */
> MP_REP, /* Response to previously-received request */
> MP_IGN, /* Response telling requester to ignore this response */
> };
> @@ -384,11 +385,11 @@ process_msg(struct mp_msg_internal *m, struct
> sockaddr_un *s)
> pthread_mutex_unlock(&mp_mutex_action);
>
> if (!action) {
> - if (m->type == MP_REQ && !internal_conf->init_complete) {
> - /* if this is a request, and init is not yet
> complete,
> - * and callback wasn't registered, we should tell
> the
> - * requester to ignore our existence because we're
> not
> - * yet ready to process this request.
> + if ((m->type == MP_REQ && !internal_conf->init_complete) ||
> + m->type == MP_REQ_IGN) {
> + /*
> + * Ask requester to ignore this peer when action
> is not
> + * registered either due to early init or explicit
> request policy.
> */
> struct rte_mp_msg dummy;
>
> @@ -936,7 +937,8 @@ mp_request_async(const char *dst, struct rte_mp_msg
> *req,
>
> static int
> mp_request_sync(const char *dst, struct rte_mp_msg *req,
> - struct rte_mp_reply *reply, const struct timespec *ts)
> + struct rte_mp_reply *reply, const struct timespec *ts,
> + enum mp_type req_type)
> {
> int ret;
> pthread_condattr_t attr;
> @@ -959,7 +961,7 @@ mp_request_sync(const char *dst, struct rte_mp_msg
> *req,
> return -1;
> }
>
> - ret = send_msg(dst, req, MP_REQ);
> + ret = send_msg(dst, req, req_type);
> if (ret < 0) {
> EAL_LOG(ERR, "Fail to send request %s:%s",
> dst, req->name);
> @@ -1010,11 +1012,20 @@ RTE_EXPORT_SYMBOL(rte_mp_request_sync)
> int
> rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
> const struct timespec *ts)
> +{
> + return rte_mp_request_sync_ex(req, reply, ts, 0);
> +}
> +
> +RTE_EXPORT_SYMBOL(rte_mp_request_sync_ex)
> +int
> +rte_mp_request_sync_ex(struct rte_mp_msg *req, struct rte_mp_reply *reply,
> + const struct timespec *ts, uint32_t flags)
> {
> int dir_fd, ret = -1;
> DIR *mp_dir;
> struct dirent *ent;
> struct timespec now, end;
> + enum mp_type req_type = MP_REQ;
> const struct internal_config *internal_conf =
> eal_get_internal_configuration();
>
> @@ -1027,6 +1038,14 @@ rte_mp_request_sync(struct rte_mp_msg *req, struct
> rte_mp_reply *reply,
> if (check_input(req) != 0)
> goto end;
>
> + if (flags & ~RTE_MP_REQ_F_IGNORE_NO_ACTION) {
> + rte_errno = EINVAL;
> + goto end;
> + }
> +
> + if (flags & RTE_MP_REQ_F_IGNORE_NO_ACTION)
> + req_type = MP_REQ_IGN;
> +
> if (internal_conf->no_shconf) {
> EAL_LOG(DEBUG, "No shared files mode enabled, IPC is
> disabled");
> rte_errno = ENOTSUP;
> @@ -1046,7 +1065,7 @@ rte_mp_request_sync(struct rte_mp_msg *req, struct
> rte_mp_reply *reply,
> /* for secondary process, send request to the primary process only
> */
> if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
> pthread_mutex_lock(&pending_requests.lock);
> - ret = mp_request_sync(eal_mp_socket_path(), req, reply,
> &end);
> + ret = mp_request_sync(eal_mp_socket_path(), req, reply,
> &end, req_type);
> pthread_mutex_unlock(&pending_requests.lock);
> goto end;
> }
> @@ -1086,7 +1105,7 @@ rte_mp_request_sync(struct rte_mp_msg *req, struct
> rte_mp_reply *reply,
> /* unlocks the mutex while waiting for response,
> * locks on receive
> */
> - if (mp_request_sync(path, req, reply, &end))
> + if (mp_request_sync(path, req, reply, &end, req_type))
> goto unlock_end;
> }
> ret = 0;
> diff --git a/lib/eal/include/rte_eal.h b/lib/eal/include/rte_eal.h
> index 7241f3be5d..415b974945 100644
> --- a/lib/eal/include/rte_eal.h
> +++ b/lib/eal/include/rte_eal.h
> @@ -173,6 +173,15 @@ struct rte_mp_reply {
> struct rte_mp_msg *msgs; /* caller to free */
> };
>
> +/** Request flags for rte_mp_request_sync_ex(). */
> +enum rte_mp_request_flags {
> + /**
> + * Ask peers that do not have a registered action to return MP_IGN
> + * instead of causing timeout for this request.
> + */
> + RTE_MP_REQ_F_IGNORE_NO_ACTION = 1u << 0,
> +};
> +
> /**
> * Action function typedef used by other components.
> *
> @@ -292,6 +301,30 @@ int
> rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
> const struct timespec *ts);
>
> +/**
> + * Send a request to peer processes with explicit request flags.
> + *
> + * This API is equivalent to rte_mp_request_sync() with opt-in behavior
> + * controls provided through @p flags.
> + *
> + * @param req
> + * The req argument contains the customized request message.
> + * @param reply
> + * The reply argument will be for storing all the replied messages;
> + * the caller is responsible for free reply->msgs.
> + * @param ts
> + * The ts argument specifies how long we can wait for the peer(s) to
> reply.
> + * @param flags
> + * Bitmask of values from enum rte_mp_request_flags.
> + *
> + * @return
> + * - On success, return 0.
> + * - On failure, return -1, and the reason will be stored in rte_errno.
> + */
> +int
> +rte_mp_request_sync_ex(struct rte_mp_msg *req, struct rte_mp_reply *reply,
> + const struct timespec *ts, uint32_t flags);
> +
> /**
> * Send a request to the peer process and expect a reply in a separate
> callback.
> *
> diff --git a/lib/eal/windows/eal_mp.c b/lib/eal/windows/eal_mp.c
> index 6703355318..1066a4d2bc 100644
> --- a/lib/eal/windows/eal_mp.c
> +++ b/lib/eal/windows/eal_mp.c
> @@ -56,10 +56,19 @@ RTE_EXPORT_SYMBOL(rte_mp_request_sync)
> int
> rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
> const struct timespec *ts)
> +{
> + return rte_mp_request_sync_ex(req, reply, ts, 0);
> +}
> +
> +RTE_EXPORT_SYMBOL(rte_mp_request_sync_ex)
> +int
> +rte_mp_request_sync_ex(struct rte_mp_msg *req, struct rte_mp_reply *reply,
> + const struct timespec *ts, uint32_t flags)
> {
> RTE_SET_USED(req);
> RTE_SET_USED(reply);
> RTE_SET_USED(ts);
> + RTE_SET_USED(flags);
> EAL_LOG_NOT_IMPLEMENTED();
> return -1;
> }
> diff --git a/lib/pdump/rte_pdump.c b/lib/pdump/rte_pdump.c
> index ac94efe7ff..3764aeb2d3 100644
> --- a/lib/pdump/rte_pdump.c
> +++ b/lib/pdump/rte_pdump.c
> @@ -3,9 +3,12 @@
> */
>
> #include <stdlib.h>
> +#include <errno.h>
>
> #include <eal_export.h>
> +#include <rte_eal.h>
> #include <rte_alarm.h>
> +#include <rte_cycles.h>
> #include <rte_mbuf.h>
> #include <rte_ethdev.h>
> #include <rte_lcore.h>
> @@ -68,6 +71,7 @@ struct pdump_request {
>
> const struct rte_bpf_prm *prm;
> uint32_t snaplen;
> + uint32_t generation;
> };
>
> struct pdump_response {
> @@ -81,6 +85,11 @@ struct pdump_bundle {
> char peer[];
> };
>
> +struct pdump_shared_ctl {
> + RTE_ATOMIC(uint32_t) enabled;
> + RTE_ATOMIC(uint32_t) inflight;
> +};
> +
> static struct pdump_rxtx_cbs {
> struct rte_ring *ring;
> struct rte_mempool *mp;
> @@ -88,11 +97,11 @@ static struct pdump_rxtx_cbs {
> const struct rte_bpf *filter;
> enum pdump_version ver;
> uint32_t snaplen;
> + uint32_t generation;
> RTE_ATOMIC(uint32_t) use_count;
> } rx_cbs[RTE_MAX_ETHPORTS][RTE_MAX_QUEUES_PER_PORT],
> tx_cbs[RTE_MAX_ETHPORTS][RTE_MAX_QUEUES_PER_PORT];
>
> -
> /*
> * The packet capture statistics keep track of packets
> * accepted, filtered and dropped. These are per-queue
> @@ -102,9 +111,59 @@ static const char MZ_RTE_PDUMP_STATS[] =
> "rte_pdump_stats";
> static struct {
> struct rte_pdump_stats
> rx[RTE_MAX_ETHPORTS][RTE_MAX_QUEUES_PER_PORT];
> struct rte_pdump_stats
> tx[RTE_MAX_ETHPORTS][RTE_MAX_QUEUES_PER_PORT];
> + struct pdump_shared_ctl
> rx_ctl[RTE_MAX_ETHPORTS][RTE_MAX_QUEUES_PER_PORT];
> + struct pdump_shared_ctl
> tx_ctl[RTE_MAX_ETHPORTS][RTE_MAX_QUEUES_PER_PORT];
> + RTE_ATOMIC(uint32_t) generation;
> const struct rte_memzone *mz;
> } *pdump_stats;
>
> +static int
> +pdump_enter(struct pdump_shared_ctl *ctl, const struct pdump_rxtx_cbs
> *cbs)
> +{
> + uint32_t generation =
> rte_atomic_load_explicit(&pdump_stats->generation,
> +
> rte_memory_order_relaxed);
> +
> + if (rte_atomic_load_explicit(&ctl->enabled,
> rte_memory_order_relaxed) == 0 ||
> + cbs->generation != generation)
> + return 0;
> +
> + rte_atomic_fetch_add_explicit(&ctl->inflight, 1,
> rte_memory_order_acquire);
> +
> + /*
> + * Disable/re-enable may race with callback entry. Re-check
> enabled and
> + * generation after taking inflight reference so disable can wait
> for this
> + * callback to drain and stale callbacks become no-op.
> + */
> + generation = rte_atomic_load_explicit(&pdump_stats->generation,
> + rte_memory_order_relaxed);
> + if (rte_atomic_load_explicit(&ctl->enabled,
> rte_memory_order_relaxed) != 0 &&
> + cbs->generation == generation)
> + return 1;
> +
> + rte_atomic_fetch_sub_explicit(&ctl->inflight, 1,
> rte_memory_order_release);
> + return 0;
> +}
> +
> +static __rte_always_inline void
> +pdump_exit(struct pdump_shared_ctl *ctl)
> +{
> + rte_atomic_fetch_sub_explicit(&ctl->inflight, 1,
> rte_memory_order_release);
> +}
> +
> +static int
> +pdump_wait_inflight(struct pdump_shared_ctl *ctl)
> +{
> + uint64_t end_tsc = rte_get_timer_cycles() + MP_TIMEOUT_S *
> rte_get_timer_hz();
> +
> + while (rte_atomic_load_explicit(&ctl->inflight,
> rte_memory_order_acquire) != 0) {
> + if (rte_get_timer_cycles() > end_tsc)
> + return -ETIMEDOUT;
> + rte_pause();
> + }
> +
> + return 0;
> +}
> +
> static void
> pdump_cb_wait(struct pdump_rxtx_cbs *cbs)
> {
> @@ -224,12 +283,17 @@ pdump_rx(uint16_t port, uint16_t queue,
> uint16_t max_pkts __rte_unused, void *user_params)
> {
> struct pdump_rxtx_cbs *cbs = user_params;
> + struct pdump_shared_ctl *ctl = &pdump_stats->rx_ctl[port][queue];
> struct rte_pdump_stats *stats = &pdump_stats->rx[port][queue];
>
> + if (!pdump_enter(ctl, cbs))
> + return nb_pkts;
> +
> pdump_cb_hold(cbs);
> pdump_copy(port, queue, RTE_PCAPNG_DIRECTION_IN,
> pkts, nb_pkts, cbs, stats);
> pdump_cb_release(cbs);
> + pdump_exit(ctl);
>
> return nb_pkts;
> }
> @@ -239,12 +303,17 @@ pdump_tx(uint16_t port, uint16_t queue,
> struct rte_mbuf **pkts, uint16_t nb_pkts, void
> *user_params)
> {
> struct pdump_rxtx_cbs *cbs = user_params;
> + struct pdump_shared_ctl *ctl = &pdump_stats->tx_ctl[port][queue];
> struct rte_pdump_stats *stats = &pdump_stats->tx[port][queue];
>
> + if (!pdump_enter(ctl, cbs))
> + return nb_pkts;
> +
> pdump_cb_hold(cbs);
> pdump_copy(port, queue, RTE_PCAPNG_DIRECTION_OUT,
> pkts, nb_pkts, cbs, stats);
> pdump_cb_release(cbs);
> + pdump_exit(ctl);
>
> return nb_pkts;
> }
> @@ -255,20 +324,43 @@ pdump_register_rx_callbacks(enum pdump_version ver,
> uint16_t end_q, uint16_t port, uint16_t queue,
> struct rte_ring *ring, struct rte_mempool *mp,
> struct rte_bpf *filter,
> - uint16_t operation, uint32_t snaplen)
> + uint16_t operation, uint32_t snaplen,
> + uint32_t generation)
> {
> uint16_t qid;
>
> qid = (queue == RTE_PDUMP_ALL_QUEUES) ? 0 : queue;
> for (; qid < end_q; qid++) {
> struct pdump_rxtx_cbs *cbs = &rx_cbs[port][qid];
> + struct pdump_shared_ctl *ctl =
> &pdump_stats->rx_ctl[port][qid];
>
> if (operation == ENABLE) {
> + int ret;
> +
> if (cbs->cb) {
> - PDUMP_LOG_LINE(ERR,
> - "rx callback for port=%d queue=%d,
> already exists",
> - port, qid);
> - return -EEXIST;
> + if (cbs->generation == generation &&
> + rte_atomic_load_explicit(&ctl->enabled,
> + rte_memory_order_relaxed) !=
> 0) {
> + PDUMP_LOG_LINE(ERR,
> + "rx callback for port=%d
> queue=%d, already exists",
> + port, qid);
> + return -EEXIST;
> + }
> +
> + PDUMP_LOG_LINE(DEBUG,
> + "reconciling stale rx callback for
> port=%d queue=%d"
> + " old_gen=%u new_gen=%u",
> + port, qid, cbs->generation,
> generation);
> +
> + ret = rte_eth_remove_rx_callback(port,
> qid, cbs->cb);
> + if (ret < 0) {
> + PDUMP_LOG_LINE(ERR,
> + "failed to reconcile stale
> rx callback, errno=%d",
> + -ret);
> + return ret;
> + }
> + pdump_cb_wait(cbs);
> + cbs->cb = NULL;
> }
> cbs->use_count = 0;
> cbs->ver = ver;
> @@ -276,6 +368,7 @@ pdump_register_rx_callbacks(enum pdump_version ver,
> cbs->mp = mp;
> cbs->snaplen = snaplen;
> cbs->filter = filter;
> + cbs->generation = generation;
>
> cbs->cb = rte_eth_add_first_rx_callback(port, qid,
> pdump_rx,
> cbs);
> @@ -286,10 +379,15 @@ pdump_register_rx_callbacks(enum pdump_version ver,
> return rte_errno;
> }
>
> + rte_atomic_store_explicit(&ctl->inflight, 0,
> rte_memory_order_relaxed);
> + rte_atomic_store_explicit(&ctl->enabled, 1,
> rte_memory_order_release);
> +
> memset(&pdump_stats->rx[port][qid], 0,
> sizeof(struct rte_pdump_stats));
> } else if (operation == DISABLE) {
> int ret;
>
> + rte_atomic_store_explicit(&ctl->enabled, 0,
> rte_memory_order_release);
> +
> if (cbs->cb == NULL) {
> PDUMP_LOG_LINE(ERR,
> "no existing rx callback for
> port=%d queue=%d",
> @@ -298,6 +396,11 @@ pdump_register_rx_callbacks(enum pdump_version ver,
> }
> ret = rte_eth_remove_rx_callback(port, qid,
> cbs->cb);
> if (ret < 0) {
> + /* Keep state coherent: callback is still
> registered,
> + * so restore enabled.
> + */
> + rte_atomic_store_explicit(&ctl->enabled, 1,
> + rte_memory_order_release);
> PDUMP_LOG_LINE(ERR,
> "failed to remove rx callback,
> errno=%d",
> -ret);
> @@ -305,6 +408,10 @@ pdump_register_rx_callbacks(enum pdump_version ver,
> }
> pdump_cb_wait(cbs);
> cbs->cb = NULL;
> +
> + ret = pdump_wait_inflight(ctl);
> + if (ret < 0)
> + return ret;
> }
> }
>
> @@ -316,7 +423,8 @@ pdump_register_tx_callbacks(enum pdump_version ver,
> uint16_t end_q, uint16_t port, uint16_t queue,
> struct rte_ring *ring, struct rte_mempool *mp,
> struct rte_bpf *filter,
> - uint16_t operation, uint32_t snaplen)
> + uint16_t operation, uint32_t snaplen,
> + uint32_t generation)
> {
>
> uint16_t qid;
> @@ -324,13 +432,36 @@ pdump_register_tx_callbacks(enum pdump_version ver,
> qid = (queue == RTE_PDUMP_ALL_QUEUES) ? 0 : queue;
> for (; qid < end_q; qid++) {
> struct pdump_rxtx_cbs *cbs = &tx_cbs[port][qid];
> + struct pdump_shared_ctl *ctl =
> &pdump_stats->tx_ctl[port][qid];
>
> if (operation == ENABLE) {
> + int ret;
> +
> if (cbs->cb) {
> - PDUMP_LOG_LINE(ERR,
> - "tx callback for port=%d queue=%d,
> already exists",
> - port, qid);
> - return -EEXIST;
> + if (cbs->generation == generation &&
> + rte_atomic_load_explicit(&ctl->enabled,
> + rte_memory_order_relaxed) !=
> 0) {
> + PDUMP_LOG_LINE(ERR,
> + "tx callback for port=%d
> queue=%d, already exists",
> + port, qid);
> + return -EEXIST;
> + }
> +
> + PDUMP_LOG_LINE(DEBUG,
> + "reconciling stale tx callback for
> port=%d queue=%d"
> + " old_gen=%u new_gen=%u",
> + port, qid, cbs->generation,
> generation);
> +
> + ret = rte_eth_remove_tx_callback(port,
> qid, cbs->cb);
> + if (ret < 0) {
> + PDUMP_LOG_LINE(ERR,
> + "failed to reconcile stale
> tx callback, errno=%d",
> + -ret);
> + return ret;
> + }
> +
> + pdump_cb_wait(cbs);
> + cbs->cb = NULL;
> }
> cbs->use_count = 0;
> cbs->ver = ver;
> @@ -338,6 +469,7 @@ pdump_register_tx_callbacks(enum pdump_version ver,
> cbs->mp = mp;
> cbs->snaplen = snaplen;
> cbs->filter = filter;
> + cbs->generation = generation;
>
> cbs->cb = rte_eth_add_tx_callback(port, qid,
> pdump_tx,
> cbs);
> @@ -347,10 +479,15 @@ pdump_register_tx_callbacks(enum pdump_version ver,
> rte_errno);
> return rte_errno;
> }
> +
> + rte_atomic_store_explicit(&ctl->inflight, 0,
> rte_memory_order_relaxed);
> + rte_atomic_store_explicit(&ctl->enabled, 1,
> rte_memory_order_release);
> memset(&pdump_stats->tx[port][qid], 0,
> sizeof(struct rte_pdump_stats));
> } else if (operation == DISABLE) {
> int ret;
>
> + rte_atomic_store_explicit(&ctl->enabled, 0,
> rte_memory_order_release);
> +
> if (cbs->cb == NULL) {
> PDUMP_LOG_LINE(ERR,
> "no existing tx callback for
> port=%d queue=%d",
> @@ -359,6 +496,11 @@ pdump_register_tx_callbacks(enum pdump_version ver,
> }
> ret = rte_eth_remove_tx_callback(port, qid,
> cbs->cb);
> if (ret < 0) {
> + /* Keep state coherent: callback is still
> registered,
> + * so restore enabled.
> + */
> + rte_atomic_store_explicit(&ctl->enabled, 1,
> + rte_memory_order_release);
> PDUMP_LOG_LINE(ERR,
> "failed to remove tx callback,
> errno=%d",
> -ret);
> @@ -367,6 +509,10 @@ pdump_register_tx_callbacks(enum pdump_version ver,
>
> pdump_cb_wait(cbs);
> cbs->cb = NULL;
> +
> + ret = pdump_wait_inflight(ctl);
> + if (ret < 0)
> + return ret;
> }
> }
>
> @@ -459,7 +605,8 @@ set_pdump_rxtx_cbs(const struct pdump_request *p)
> end_q = (queue == RTE_PDUMP_ALL_QUEUES) ? nb_rx_q : queue
> + 1;
> ret = pdump_register_rx_callbacks(p->ver, end_q, port,
> queue,
> ring, mp, filter,
> - operation, p->snaplen);
> + operation, p->snaplen,
> + p->generation);
> if (ret < 0)
> return ret;
> }
> @@ -469,7 +616,8 @@ set_pdump_rxtx_cbs(const struct pdump_request *p)
> end_q = (queue == RTE_PDUMP_ALL_QUEUES) ? nb_tx_q : queue
> + 1;
> ret = pdump_register_tx_callbacks(p->ver, end_q, port,
> queue,
> ring, mp, filter,
> - operation, p->snaplen);
> + operation, p->snaplen,
> + p->generation);
> if (ret < 0)
> return ret;
> }
> @@ -477,12 +625,14 @@ set_pdump_rxtx_cbs(const struct pdump_request *p)
> return ret;
> }
>
> -static void
> -pdump_request_to_secondary(const struct pdump_request *req)
> +static int
> +pdump_request_to_secondary_sync(const struct pdump_request *req)
> {
> struct rte_mp_msg mp_req = { };
> struct rte_mp_reply mp_reply;
> struct timespec ts = {.tv_sec = MP_TIMEOUT_S, .tv_nsec = 0};
> + int ret = 0;
> + uint16_t i;
>
> PDUMP_LOG_LINE(DEBUG, "forward req %s to secondary",
> pdump_opname(req->op));
>
> @@ -490,14 +640,41 @@ pdump_request_to_secondary(const struct
> pdump_request *req)
> strlcpy(mp_req.name, PDUMP_MP, sizeof(mp_req.name));
> mp_req.len_param = sizeof(*req);
>
> - if (rte_mp_request_sync(&mp_req, &mp_reply, &ts) != 0)
> - PDUMP_LOG_LINE(ERR, "rte_mp_request_sync failed");
> + if (rte_mp_request_sync_ex(&mp_req, &mp_reply, &ts,
> + RTE_MP_REQ_F_IGNORE_NO_ACTION) != 0) {
> + PDUMP_LOG_LINE(ERR, "rte_mp_request_sync failed: %s",
> + strerror(rte_errno));
> + return -rte_errno;
> + }
>
> - else if (mp_reply.nb_sent != mp_reply.nb_received)
> + if (mp_reply.nb_sent != mp_reply.nb_received) {
> PDUMP_LOG_LINE(ERR, "not all secondary's replied (sent %u
> recv %u)",
> mp_reply.nb_sent, mp_reply.nb_received);
> + ret = -ETIMEDOUT;
> + }
> +
> + for (i = 0; i < mp_reply.nb_received; i++) {
> + struct rte_mp_msg *mp_rep = &mp_reply.msgs[i];
> + const struct pdump_response *resp;
> +
> + if (mp_rep->len_param != sizeof(*resp)) {
> + PDUMP_LOG_LINE(ERR, "invalid secondary reply size
> %u", mp_rep->len_param);
> + if (ret == 0)
> + ret = -EINVAL;
> + continue;
> + }
> +
> + resp = (const struct pdump_response *)mp_rep->param;
> + if (resp->err_value != 0) {
> + PDUMP_LOG_LINE(ERR, "secondary reply failed: op=%u
> err=%d",
> + resp->res_op, resp->err_value);
> + if (ret == 0)
> + ret = resp->err_value;
> + }
> + }
>
> free(mp_reply.msgs);
> + return ret;
> }
>
> /* Allocate temporary storage for passing state to the alarm thread for
> deferred handling */
> @@ -556,7 +733,10 @@ pdump_handle_primary_request(const struct rte_mp_msg
> *mp_msg, const void *peer)
> PDUMP_LOG_LINE(DEBUG, "secondary pdump %s",
> pdump_opname(req->op));
>
> /* Can just do it now, no need for interrupt thread */
> - ret = set_pdump_rxtx_cbs(req);
> + if (req->op == ENABLE || req->op == DISABLE)
> + ret = set_pdump_rxtx_cbs(req);
> + else
> + ret = -EINVAL;
> }
>
> return pdump_send_response(req, ret, peer);
> @@ -569,17 +749,29 @@ __pdump_request(void *param)
> {
> struct pdump_bundle *bundle = param;
> struct rte_mp_msg *msg = &bundle->msg;
> - const struct pdump_request *req =
> - (const struct pdump_request *)msg->param;
> + struct pdump_request *req =
> + (struct pdump_request *)msg->param;
> int ret;
>
> PDUMP_LOG_LINE(DEBUG, "primary pdump %s", pdump_opname(req->op));
>
> + if (req->op == DISABLE) {
> + req->generation =
> rte_atomic_fetch_add_explicit(&pdump_stats->generation, 1,
> +
> rte_memory_order_acq_rel) + 1;
> + } else if (req->op == ENABLE) {
> + req->generation =
> rte_atomic_load_explicit(&pdump_stats->generation,
> +
> rte_memory_order_acquire);
> + }
> +
> ret = set_pdump_rxtx_cbs(req);
>
> - /* Primary process is responsible for broadcasting request to all
> secondaries */
> + /*
> + * Primary process is responsible for broadcasting the request to
> all
> + * secondaries. The sync request uses opt-in ignore-missing-action
> mode so
> + * pdump does not depend on unrelated secondary processes.
> + */
> if (ret == 0)
> - pdump_request_to_secondary(req);
> + ret = pdump_request_to_secondary_sync(req);
>
> pdump_send_response(req, ret, bundle->peer);
> free(bundle);
> @@ -659,6 +851,9 @@ rte_pdump_init(void)
>
> pdump_stats = mz->addr;
> pdump_stats->mz = mz;
> + if (rte_eal_process_type() == RTE_PROC_PRIMARY)
> + rte_atomic_store_explicit(&pdump_stats->generation, 1,
> + rte_memory_order_release);
>
> return 0;
> }
> diff --git a/lib/pdump/rte_pdump.h b/lib/pdump/rte_pdump.h
> index 1e32d46097..77e5914420 100644
> --- a/lib/pdump/rte_pdump.h
> +++ b/lib/pdump/rte_pdump.h
> @@ -64,7 +64,7 @@ rte_pdump_uninit(void);
> * queues of a given port.
> * @param flags
> * flags specifies
> RTE_PDUMP_FLAG_RX/RTE_PDUMP_FLAG_TX/RTE_PDUMP_FLAG_RXTX
> - * on which packet capturing should be enabled for a given port and
> queue.
> + * on which packet capturing should be disabled for a given port and
> queue.
> * @param ring
> * ring on which captured packets will be enqueued for user.
> * @param mp
> @@ -72,6 +72,11 @@ rte_pdump_uninit(void);
> * @param filter
> * Unused should be NULL.
> *
> + * @note
> + * In applications that enable capture on multiple interfaces, enable
> may be
> + * partially applied before an error is returned. Callers should
> explicitly
> + * unwind partial enable state.
> + *
> * @return
> * 0 on success, -1 on error, rte_errno is set accordingly.
> */
> @@ -103,6 +108,11 @@ rte_pdump_enable(uint16_t port, uint16_t queue,
> uint32_t flags,
> * @param prm
> * Use BPF program to run to filter packes (can be NULL)
> *
> + * @note
> + * In applications that enable capture on multiple interfaces, enable
> may be
> + * partially applied before an error is returned. Callers should
> explicitly
> + * unwind partial enable state.
> + *
> * @return
> * 0 on success, -1 on error, rte_errno is set accordingly.
> */
> @@ -124,7 +134,14 @@ rte_pdump_enable_bpf(uint16_t port_id, uint16_t queue,
> * queues of a given port.
> * @param flags
> * flags specifies
> RTE_PDUMP_FLAG_RX/RTE_PDUMP_FLAG_TX/RTE_PDUMP_FLAG_RXTX
> - * on which packet capturing should be enabled for a given port and
> queue.
> + * on which packet capturing should be disabled for a given port and
> queue.
> + *
> + * @note
> + * A disable failure (including timeout/no response) means teardown is
> not
> + * complete across pdump-enabled peers. The caller must not release
> shared
> + * pdump resources and must not uninitialize pdump for that capture
> session
> + * until disable succeeds. Releasing resources on disable failure can
> lead to
> + * crashes in peer processes still accessing those resources.
> *
> * @return
> * 0 on success, -1 on error, rte_errno is set accordingly.
> @@ -153,6 +170,11 @@ rte_pdump_disable(uint16_t port, uint16_t queue,
> uint32_t flags);
> * @param filter
> * unused should be NULL
> *
> + * @note
> + * In applications that enable capture on multiple interfaces, enable
> may be
> + * partially applied before an error is returned. Callers should
> explicitly
> + * unwind partial enable state.
> + *
> * @return
> * 0 on success, -1 on error, rte_errno is set accordingly.
> */
> @@ -186,6 +208,11 @@ rte_pdump_enable_by_deviceid(char *device_id,
> uint16_t queue,
> * @param filter
> * Use BPF program to run to filter packes (can be NULL)
> *
> + * @note
> + * In applications that enable capture on multiple interfaces, enable
> may be
> + * partially applied before an error is returned. Callers should
> explicitly
> + * unwind partial enable state.
> + *
> * @return
> * 0 on success, -1 on error, rte_errno is set accordingly.
> */
> @@ -210,7 +237,14 @@ rte_pdump_enable_bpf_by_deviceid(const char
> *device_id, uint16_t queue,
> * queues of a given device id.
> * @param flags
> * flags specifies
> RTE_PDUMP_FLAG_RX/RTE_PDUMP_FLAG_TX/RTE_PDUMP_FLAG_RXTX
> - * on which packet capturing should be enabled for a given port and
> queue.
> + * on which packet capturing should be disabled for a given port and
> queue.
> + *
> + * @note
> + * A disable failure (including timeout/no response) means teardown is
> not
> + * complete across pdump-enabled peers. The caller must not release
> shared
> + * pdump resources and must not uninitialize pdump for that capture
> session
> + * until disable succeeds. Releasing resources on disable failure can
> lead to
> + * crashes in peer processes still accessing those resources.
> *
> * @return
> * 0 on success, -1 on error, rte_errno is set accordingly.
> --
> 2.43.0
>
>
[-- Attachment #2: Type: text/html, Size: 57594 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2026-07-06 8:26 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-07-01 6:02 [PATCH] pdump: fix request timeout on unresponsive secondary Pushpendra Kumar
2026-07-02 4:01 ` Pushpendra Kumar
2026-07-03 15:52 ` Stephen Hemminger
2026-07-05 8:44 ` [PATCH v2] pdump: fix teardown race with opt-in MP request mode Pushpendra Kumar
2026-07-05 17:07 ` [PATCH v3] " Pushpendra Kumar
2026-07-05 18:27 ` [PATCH v2] " Stephen Hemminger
2026-07-05 21:08 ` Kumar, Pushpendra1X
2026-07-06 8:25 ` Stephen Hemminger
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox