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

[Thomas Monjalon <thomas@monjalon.net>]

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