RE: [PATCH v4 2/6] ethdev: add trace points for ethdev

["Morten Brørup" <mb@smartsharesystems.com>]

> From: Ankur Dwivedi [mailto:adwivedi@marvell.com]
> Sent: Thursday, 22 December 2022 07.33
> 
> Adds trace points for ethdev functions.
> Moved the rte_ethdev_trace_rx_burst and rte_ethdev_trace_tx_burst to
> a new file rte_ethdev_trace_fp_burst.h. This is needed to resolve
> cyclic dependency between rte_ethdev.h and rte_ethdev_trace_fp.h.
> 
> Signed-off-by: Ankur Dwivedi <adwivedi@marvell.com>
> ---
>  lib/ethdev/ethdev_private.c            |   3 +
>  lib/ethdev/ethdev_trace_points.c       | 193 +++++++++++++++++
>  lib/ethdev/meson.build                 |   1 +
>  lib/ethdev/rte_ethdev.c                | 203 ++++++++++++++++--
>  lib/ethdev/rte_ethdev.h                |   2 +-
>  lib/ethdev/rte_ethdev_trace.h          | 285 +++++++++++++++++++++++++
>  lib/ethdev/rte_ethdev_trace_fp.h       | 276 +++++++++++++++++++++++-
>  lib/ethdev/rte_ethdev_trace_fp_burst.h |  44 ++++
>  lib/ethdev/version.map                 |  66 ++++++
>  9 files changed, 1047 insertions(+), 26 deletions(-)
>  create mode 100644 lib/ethdev/rte_ethdev_trace_fp_burst.h
> 
> diff --git a/lib/ethdev/ethdev_private.c b/lib/ethdev/ethdev_private.c
> index 48090c879a..a8f87f22c1 100644
> --- a/lib/ethdev/ethdev_private.c
> +++ b/lib/ethdev/ethdev_private.c
> @@ -5,6 +5,7 @@
>  #include <rte_debug.h>
> 
>  #include "rte_ethdev.h"
> +#include "rte_ethdev_trace_fp.h"
>  #include "ethdev_driver.h"
>  #include "ethdev_private.h"
> 
> @@ -297,6 +298,7 @@ rte_eth_call_rx_callbacks(uint16_t port_id,
> uint16_t queue_id,
>  		cb = cb->next;
>  	}
> 
> +	rte_eth_trace_call_rx_callbacks(port_id, queue_id, nb_rx,
> nb_pkts);

Reviewing code like this, it is impossible to determine if this is a fast path trace or not, without also looking elsewhere.

Could we please agree on a naming convention to include _fp in fast path trace functions and macros?

>  	return nb_rx;
>  }
> 
> @@ -312,6 +314,7 @@ rte_eth_call_tx_callbacks(uint16_t port_id,
> uint16_t queue_id,
>  		cb = cb->next;
>  	}
> 
> +	rte_eth_trace_call_tx_callbacks(port_id, queue_id, nb_pkts);
>  	return nb_pkts;
>  }
> 
> diff --git a/lib/ethdev/ethdev_trace_points.c
> b/lib/ethdev/ethdev_trace_points.c
> index 2919409a15..4fea76e0ff 100644
> --- a/lib/ethdev/ethdev_trace_points.c
> +++ b/lib/ethdev/ethdev_trace_points.c
> @@ -5,6 +5,7 @@
>  #include <rte_trace_point_register.h>
> 
>  #include <rte_ethdev_trace.h>
> +#include <rte_ethdev_trace_fp.h>
> 
>  RTE_TRACE_POINT_REGISTER(rte_ethdev_trace_configure,
>  	lib.ethdev.configure)
> @@ -29,3 +30,195 @@ RTE_TRACE_POINT_REGISTER(rte_ethdev_trace_rx_burst,
> 
>  RTE_TRACE_POINT_REGISTER(rte_ethdev_trace_tx_burst,
>  	lib.ethdev.tx.burst)
> +
> +RTE_TRACE_POINT_REGISTER(rte_eth_trace_call_rx_callbacks,
> +	lib.ethdev.call_rx_callbacks)

Same comment as above: Reviewing code like this, I cannot immediately determine if this is fast path trace or not.