Re: [PATCH v3 1/4] ethdev: introduce ethdev HW desc dump PI

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

On 6/1/22 10:49, Min Hu (Connor) wrote:
> Added the ethdev HW Rx desc dump API which provides functions for query
> HW descriptor from device. HW descriptor info differs in different NICs.
> The information demonstrates I/O process which is important for debug.
> As the information is different between NICs, the new API is introduced.
> 
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> ---
>   doc/guides/rel_notes/release_22_07.rst |  7 ++++
>   lib/ethdev/ethdev_driver.h             | 42 ++++++++++++++++++++++++
>   lib/ethdev/rte_ethdev.c                | 44 ++++++++++++++++++++++++++
>   lib/ethdev/rte_ethdev.h                | 44 ++++++++++++++++++++++++++
>   lib/ethdev/version.map                 |  2 ++
>   5 files changed, 139 insertions(+)
> 
> diff --git a/doc/guides/rel_notes/release_22_07.rst b/doc/guides/rel_notes/release_22_07.rst
> index 8932a1d478..56c675121a 100644
> --- a/doc/guides/rel_notes/release_22_07.rst
> +++ b/doc/guides/rel_notes/release_22_07.rst
> @@ -137,6 +137,13 @@ New Features
>     * ``RTE_EVENT_QUEUE_ATTR_WEIGHT``
>     * ``RTE_EVENT_QUEUE_ATTR_AFFINITY``
>   
> +* **Added ethdev HW desc dump API, to dump Rx/Tx HW desc info from device.**
> +
> +  Added the ethdev HW Rx desc dump API which provides functions for query
> +  HW descriptor from device. HW descriptor info differs in different NICs.
> +  The information demonstrates I/O process which is important for debug.
> +  As the information is different between NICs, the new API is introduced.
> +
>   
>   Removed Items
>   -------------
> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
> index 69d9dc21d8..9c1726eb2d 100644
> --- a/lib/ethdev/ethdev_driver.h
> +++ b/lib/ethdev/ethdev_driver.h
> @@ -1073,6 +1073,42 @@ typedef int (*eth_ip_reassembly_conf_set_t)(struct rte_eth_dev *dev,
>    */
>   typedef int (*eth_dev_priv_dump_t)(struct rte_eth_dev *dev, FILE *file);
>   
> +/**
> + * @internal
> + * Dump ethdev Rx descriptor info to a file.

ethdev is not required in the description above. It is an ethdev
operation type.

Is there any limitations on caller context? Should be it the same
CPU core (since typically it is the case for per-queue API) which
polls the queue? The answer must be in the API function description.

> + *
> + * @param file
> + *   A pointer to a file for output.
> + * @param dev
> + *   Port (ethdev) handle.
> + * @param queue_id
> + *   The selected queue.
> + * @param desc_id
> + *   The selected descriptor.

Is it an ID in the ring regardless of the current position or
is it offset relative to current position in the ring?
It should be clarified in any case.
I'd say that it should be an offset to be consistent with
descriptor status API.

It would be useful to be able to dump many descriptor at once.

> + * @return
> + *   Negative errno value on error, zero on success.
> + */
> +typedef int (*eth_rx_hw_desc_dump_t)(FILE *file, const struct rte_eth_dev *dev,
> +				     uint16_t queue_id, uint16_t desc_id);
> +
> +/**
> + * @internal
> + * Dump ethdev Tx descriptor info to a file.
> + *
> + * @param file
> + *   A pointer to a file for output.
> + * @param dev
> + *   Port (ethdev) handle.
> + * @param queue_id
> + *   The selected queue.
> + * @param desc_id
> + *   The selected descriptor.
> + * @return
> + *   Negative errno value on error, zero on success.
> + */
> +typedef int (*eth_tx_hw_desc_dump_t)(FILE *file, const struct rte_eth_dev *dev,
> +				     uint16_t queue_id, uint16_t desc_id);
> +
>   /**
>    * @internal A structure containing the functions exported by an Ethernet driver.
>    */
> @@ -1283,6 +1319,12 @@ struct eth_dev_ops {
>   
>   	/** Dump private info from device */
>   	eth_dev_priv_dump_t eth_dev_priv_dump;
> +
> +	/** Dump ethdev Rx descriptor info */\\

It is an ethdev operations. So, 'ethdev' is not necessary in the
description.

> +	eth_rx_hw_desc_dump_t eth_rx_hw_desc_dump;
> +
> +	/** Dump ethdev Tx descriptor info */
> +	eth_tx_hw_desc_dump_t eth_tx_hw_desc_dump;
>   };
>   
>   /**
> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> index 46c088dc88..bbd8439fa0 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -5876,6 +5876,50 @@ rte_eth_dev_priv_dump(uint16_t port_id, FILE *file)
>   	return eth_err(port_id, (*dev->dev_ops->eth_dev_priv_dump)(dev, file));
>   }
>   
> +int
> +rte_eth_rx_hw_desc_dump(FILE *file, uint16_t port_id, uint16_t queue_id,
> +			uint16_t desc_id)
> +{
> +	struct rte_eth_dev *dev;
> +	int ret;
> +
> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> +	dev = &rte_eth_devices[port_id];

Shouldn't we check file vs null?

> +
> +	if (queue_id >= dev->data->nb_rx_queues) {
> +		RTE_ETHDEV_LOG(ERR, "Invalid Rx queue_id=%u\n", queue_id);
> +		return -EINVAL;
> +	}

Don't we need a check vs queue size?

> +
> +	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->eth_rx_hw_desc_dump, -ENOTSUP);
> +	ret = (*dev->dev_ops->eth_rx_hw_desc_dump)(file, dev, queue_id,
> +						   desc_id);
> +
> +	return ret;
> +}