Re: [PATCH v3 2/5] mbuf: record mbuf operations history

[Thomas Monjalon <thomas@monjalon.net>]

13/10/2025 22:08, Morten Brørup:
> > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > 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>

I'm not sure what we can use to identify mbuf pools.
Anyway I suggest doing such optimization later.

> > > 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().

OK thanks.

> Also, consider adding unlikely: "do {...} while (unlikely(!CAS(...)));".

unlikely because it happens only for cloned mbufs?