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

[Morten Brørup]

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

+1

> 
> > > > 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?

Yes.