On Fri, Oct 11, 2024 at 10:51 PM Pavan Nikhilesh Bhagavatula
<pbhagavat...@marvell.com> wrote:
>
>
>
> > -----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.
IMO, it is OK to expose a version of rte_node_increment_error() as
Robin suggested.
Application is free to use public or more optimized version based on the needed.
Prefers rte_node_error_increment() (Keeping action/verb as last) though.
