RE: [EXTERNAL] Re: [PATCH v4 3/5] graph: add stats for node specific errors

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

> > From: Robin Jarry <rjarry@redhat.com>
> > Sent: Friday, October 11, 2024 3:24 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; Zhirun Yan
> > <yanzhirun_163@163.com>
> > Cc: dev@dpdk.org
> > Subject: [EXTERNAL] Re: [PATCH v4 3/5] graph: add stats for node specific
> > errors
> >
> > Hi Pavan, , Aug 16, 2024 at 17: 09: > From: Pavan Nikhilesh
> > <pbhagavatula@ marvell. com> > > Add support for retrieving/printing stats
> > for node specific > errors using rte_graph_cluster_stats_get(). > > Signed-off-
> > by: Pavan
> >
> > Hi Pavan,
> >
> > , Aug 16, 2024 at 17:09:
> > > From: Pavan Nikhilesh <pbhagavatula@marvell.com>
> > >
> > > Add support for retrieving/printing stats for node specific
> > > errors using rte_graph_cluster_stats_get().
> > >
> > > Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
> > > ---
> >
> > [snip]
> >
> > > diff --git a/lib/graph/rte_graph.h b/lib/graph/rte_graph.h
> > > index b28143d737..12b6461cf5 100644
> > > --- a/lib/graph/rte_graph.h
> > > +++ b/lib/graph/rte_graph.h
> > > @@ -223,6 +223,10 @@ struct __rte_cache_aligned
> > rte_graph_cluster_node_stats {
> > >
> > >  	uint64_t realloc_count; /**< Realloc count. */
> > >
> > > +	uint8_t node_error_cntrs;			   /**< Number of
> > Node error counters. */
> > > +	char (*node_error_desc)[RTE_NODE_ERROR_DESC_SIZE]; /**< Names
> > of the Node error counters. */
> >
> > Why do you need the parentheses here?

The parenthesis signify that it's a pointer to array that has fixed size characters.
If we remove the parenthesis, compiler will treat it as array of pointers to char.

../lib/graph/graph_stats.c: In function ‘stats_mem_populate’:
../lib/graph/graph_stats.c:274:42: error: assignment to expression with array type
  274 |                 cluster->stat.error_desc = rte_zmalloc_socket(
      |                                          ^
../lib/graph/graph_stats.c:277:46: error: the comparison will always evaluate as ‘false’ for the address of ‘error_desc’ will never be NULL [-Werror=address]
  277 |                 if (cluster->stat.error_desc == NULL) {
      |                                              ^~
In file included from ../lib/graph/graph_private.h:17,
                 from ../lib/graph/graph_stats.c:13:
../lib/graph/rte_graph.h:227:15: note: ‘error_desc’ declared here
  227 |         char *error_desc[RTE_NODE_ERROR_DESC_SIZE]; /**< Names of the Node error counters. */
      |               ^~~~~~~~~~
cc1: all warnings being treated as errors

> >
> > > +	uint64_t *node_error_count;			   /**< Total error
> > count per each error. */
> >
> > The node_ prefix is redundant here. Can you use something shorter?
> >
> >    uint8_t errors_num; /**< Number of Node error counters. */
> >    char (*errors_desc)[RTE_NODE_ERROR_DESC_SIZE];
> >    uint64_t *errors_value;
> >
> 
> I will shrink them in the next version.
> 
> > Thanks!