+Thomas Monjalon & +David Marchand, as intended by Jerin > From: Jerin Jacob [mailto:[email protected]] > Sent: Tuesday, 23 June 2026 08.57 > > On Tue, Jun 23, 2026 at 12:15 PM Morten Brørup > <[email protected]> wrote: > > > > > From: Jerin Jacob [mailto:[email protected]] > > > Sent: Tuesday, 23 June 2026 07.13 > > > > > > On Mon, Jun 22, 2026 at 12:11 AM Morten Brørup > > > <[email protected]> wrote: > > > > > > > > Added graph node profiling stats, build time configurable by > enabling > > > > RTE_GRAPH_PROFILE in rte_config.h. > > > > > > > > Signed-off-by: Morten Brørup <[email protected]> > > > > > > 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. */ > > > > }; > > > > }; > > > >
