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