[v3,1/4] ethdev: introduce ethdev HW desc dump PI
Checks
Commit Message
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(+)
Comments
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;
> +}
Hi Andrew
Many thanks for your review.
On 2022/6/2 3:53, Andrew Rybchenko wrote:
> 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.
Will remove it.
>
> 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.
"[PATCH v3 4/4] app/procinfo: support descriptor dump" shows how to use
the API. It is used for debug, not a dataplane API, no special
limitations on caller context.
>
>> + *
>> + * @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 is an ID in the ring regardless of the current position.
>
> It would be useful to be able to dump many descriptor at once.
This can be done by the appliacation.
>
>> + * @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.
Will fix.
>
>> + 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?
Yes, will do.
>
>> +
>> + 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?
This need to be done in pmd as "[PATCH v3 3/4] net/hns3: support Rx/Tx
bd dump" shows.
Thanks,
Dongdong
>
>> +
>> + 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;
>> +}
>
> .
>
Cc various driver maintainers looking for opinion about API discussion
below
On 6/7/22 16:59, Dongdong Liu wrote:
> Hi Andrew
>
> Many thanks for your review.
>
> On 2022/6/2 3:53, Andrew Rybchenko wrote:
>> 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.
>
> Will remove it.
>>
>> 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.
>
> "[PATCH v3 4/4] app/procinfo: support descriptor dump" shows how to use
> the API. It is used for debug, not a dataplane API, no special
> limitations on caller context.
Please, be explicit in the ethdev API documentation.
>>
>>> + *
>>> + * @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 is an ID in the ring regardless of the current position.
IMHO, it is inconvenient since typically it is interesting
what happens nearby current position.
>>
>> It would be useful to be able to dump many descriptor at once.
> This can be done by the appliacation.
Yes, that's true, but easy to do in the driver as well. It would
be especially important if ID semantics changes.
>>
>>> + * @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.
> Will fix.
>>
>>> + 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?
> Yes, will do.
>>
>>> +
>>> + 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?
>
> This need to be done in pmd as "[PATCH v3 3/4] net/hns3: support Rx/Tx
> bd dump" shows.
All applicable generic checks must be done on ethdev level to avoid code
duplication and missing checks in PMDs.
Andrew.
Hi Andrew
Many thanks for your review.
On 2022/6/8 18:09, Andrew Rybchenko wrote:
> Cc various driver maintainers looking for opinion about API discussion
> below
>
> On 6/7/22 16:59, Dongdong Liu wrote:
>> Hi Andrew
>>
>> Many thanks for your review.
>>
>> On 2022/6/2 3:53, Andrew Rybchenko wrote:
>>> 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.
>>
>> Will remove it.
>>>
>>> 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.
>>
>> "[PATCH v3 4/4] app/procinfo: support descriptor dump" shows how to use
>> the API. It is used for debug, not a dataplane API, no special
>> limitations on caller context.
>
> Please, be explicit in the ethdev API documentation.
OK, will do
>
>>>
>>>> + *
>>>> + * @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 is an ID in the ring regardless of the current position.
>
> IMHO, it is inconvenient since typically it is interesting
> what happens nearby current position.
>
OK, will fix.
>>>
>>> It would be useful to be able to dump many descriptor at once.
>> This can be done by the appliacation.
>
> Yes, that's true, but easy to do in the driver as well. It would
> be especially important if ID semantics changes.
How about changing parameter 'desc_id' to 'num' to dump
[current position, current position + num - 1] descriptors.
/**
* @internal
* Dump Rx 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 num
* The number of the descriptors to dump.
* @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 num);
>
>>>
>>>> + * @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.
>> Will fix.
>>>
>>>> + 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?
>> Yes, will do.
>>>
>>>> +
>>>> + 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?
>>
>> This need to be done in pmd as "[PATCH v3 3/4] net/hns3: support Rx/Tx
>> bd dump" shows.
>
> All applicable generic checks must be done on ethdev level to avoid code
> duplication and missing checks in PMDs.
The ethdev level does not know the queue size.
This is similar to rte_eth_rx_descriptor_status(),
queue size check is also done in PMDs.
Thanks,
Dongdong.
>
> Andrew.
> .
>
@@ -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
-------------
@@ -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.
+ *
+ * @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_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 */
+ 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;
};
/**
@@ -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];
+
+ if (queue_id >= dev->data->nb_rx_queues) {
+ RTE_ETHDEV_LOG(ERR, "Invalid Rx queue_id=%u\n", queue_id);
+ return -EINVAL;
+ }
+
+ 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;
+}
+
+int
+rte_eth_tx_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];
+
+ if (queue_id >= dev->data->nb_tx_queues) {
+ RTE_ETHDEV_LOG(ERR, "Invalid Tx queue_id=%u\n", queue_id);
+ return -EINVAL;
+ }
+
+ RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->eth_tx_hw_desc_dump, -ENOTSUP);
+ ret = (*dev->dev_ops->eth_tx_hw_desc_dump)(file, dev, queue_id,
+ desc_id);
+
+ return ret;
+}
+
RTE_LOG_REGISTER_DEFAULT(rte_eth_dev_logtype, INFO);
RTE_INIT(ethdev_init_telemetry)
@@ -5456,6 +5456,50 @@ typedef struct {
__rte_experimental
int rte_eth_dev_priv_dump(uint16_t port_id, FILE *file);
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
+ *
+ * Dump ethdev Rx 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
+ * - On success, zero.
+ * - On failure, a negative value.
+ */
+__rte_experimental
+int rte_eth_rx_hw_desc_dump(FILE *file, uint16_t port_id, uint16_t queue_id,
+ uint16_t desc_id);
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
+ *
+ * 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
+ * - On success, zero.
+ * - On failure, a negative value.
+ */
+__rte_experimental
+int rte_eth_tx_hw_desc_dump(FILE *file, uint16_t port_id, uint16_t queue_id,
+ uint16_t desc_id);
+
#include <rte_ethdev_core.h>
/**
@@ -285,6 +285,8 @@ EXPERIMENTAL {
rte_mtr_color_in_protocol_priority_get;
rte_mtr_color_in_protocol_set;
rte_mtr_meter_vlan_table_update;
+ rte_eth_rx_hw_desc_dump;
+ rte_eth_tx_hw_desc_dump;
};
INTERNAL {