Re: [PATCH 1/6] ethdev: add trace points

[Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>]

On 8/4/22 16:44, Ankur Dwivedi wrote:
> Add trace points for ethdev functions.
> 
> Signed-off-by: Ankur Dwivedi <adwivedi@marvell.com>
> ---

[snip]

> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> index 1979dc0850..a6fb370b22 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c

[snip]

> @@ -525,6 +536,7 @@ rte_eth_dev_owner_delete(const uint64_t owner_id)
>   
>   	rte_spinlock_unlock(&eth_dev_shared_data->ownership_lock);
>   
> +	rte_ethdev_trace_owner_delete(owner_id, ret);

I'm wondering why trace is sometimes added in the middle of the
function, but in the majority of cases it is added as the first
or the last action. Is there any logical/guidelines behind it?

>   	return ret;
>   }
>   

[snip]

> diff --git a/lib/ethdev/rte_ethdev_trace.h b/lib/ethdev/rte_ethdev_trace.h
> index 1491c815c3..de728d355d 100644
> --- a/lib/ethdev/rte_ethdev_trace.h
> +++ b/lib/ethdev/rte_ethdev_trace.h

[snip]

> +RTE_TRACE_POINT(

Shouldn't it be RTE_TRACE_POINT_FP? Isn't it fast path?

> +	rte_eth_trace_call_rx_callbacks,
> +	RTE_TRACE_POINT_ARGS(uint16_t port_id, uint16_t queue_id,
> +		struct rte_mbuf **rx_pkts, uint16_t nb_rx,
> +		uint16_t nb_pkts, void *opaque),
> +	rte_trace_point_emit_u16(port_id);
> +	rte_trace_point_emit_u16(queue_id);
> +	rte_trace_point_emit_ptr(rx_pkts);
> +	rte_trace_point_emit_u16(nb_rx);
> +	rte_trace_point_emit_u16(nb_pkts);
> +	rte_trace_point_emit_ptr(opaque);
> +)
> +
> +RTE_TRACE_POINT(

same here

> +	rte_eth_trace_call_tx_callbacks,
> +	RTE_TRACE_POINT_ARGS(uint16_t port_id, uint16_t queue_id,
> +		struct rte_mbuf **tx_pkts, uint16_t nb_pkts,
> +		void *opaque),
> +	rte_trace_point_emit_u16(port_id);
> +	rte_trace_point_emit_u16(queue_id);
> +	rte_trace_point_emit_ptr(tx_pkts);
> +	rte_trace_point_emit_u16(nb_pkts);
> +	rte_trace_point_emit_ptr(opaque);
> +)
> +

[snip]

> +RTE_TRACE_POINT(
> +	rte_ethdev_trace_info_get,
> +	RTE_TRACE_POINT_ARGS(uint16_t port_id,
> +		struct rte_eth_dev_info *dev_info),
> +	rte_trace_point_emit_u16(port_id);
> +	rte_trace_point_emit_string(dev_info->driver_name);
> +	rte_trace_point_emit_u32(dev_info->if_index);
> +	rte_trace_point_emit_u16(dev_info->min_mtu);
> +	rte_trace_point_emit_u16(dev_info->max_mtu);
> +	rte_trace_point_emit_u32(dev_info->min_rx_bufsize);
> +	rte_trace_point_emit_u32(dev_info->max_rx_pktlen);
> +	rte_trace_point_emit_u64(dev_info->rx_offload_capa);
> +	rte_trace_point_emit_u64(dev_info->tx_offload_capa);
> +	rte_trace_point_emit_u64(dev_info->rx_queue_offload_capa);
> +	rte_trace_point_emit_u64(dev_info->tx_queue_offload_capa);
> +	rte_trace_point_emit_u16(dev_info->reta_size);
> +	rte_trace_point_emit_u8(dev_info->hash_key_size);
> +	rte_trace_point_emit_u16(dev_info->nb_rx_queues);
> +	rte_trace_point_emit_u16(dev_info->nb_tx_queues);

How to make a choice which information should be included
above?

> +)
> +

[snip]