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

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

+Thomas Monjalon & +David Marchand, as intended by Jerin

> From: Jerin Jacob [mailto:jerinjacobk@gmail.com]
> Sent: Tuesday, 23 June 2026 08.57
> 
> On Tue, Jun 23, 2026 at 12:15 PM Morten Brørup
> <mb@smartsharesystems.com> wrote:
> >
> > > 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.
> 
> 
> Yes. I don't know the current policy for this. Adding @Thomas Monjalon
> @David Marchand
> 
> 
> >
> > 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.
> 
> Yes for the structure inclusion we can use #ifdef. But inside the
> function we can use rte_lib_has_some_feature() scheme. Reasons are :
> 1)It will remove the ifdef cultter
> 2)Detect the compilation issue even if the feature is disabled. This
> will make sure reduce the build options to enable build sanity
> 3) Compiler is smart enough to understand to disable the block if the
> feature is not enabled.(Just like #ifdef)

I agree with these advantages.
But a function using rte_lib_has_some_feature() cannot access non-existing fields:
https://godbolt.org/z/s3nKx45Ms

So sometimes #ifdef is required in the code too.

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