Jerin, Saeed, > From: Jerin Jacob [mailto:[email protected]] > Sent: Tuesday, 23 June 2026 11.09 > > On Tue, Jun 23, 2026 at 12:40 PM Morten Brørup > <[email protected]> wrote: > > > > +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 > > > I missed that. > > You can add these new struct updates in the slowpath area of rte_node. > Above offsetof(struct rte_node, ctx) > > Use RTE_NEXT_ABI, Get around off, ABI breakge issue.
Please review [v9], and I will submit a rebased version for DPDK 26.11. [v9]: https://patchwork.dpdk.org/project/dpdk/patch/[email protected]/ > > > > > > So sometimes #ifdef is required in the code too. > > > > > > > > > > > > > > > > > > > > > > > > > > alignas(RTE_CACHE_LINE_MIN_SIZE) struct > rte_node > > > > > *nodes[]; /**< Next nodes. */ > > > > > > }; > > > > > > }; > > > > > >
