<div dir="auto">Not registering pdump in secondary means there is no capture in secondary. Still think that is misuse</div> <div class="gmail_quote gmail_quote_container"><div dir="ltr" class="gmail_attr">On Sun, Jul 5, 2026, 23:08 Kumar, Pushpendra1X <<a href="mailto:pushpendra1x.kumar@intel.com">pushpendra1x.kumar@intel.com</a>> wrote: </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <div lang="EN-IN" link="blue" vlink="purple" style="overflow-wrap: break-word;"> <div class="m_-4574549248435063437WordSection1"> 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 <div style="border-width:1pt medium medium;border-style:solid none none;border-color:rgb(225,225,225) currentcolor currentcolor;padding:3pt 0in 0in"> From: Stephen Hemminger <<a href="mailto:stephen@networkplumber.org" target="_blank" rel="noreferrer">stephen@networkplumber.org</a>> Sent: Sunday, July 5, 2026 11:58 PM To: Kumar, Pushpendra1X <<a href="mailto:pushpendra1x.kumar@intel.com" target="_blank" rel="noreferrer">pushpendra1x.kumar@intel.com</a>> Cc: dev <<a href="mailto:dev@dpdk.org" target="_blank" rel="noreferrer">dev@dpdk.org</a>>; stable <<a href="mailto:stable@dpdk.org" target="_blank" rel="noreferrer">stable@dpdk.org</a>>; Thomas Monjalon <<a href="mailto:thomas@monjalon.net" target="_blank" rel="noreferrer">thomas@monjalon.net</a>>; Pattan, Reshma <<a href="mailto:reshma.pattan@intel.com" target="_blank" rel="noreferrer">reshma.pattan@intel.com</a>>; Burakov, Anatoly <<a href="mailto:anatoly.burakov@intel.com" target="_blank" rel="noreferrer">anatoly.burakov@intel.com</a>>; Dmitry Kozlyuk <<a href="mailto:dmitry.kozliuk@gmail.com" target="_blank" rel="noreferrer">dmitry.kozliuk@gmail.com</a>>; Richardson, Bruce <<a href="mailto:bruce.richardson@intel.com" target="_blank" rel="noreferrer">bruce.richardson@intel.com</a>>; <a href="mailto:14pwcse1224@uetpeshawar.edu.pk" target="_blank" rel="noreferrer">14pwcse1224@uetpeshawar.edu.pk</a>; <a href="mailto:pushpendra.kumar@tieto.com" target="_blank" rel="noreferrer">pushpendra.kumar@tieto.com</a> Subject: Re: [PATCH v2] pdump: fix teardown race with opt-in MP request mode </div> <div> 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 </div> <div> <div> On Sun, Jul 5, 2026, 01:48 Pushpendra Kumar <<a href="mailto:pushpendra1x.kumar@intel.com" target="_blank" rel="noreferrer">pushpendra1x.kumar@intel.com</a>> wrote: </div> <blockquote style="border-width:medium medium medium 1pt;border-style:none none none solid;border-color:currentcolor currentcolor currentcolor rgb(204,204,204);padding:0in 0in 0in 6pt;margin-left:4.8pt;margin-right:0in"> 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 <<a href="mailto:pushpendra1x.kumar@intel.com" target="_blank" rel="noreferrer">pushpendra1x.kumar@intel.com</a>> Cc: <a href="mailto:stable@dpdk.org" target="_blank" rel="noreferrer">stable@dpdk.org</a> --- 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 <<a href="mailto:przemyslaw.gierszynski@intel.com" target="_blank" rel="noreferrer">przemyslaw.gierszynski@intel.com</a>> Przemyslaw Patynowski <<a href="mailto:przemyslawx.patynowski@intel.com" target="_blank" rel="noreferrer">przemyslawx.patynowski@intel.com</a>> Przemyslaw Zegan <<a href="mailto:przemyslawx.zegan@intel.com" target="_blank" rel="noreferrer">przemyslawx.zegan@intel.com</a>> Pu Xu <<a href="mailto:583493798@qq.com" target="_blank" rel="noreferrer">583493798@qq.com</a>> +Pushpendra Kumar <<a href="mailto:pushpendra1x.kumar@intel.com" target="_blank" rel="noreferrer">pushpendra1x.kumar@intel.com</a>> Qi Fu <<a href="mailto:qi.fu@intel.com" target="_blank" rel="noreferrer">qi.fu@intel.com</a>> Qi Zhang <<a href="mailto:qi.z.zhang@intel.com" target="_blank" rel="noreferrer">qi.z.zhang@intel.com</a>> Qian Hao <<a href="mailto:qi_an_hao@126.com" target="_blank" rel="noreferrer">qi_an_hao@126.com</a>> 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(<a href="http://mp_req.name" target="_blank" rel="noreferrer">mp_req.name</a>, PDUMP_MP, sizeof(<a href="http://mp_req.name" target="_blank" rel="noreferrer">mp_req.name</a>)); 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 </blockquote> </div> </div> </div> </blockquote></div>