> -----Original Message-----
> From: Robin Jarry <rja...@redhat.com>
> Sent: Friday, October 11, 2024 3:19 PM
> To: Pavan Nikhilesh Bhagavatula <pbhagavat...@marvell.com>; Jerin Jacob
> <jer...@marvell.com>; Nithin Kumar Dabilpuram
> <ndabilpu...@marvell.com>; Kiran Kumar Kokkilagadda
> <kirankum...@marvell.com>; zhirun....@intel.com; Wathsala Vithanage
> <wathsala.vithan...@arm.com>; Bruce Richardson
> <bruce.richard...@intel.com>; Konstantin Ananyev
> <konstantin.v.anan...@yandex.ru>
> Cc: dev@dpdk.org
> Subject: [EXTERNAL] Re: [PATCH v4 4/5] node: add error stats for ip4 lookup
> node
>
> Hi Pavan, , Aug 16, 2024 at 17: 09: > From: Pavan Nikhilesh
> <pbhagavatula@ marvell. com> > > Add error counters for ip4 LPM lookup
> failures in > ip4_lookup node. > > Signed-off-by: Pavan Nikhilesh
> <pbhagavatula@ marvell. com>
>
> Hi Pavan,
>
> , Aug 16, 2024 at 17:09:
> > From: Pavan Nikhilesh <pbhagavat...@marvell.com>
> >
> > Add error counters for ip4 LPM lookup failures in
> > ip4_lookup node.
> >
> > Signed-off-by: Pavan Nikhilesh <pbhagavat...@marvell.com>
>
> [snip]
>
> > diff --git a/lib/node/node_private.h b/lib/node/node_private.h
> > index 1de7306792..36b2a733db 100644
> > --- a/lib/node/node_private.h
> > +++ b/lib/node/node_private.h
> > @@ -12,6 +12,8 @@
> > #include <rte_mbuf.h>
> > #include <rte_mbuf_dyn.h>
> >
> > +#include <rte_graph_worker_common.h>
> > +
> > extern int rte_node_logtype;
> > #define RTE_LOGTYPE_NODE rte_node_logtype
> >
> > @@ -88,4 +90,10 @@ node_mbuf_priv2(struct rte_mbuf *m)
> > return (struct node_mbuf_priv2 *)rte_mbuf_to_priv(m);
> > }
> >
> > +#define NODE_INCREMENT_ERROR_ID(node, id, cond, cnt)
> \
> > + {
> > \
> > + if (unlikely(rte_graph_has_stats_feature() && (cond)))
> \
> > + ((uint64_t *)RTE_PTR_ADD(node, node->err_off))[id]
> += (cnt); \
> > + }
>
> This is private API which is not usable with out-of-tree nodes. Could
> you expose a way to increment a given error stat that does not involve
> a hacky macro?
>
> How about something like this in rte_graph_worker_common.h:
>
I would prefer nodes having their own error increment implementation, leaves
room
for optimization rather than forcing all the nodes to use a single
implementation.
Some nodes might also be able to use SIMD to increment error count at once.
Also, I really hate doing
if (error)
rte_node_increment_error()
The macro really looks really clean atleast for inbuild nodes.
We can still have this additional inline function as a utility function.
I will leave it upto the maintainers to decide.
> static inline void
> rte_node_increment_error(struct rte_node *node, uint16_t err_id, uint64_t
> value)
> {
> #ifdef RTE_LIBRTE_GRAPH_STATS
> uint64_t *errors = RTE_PTR_ADD(node, node->err_off);
> errors[err_id] += value;
> #else
> RTE_SET_USED(node);
> RTE_SET_USED(err_id);
> RTE_SET_USED(value);
> #endif
> }
>
> NB: do *not* include a condition in that function. The decision whether
> to increment an error stat or not should remain in the nodes.
