> From: Thomas Monjalon [mailto:[email protected]] > Sent: Monday, 13 October 2025 20.39 > > 02/10/2025 09:37, Morten Brørup: > > > +RTE_EXPORT_EXPERIMENTAL_SYMBOL(rte_mbuf_history_dump_mempool, > 25.11) > > > +void rte_mbuf_history_dump_mempool(FILE *f, struct rte_mempool > *mp) > > > +{ > > > +#if !RTE_MBUF_HISTORY_DEBUG > > > + RTE_SET_USED(f); > > > + RTE_SET_USED(mp); > > > + MBUF_LOG(INFO, "mbuf history recorder is not enabled"); > > > +#else > > > + if (f == NULL) { > > > + MBUF_LOG(ERR, "Invalid mbuf dump file."); > > > + return; > > > + } > > > + if (mp == NULL) { > > > + fprintf(f, "ERROR: Invalid mempool pointer\n"); > > > > Should be MBUF_LOG(ERR, ...), not fprintf(). > > > > > + return; > > > + } > > > + if (rte_mbuf_history_field_offset < 0) { > > > + fprintf(f, "WARNING: mbuf history not initialized. > Call > > > rte_mbuf_history_init() first.\n"); > > > > Should be MBUF_LOG(ERR, ...), not fprintf(). > > > > > + return; > > > + } > > > > Since the type of objects held in a mempool is not identifiable as > mbufs, you should check that (mp->elt_size >= sizeof(struct rte_mbuf)). > Imagine some non-mbuf mempool holding 64 byte sized objects, and > rte_mbuf_history_field_offset being in the second cache line.
<feature creep> You can check more properties of the mempool to identify mbuf mempools, seeking inspiration from the RTE_ASSERT()'s in rte_pktmbuf_pool_init(): https://elixir.bootlin.com/dpdk/v25.07/source/lib/mbuf/rte_mbuf.c#L35 </feature creep> > > > > You might want to log an error if called directly, and silently skip > of called from rte_mbuf_history_dump_all(), so suggest adding a wrapper > when calling this function through rte_mempool_walk(). > > Yes good idea. > > > > + mbuf_history_get_stats(mp, f); > > > +#endif > > > +} > > [...] > > > > > +/** > > > + * Mark an mbuf with a history event. > > > + * > > > + * @param m > > > + * Pointer to the mbuf. > > > + * @param op > > > + * The operation to record. > > > + */ > > > +static inline void rte_mbuf_history_mark(struct rte_mbuf *m, > uint32_t > > > op) > > > +{ > > > +#if !RTE_MBUF_HISTORY_DEBUG > > > + RTE_SET_USED(m); > > > + RTE_SET_USED(op); > > > +#else > > > + RTE_ASSERT(rte_mbuf_history_field_offset >= 0); > > > + RTE_ASSERT(op < RTE_MBUF_HISTORY_OP_MAX); > > > + if (unlikely (m == NULL)) > > > + return; > > > + > > > + rte_mbuf_history_t *history_field = RTE_MBUF_DYNFIELD(m, > > > + rte_mbuf_history_field_offset, > rte_mbuf_history_t *); > > > + uint64_t history = rte_atomic_load_explicit(history_field, > > > rte_memory_order_acquire); > > > + history = (history << RTE_MBUF_HISTORY_BITS) | op; > > > + rte_atomic_store_explicit(history_field, history, > > > rte_memory_order_release); > > > > This is not thread safe. > > Some other thread can race to update history_field after this thread > loads it, so when this tread stores the updated history, the other > thread's history update is overwritten and lost. > > You're right. > I suppose this change was to align with the atomic read operation > done in the "get" function. > > > To make it thread safe, you must use a CAS operation and start over > if it failed. > > By "failed", you mean if the previous value returned by the CAS > operation > is different of what we used earlier to build our new value? > > I suggest this: > > rte_mbuf_history_t *history_field = RTE_MBUF_DYNFIELD(m, > rte_mbuf_history_field_offset, rte_mbuf_history_t *); > uint64_t old_history = rte_atomic_load_explicit(history_field, > rte_memory_order_acquire); > uint64_t new_history; > do { > new_history = (old_history << RTE_MBUF_HISTORY_BITS) | op; > } while (!rte_atomic_compare_exchange_weak_explicit(history_field, > &old_history, new_history, > rte_memory_order_release, rte_memory_order_relaxed)); Yes, that was the thread safety concept I was looking for. Small bugfix: rte_memory_order_relaxed should be rte_memory_order_acquire (when the CAS comparison fails), like when first loading old_history with rte_atomic_load_explicit(). Also, consider adding unlikely: "do {...} while (unlikely(!CAS(...)));".
