On 7/22/2021 10:33 AM, Andrew Rybchenko wrote:

On 7/20/21 7:51 PM, Ferruh Yigit wrote:

quoted

On 6/4/2021 3:42 PM, Andrew Rybchenko wrote:

quoted

From: Ivan Ilchenko <redacted>

Update xstats by IDs callbacks documentation in accordance with
ethdev usage of these callbacks. Document valid combinations of
input arguments to make driver implementation simpler.

Fixes: 79c913a42f0 ("ethdev: retrieve xstats by ID")
Cc: stable@dpdk.org

Signed-off-by: Ivan Ilchenko <redacted>
Signed-off-by: Andrew Rybchenko <redacted>
Reviewed-by: Andy Moreton <redacted>
---
  lib/ethdev/ethdev_driver.h | 43 ++++++++++++++++++++++++++++++++++++--
  1 file changed, 41 insertions(+), 2 deletions(-)

diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
index 40e474aa7e..fd5b7ca550 100644
--- a/lib/ethdev/ethdev_driver.h
+++ b/lib/ethdev/ethdev_driver.h

@@ -187,11 +187,28 @@ typedef int (*eth_xstats_get_t)(struct rte_eth_dev *dev,

      struct rte_eth_xstat *stats, unsigned int n);
  /**< @internal Get extended stats of an Ethernet device. */
  +/**
+ * @internal
+ * Get extended stats of an Ethernet device.

Should it mention _by_id detail?

Yes, will fix in v2.

quoted

quoted

+ *
+ * @param dev
+ *   ethdev handle of port.
+ * @param ids
+ *   IDs array to retrieve specific statistics. Must not be NULL.
+ * @param values
+ *   A pointer to a table to be filled with device statistics values.
+ *   Must not be NULL.
+ * @param n
+ *   Element count in @p ids and @p values
+ *
+ * @return
+ *   - A number of filled in stats.
+ *   - A negative value on error.
+ */
  typedef int (*eth_xstats_get_by_id_t)(struct rte_eth_dev *dev,
                        const uint64_t *ids,
                        uint64_t *values,
                        unsigned int n);
-/**< @internal Get extended stats of an Ethernet device. */
    /**
   * @internal

@@ -218,10 +235,32 @@ typedef int (*eth_xstats_get_names_t)(struct

rte_eth_dev *dev,
      struct rte_eth_xstat_name *xstats_names, unsigned int size);
  /**< @internal Get names of extended stats of an Ethernet device. */
  +/**
+ * @internal
+ * Get names of extended stats of an Ethernet device.

Should it mention _by_id detail?

Yes, will fix in v2.

quoted

quoted

+ * For name count, set @p xstats_names and @p ids to NULL.
+ *

isn't the 'count' part handled in the API? I think in the devops both should not
be NULL.

No, eth_dev_get_xstats_count() uses the callback with NULL, NULL, 0.

Right, I missed it, xstats_get_names_by_id() seems used outside of the ethdev
_by_id() APIs.

quoted

quoted

+ * @param dev
+ *   ethdev handle of port.
+ * @param xstats_names
+ *   An rte_eth_xstat_name array of at least *size* elements to
+ *   be filled. Can be NULL together with @p ids to retrieve number of
+ *   available statistics.

As far as I understand both _by_id APIs and devops behave same, so argument
descriptions/behavior should be same.

In fact no, it is slightly different. For example, devops is never
called with NULL ids and not NULL names or non-zero size. It allows to
check less in drivers.

I am not sure if this difference is intentional.

'eth_dev_get_xstats_count()' is ethdev internal function, what do you think
removing 'xstats_get_names_by_id()' call from it. And make both _by_id() dev_ops
doesn't accept NULL ids and NULL values/names, so simplifies PMD implementations.

quoted

quoted

+ * @param ids
+ *   IDs array to retrieve specific statistics. Can be NULL together
+ *   with @p xstats_names to retrieve number of available statistics.
+ * @param size
+ *   Size of ids and xstats_names arrays.
+ *   Element count in @p ids and @p xstats_names
+ *
+ * @return
+ *   - A number of filled in stats if both xstats_names and ids are not NULL.
+ *   - A number of available stats if both xstats_names and ids are NULL.

Again as far as I can see these covered by API, not devops, am I missing
something.

See eth_dev_get_xstats_count()

quoted

quoted

+ *   - A negative value on error.
+ */
  typedef int (*eth_xstats_get_names_by_id_t)(struct rte_eth_dev *dev,
      struct rte_eth_xstat_name *xstats_names, const uint64_t *ids,
      unsigned int size);
-/**< @internal Get names of extended stats of an Ethernet device. */
    typedef int (*eth_queue_stats_mapping_set_t)(struct rte_eth_dev *dev,
                           uint16_t queue_id,