RE: [PATCH v5] graph: add optional profiling stats

["Morten Brørup" <mb@smartsharesystems.com>]

> From: Jerin Jacob [mailto:jerinjacobk@gmail.com]
> Sent: Tuesday, 23 June 2026 07.13
> 
> On Mon, Jun 22, 2026 at 12:11 AM Morten Brørup
> <mb@smartsharesystems.com> wrote:
> >
> > Added graph node profiling stats, build time configurable by enabling
> > RTE_GRAPH_PROFILE in rte_config.h.
> >
> > Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> 
> Please update app/test/test_graph.c to validate this featue.

Ack.

> > @@ -92,7 +92,62 @@ rte_graph_obj_dump(FILE *f, struct rte_graph *g,
> bool all)
> >                         fprintf(f, "       total_sched_fail=%" PRId64
> "\n",
> >                                 n->dispatch.total_sched_fail);
> >                 }
> > -               fprintf(f, "       total_calls=%" PRId64 "\n", n-
> >total_calls);
> > +               fprintf(f, "       total_calls=%" PRIu64 "\n", n-
> >total_calls);
> > +               if (rte_graph_has_stats_feature()) {
> > +                       fprintf(f, "       total_cycles=%" PRIu64 ",
> avg cycles/call=%.1f\n",
> > +                                       n->total_cycles,
> > +                                       n->total_calls == 0 ?
> (double)0 :
> > +                                       (double)n->total_cycles /
> (double)n->total_calls);
> > +               }
> > +#ifdef RTE_GRAPH_PROFILE
> 
> 
> Please introduce rte_graph_has_profile_featue() similar to
> rte_graph_has_stats_feature() to reduce if def clutter as possible.

Disagree, see below.

> 
> > +               uint64_t calls = n->usage_stats[0].calls;
> > +               fprintf(f, "       objs[0]\n");
> > +               fprintf(f, "         calls=%" PRIu64 ", cycles=%"
> PRIu64 ", avg cycles/call=%.1f\n",
> > +                               calls,
> 
> >
> > diff --git a/lib/graph/rte_graph_worker_common.h
> b/lib/graph/rte_graph_worker_common.h
> > index 4ab53a533e..0d8039575d 100644
> > --- a/lib/graph/rte_graph_worker_common.h
> > +++ b/lib/graph/rte_graph_worker_common.h
> > @@ -144,12 +144,26 @@ struct __rte_cache_aligned rte_node {
> >                         rte_node_process_t process; /**< Process
> function. */
> >                         uint64_t process_u64;
> >                 };
> > +               /** Fast path area cache line 3. */
> > +#ifdef RTE_GRAPH_PROFILE
> > +               struct {
> > +                       uint64_t calls;     /**< Calls processing
> resp. 0 or 1 objects. */
> > +                       uint64_t cycles;    /**< Cycles spent
> processing resp. 0 or 1 objects. */
> > +               } usage_stats[2];       /**< Usage when this node
> processed 0 or 1 objects. */
> > +               uint64_t full_burst_calls;  /**< Calls processing a
> full burst of objects. */
> > +               uint64_t full_burst_cycles; /**< Cycles spent
> processing a full burst of objects. */
> > +               uint64_t half_burst_calls;  /**< Calls processing a
> half burst of objects. */
> > +               uint64_t half_burst_cycles; /**< Cycles spent
> processing a half burst of objects. */
> > +               /** Fast path area cache line 4. */
> > +#endif
> 
> Is it an ABI breakage?

No. The modifications are enclosed in #ifdef, and disabled by default.
It is generally required that when rte_config.h options are modified, both the application and DPDK itself are built together; and then API/ABI breakage becomes irrelevant.

IMO, we should keep our structures lean in release builds. This means that fields used for detailed profiling, advanced debugging, cookie validation, etc. should use the #ifdef pattern rather than the rte_lib_has_some_feature() pattern; especially if they affect the size of a structure. And when those fields are not present, any code accessing them cannot use the rte_lib_has_some_feature() pattern.
The mbuf and mempool libraries also use #ifdef pattern for similar features.

> 
> >                 alignas(RTE_CACHE_LINE_MIN_SIZE) struct rte_node
> *nodes[]; /**< Next nodes. */
> >         };
> >  };
> >