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) <humi...@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.
