Re: [PATCH v5 0/3] Introduce node-specific errors in graph library

["Robin Jarry" <rjarry@redhat.com>]

Hi Pavan,

I am resending my review here. It seems you didn't get it.

, Oct 14, 2024 at 13:58:
> From: Pavan Nikhilesh <pbhagavatula@marvell.com>
>
>
> Introduce the ability for nodes to advertise error counters during
> registration and increment them during the node process function in
> the graph library.
> This enhancement allows for better error tracking and debugging
> capabilities within the graph framework.
>
> The number of errors and the mapping of error IDs to error descriptions
> are defined during node registration.
> If an error is encountered during the node process function while walking
> the graph, the respective error counter is incremented.

This feature could be useful to store detailed statistics per node, not 
only for errors. It would be better to rename "errors" to "xstats".

See below for a concrete suggestion.

>
> Example:
> 	static struct rte_node_errors ip4_reassembly_errors = {
> 		.nb_errors = 1,
> 		.err_desc = {
> 			[0] = "ip4_reassembly_error",
> 		},
> 	};

  static const struct rte_node_xstats ip4_reassembly_xstats = {
    .xstats_num = 1,
    .xstats_desc = {
      [0] = "ip4_reassembly_error",
    },
  }

>
> Here, "ip4_reassembly_error" is mapped to error ID 0, and the same ID is
> used in the `ip4_reassembly_node_process` function to increment reassembly
> errors.
> Depending on the node, there can be multiple such errors that can be
> updated independently and retrieved using `rte_graph_cluster_stats_get`.
>
> Example:
> +-------------------------------+---------------+---------------+--------------+
> |Node                           |calls          |objs           |realloc_count |
> +-------------------------------+---------------+---------------+--------------+
> |ip4_lookup                     |1324083        |338965248      |2             |
> |       ip4_lookup_error        |               |338965496      |              |
> |pkt_drop                       |1324084        |338965504      |1             |
> |ethdev_rx-0-0                  |1324086        |338966016      |2             |
> |pkt_cls                        |1324086        |338966016      |1             |
> +-------------------------------+---------------+---------------+--------------+
>
> v2 Changes:
> - Fix compilation.
> v3 Changes:
> - Resend as 1/5 didn't make it through.
> v4 Changes:
> - Address review comments.
> - Rebase on main branch.
> v5 Changes:
> - Shrink structure member names.(Robin)
> - add rte_node_error_increment utility function. (Robin)
> - Squash patches. (Robin)
> - Update RN, DN. (David)
>
> Pavan Nikhilesh (3):
>   graph: add support for node specific errors
>   graph: add node error counters

I think patch 1/3 and 2/3 should be split differently. My preference 
would be to have documentation (especially large svg images) in 
a separate commit. Other than that, the changes in lib/graph should be 
squashed in the same patch.

>   node: add error stats for ip4 nodes
>
>  doc/guides/prog_guide/graph_lib.rst           |  22 +-
>  .../prog_guide/img/anatomy_of_a_node.svg      | 329 +++++--
>  .../prog_guide/img/graph_mem_layout.svg       | 921 +++++++++++++-----
>  doc/guides/rel_notes/deprecation.rst          |   6 -
>  doc/guides/rel_notes/release_24_11.rst        |   8 +
>  lib/graph/graph_populate.c                    |  20 +-
>  lib/graph/graph_private.h                     |   3 +
>  lib/graph/graph_stats.c                       |  78 +-
>  lib/graph/node.c                              |  37 +-
>  lib/graph/rte_graph.h                         |  11 +
>  lib/graph/rte_graph_worker_common.h           |  23 +
>  lib/graph/version.map                         |   7 +
>  lib/node/ip4_lookup.c                         |   9 +
>  lib/node/ip4_lookup_neon.h                    |   5 +
>  lib/node/ip4_lookup_sse.h                     |   6 +
>  lib/node/ip4_reassembly.c                     |   9 +
>  lib/node/node_private.h                       |   8 +
>  17 files changed, 1192 insertions(+), 310 deletions(-)

To summarize changes, here is my proposal:

 struct rte_node_xstats {
   uint8_t xstats_num; /**< Number of xstats. */
   char (*xstats_desc)[RTE_NODE_XSTATS_DESC_SIZE]; /**< Names of xstats. */
 };

 struct rte_node_register {
   ...
   const struct rte_node_xstats *xstats; /**< Node specific extra statistics. */
   ...
 };

 static inline void
 rte_node_xstat_increment(struct rte_node *node, uint16_t stat_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
 }

 struct __rte_cache_aligned rte_graph_cluster_node_stats {
   ...
   uint8_t xstats_num;
   char (*xstats_desc)[RTE_NODE_XSTATS_DESC_SIZE];
   uint64_t *xstats_val;
   ...
 };

Let me know what you think. Thanks!