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

[Jerin Jacob <jerinjacobk@gmail.com>]

On Fri, Oct 11, 2024 at 10:51 PM Pavan Nikhilesh Bhagavatula
<pbhagavatula@marvell.com> wrote:
>
>
>
> > -----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.

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.