RE: [EXTERNAL] Re: [PATCH v4 4/5] node: add error stats for ip4 lookup node

[Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com>]

> -----Original Message-----
> From: Robin Jarry <rjarry@redhat.com>
> Sent: Friday, October 11, 2024 3:19 PM
> To: Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com>; Jerin Jacob
> <jerinj@marvell.com>; Nithin Kumar Dabilpuram
> <ndabilpuram@marvell.com>; Kiran Kumar Kokkilagadda
> <kirankumark@marvell.com>; zhirun.yan@intel.com; Wathsala Vithanage
> <wathsala.vithanage@arm.com>; Bruce Richardson
> <bruce.richardson@intel.com>; Konstantin Ananyev
> <konstantin.v.ananyev@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 <pbhagavatula@marvell.com>
> >
> > Add error counters for ip4 LPM lookup failures in
> > ip4_lookup node.
> >
> > Signed-off-by: Pavan Nikhilesh <pbhagavatula@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.