diff mbox series

[v2,08/37] net/mvpp2: extend xstats support

Message ID 20210122191925.24308-9-lironh@marvell.com (mailing list archive)
State Superseded, archived
Delegated to: Jerin Jacob
Headers
Series net/mvpp2: misc updates |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Liron Himi Jan. 22, 2021, 7:18 p.m. UTC 
  From: Yuri Chipchev <yuric@marvell.com>

Add xstats_by_id callbacks

Signed-off-by: Yuri Chipchev <yuric@marvell.com>
Reviewed-by: Liron Himi <lironh@marvell.com>
---
 drivers/net/mvpp2/mrvl_ethdev.c | 98 ++++++++++++++++++++++++++++++++-
 1 file changed, 95 insertions(+), 3 deletions(-)

Comments

Ferruh Yigit Jan. 26, 2021, 6:26 p.m. UTC | #1 
On 1/22/2021 7:18 PM, lironh@marvell.com wrote:
> From: Yuri Chipchev <yuric@marvell.com>
> 
> Add xstats_by_id callbacks
> 
> Signed-off-by: Yuri Chipchev <yuric@marvell.com>
> Reviewed-by: Liron Himi <lironh@marvell.com>
> ---
>   drivers/net/mvpp2/mrvl_ethdev.c | 98 ++++++++++++++++++++++++++++++++-
>   1 file changed, 95 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/mvpp2/mrvl_ethdev.c b/drivers/net/mvpp2/mrvl_ethdev.c
> index e4ec343d5..ba1882266 100644
> --- a/drivers/net/mvpp2/mrvl_ethdev.c
> +++ b/drivers/net/mvpp2/mrvl_ethdev.c
> @@ -173,6 +173,8 @@ static struct {
>   	MRVL_XSTATS_TBL_ENTRY(tx_errors)
>   };
>   
> +#define MRVL_NUM_XSTATS RTE_DIM(mrvl_xstats_tbl)
> +
>   static inline void
>   mrvl_fill_shadowq(struct mrvl_shadow_txq *sq, struct rte_mbuf *buf)
>   {
> @@ -1376,7 +1378,7 @@ mrvl_xstats_get(struct rte_eth_dev *dev,
>   		return 0;
>   
>   	pp2_ppio_get_statistics(priv->ppio, &ppio_stats, 0);
> -	for (i = 0; i < n && i < RTE_DIM(mrvl_xstats_tbl); i++) {
> +	for (i = 0; i < n && i < MRVL_NUM_XSTATS; i++) {
>   		uint64_t val;
>   
>   		if (mrvl_xstats_tbl[i].size == sizeof(uint32_t))
> @@ -1430,9 +1432,9 @@ mrvl_xstats_get_names(struct rte_eth_dev *dev __rte_unused,
>   	unsigned int i;
>   
>   	if (!xstats_names)
> -		return RTE_DIM(mrvl_xstats_tbl);
> +		return MRVL_NUM_XSTATS;
>   
> -	for (i = 0; i < size && i < RTE_DIM(mrvl_xstats_tbl); i++)
> +	for (i = 0; i < size && i < MRVL_NUM_XSTATS; i++)
>   		strlcpy(xstats_names[i].name, mrvl_xstats_tbl[i].name,
>   			RTE_ETH_XSTATS_NAME_SIZE);
>   
> @@ -2015,6 +2017,94 @@ mrvl_eth_filter_ctrl(struct rte_eth_dev *dev __rte_unused,
>   	}
>   }
>   
> +/**
> + * DPDK callback to get xstats by id.
> + *
> + * @param dev
> + *   Pointer to the device structure.
> + * @param ids
> + *   Pointer to the ids table.
> + * @param values
> + *   Pointer to the values table.
> + * @param n
> + *   Values table size.
> + * @returns
> + *   Number of read values, negative value otherwise.
> + */
> +static int
> +mrvl_xstats_get_by_id(struct rte_eth_dev *dev, const uint64_t *ids,
> +		      uint64_t *values, unsigned int n)
> +{
> +	struct rte_eth_xstat xstats[MRVL_NUM_XSTATS];
> +	uint16_t i;
> +
> +	if (n < MRVL_NUM_XSTATS && ids == NULL)
> +		return MRVL_NUM_XSTATS;
> +
> +	if (n > MRVL_NUM_XSTATS)
> +		return -EINVAL;

Why 'n > MRVL_NUM_XSTATS' is an error? I am not aware of this kind of 
restriction, can you please point if I am missing it.

> +
> +	if (values == NULL)
> +		return -ENOMEM;
> +
> +	mrvl_xstats_get(dev, xstats, n);
> +

What if 'n' is a small number, like in a case only single id is requested, so 
n==1, will above work?

Overall there is ethdev level implementation of "_by_id" APIs, if driver has 
'.xstats_get' & '.xstats_get_names' implemented.
Unless driver has more performant way to get these ids, I suggest using the 
ethdev layer ones, and drop these.

> +	for (i = 0; i < MRVL_NUM_XSTATS; i++) {
> +		if (ids[i] >= MRVL_NUM_XSTATS) {
> +			MRVL_LOG(ERR, "id value is not valid\n");
> +			return -EINVAL;
> +		}
> +		values[i] = xstats[ids[i]].value;
> +	}

Why this loop goes up to 'MRVL_NUM_XSTATS', does it assumes "n == 
MRVL_NUM_XSTATS", if so this assumption can be wrong.

> +
> +	return n;
> +}
> +
> +/**
> + * DPDK callback to get xstats names by ids.
> + *
> + * @param dev
> + *   Pointer to the device structure.
> + * @param xstats_names
> + *   Pointer to table with xstats names.
> + * @param ids
> + *   Pointer to table with ids.
> + * @param size
> + *   Xstats names table size.
> + * @returns
> + *   Number of names read, negative value otherwise.
> + */
> +static int
> +mrvl_xstats_get_names_by_id(struct rte_eth_dev *dev,
> +			    struct rte_eth_xstat_name *xstats_names,
> +			    const uint64_t *ids, unsigned int size)
> +{
> +	struct rte_eth_xstat_name names[MRVL_NUM_XSTATS];
> +	uint16_t i;
> +
> +	if (size < MRVL_NUM_XSTATS && ids == NULL)
> +		return MRVL_NUM_XSTATS;
> +
> +	if (size > MRVL_NUM_XSTATS)
> +		return -EINVAL;
> +
> +	if (xstats_names == NULL)
> +		return -ENOMEM;
> +
> +	mrvl_xstats_get_names(dev, names, size);
> +
> +	for (i = 0; i < MRVL_NUM_XSTATS; i++) {
> +		if (ids[i] >= MRVL_NUM_XSTATS) {
> +			MRVL_LOG(ERR, "id value is not valid");
> +			return -EINVAL;
> +		}
> +		strncpy(xstats_names[i].name, names[ids[i]].name,
> +			sizeof(xstats_names[i].name));
> +	}
> +
> +	return size;
> +}
> +
>   /**
>    * DPDK callback to get rte_mtr callbacks.
>    *
> @@ -2090,6 +2180,8 @@ static const struct eth_dev_ops mrvl_ops = {
>   	.rss_hash_update = mrvl_rss_hash_update,
>   	.rss_hash_conf_get = mrvl_rss_hash_conf_get,
>   	.filter_ctrl = mrvl_eth_filter_ctrl,
> +	.xstats_get_by_id = mrvl_xstats_get_by_id,
> +	.xstats_get_names_by_id = mrvl_xstats_get_names_by_id,
>   	.mtr_ops_get = mrvl_mtr_ops_get,
>   	.tm_ops_get = mrvl_tm_ops_get,
>   };
>
Liron Himi Jan. 27, 2021, 2 p.m. UTC | #2 
-----Original Message-----
From: Ferruh Yigit <ferruh.yigit@intel.com> 
Sent: Tuesday, 26 January 2021 20:27
To: Liron Himi <lironh@marvell.com>; Jerin Jacob Kollanukkaran <jerinj@marvell.com>
Cc: dev@dpdk.org; Yuri Chipchev <yuric@marvell.com>
Subject: [EXT] Re: [dpdk-dev] [PATCH v2 08/37] net/mvpp2: extend xstats support

External Email

----------------------------------------------------------------------
On 1/22/2021 7:18 PM, lironh@marvell.com wrote:
> From: Yuri Chipchev <yuric@marvell.com>
> 
> Add xstats_by_id callbacks
> 
> Signed-off-by: Yuri Chipchev <yuric@marvell.com>
> Reviewed-by: Liron Himi <lironh@marvell.com>
> ---
>   drivers/net/mvpp2/mrvl_ethdev.c | 98 ++++++++++++++++++++++++++++++++-
>   1 file changed, 95 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/mvpp2/mrvl_ethdev.c 
> b/drivers/net/mvpp2/mrvl_ethdev.c index e4ec343d5..ba1882266 100644
> --- a/drivers/net/mvpp2/mrvl_ethdev.c
> +++ b/drivers/net/mvpp2/mrvl_ethdev.c
> @@ -173,6 +173,8 @@ static struct {
>   	MRVL_XSTATS_TBL_ENTRY(tx_errors)
>   };
>   
> +#define MRVL_NUM_XSTATS RTE_DIM(mrvl_xstats_tbl)
> +
>   static inline void
>   mrvl_fill_shadowq(struct mrvl_shadow_txq *sq, struct rte_mbuf *buf)
>   {
> @@ -1376,7 +1378,7 @@ mrvl_xstats_get(struct rte_eth_dev *dev,
>   		return 0;
>   
>   	pp2_ppio_get_statistics(priv->ppio, &ppio_stats, 0);
> -	for (i = 0; i < n && i < RTE_DIM(mrvl_xstats_tbl); i++) {
> +	for (i = 0; i < n && i < MRVL_NUM_XSTATS; i++) {
>   		uint64_t val;
>   
>   		if (mrvl_xstats_tbl[i].size == sizeof(uint32_t)) @@ -1430,9 
> +1432,9 @@ mrvl_xstats_get_names(struct rte_eth_dev *dev __rte_unused,
>   	unsigned int i;
>   
>   	if (!xstats_names)
> -		return RTE_DIM(mrvl_xstats_tbl);
> +		return MRVL_NUM_XSTATS;
>   
> -	for (i = 0; i < size && i < RTE_DIM(mrvl_xstats_tbl); i++)
> +	for (i = 0; i < size && i < MRVL_NUM_XSTATS; i++)
>   		strlcpy(xstats_names[i].name, mrvl_xstats_tbl[i].name,
>   			RTE_ETH_XSTATS_NAME_SIZE);
>   
> @@ -2015,6 +2017,94 @@ mrvl_eth_filter_ctrl(struct rte_eth_dev *dev __rte_unused,
>   	}
>   }
>   
> +/**
> + * DPDK callback to get xstats by id.
> + *
> + * @param dev
> + *   Pointer to the device structure.
> + * @param ids
> + *   Pointer to the ids table.
> + * @param values
> + *   Pointer to the values table.
> + * @param n
> + *   Values table size.
> + * @returns
> + *   Number of read values, negative value otherwise.
> + */
> +static int
> +mrvl_xstats_get_by_id(struct rte_eth_dev *dev, const uint64_t *ids,
> +		      uint64_t *values, unsigned int n) {
> +	struct rte_eth_xstat xstats[MRVL_NUM_XSTATS];
> +	uint16_t i;
> +
> +	if (n < MRVL_NUM_XSTATS && ids == NULL)
> +		return MRVL_NUM_XSTATS;
> +
> +	if (n > MRVL_NUM_XSTATS)
> +		return -EINVAL;

Why 'n > MRVL_NUM_XSTATS' is an error? I am not aware of this kind of restriction, can you please point if I am missing it.

> +
> +	if (values == NULL)
> +		return -ENOMEM;
> +
> +	mrvl_xstats_get(dev, xstats, n);
> +

What if 'n' is a small number, like in a case only single id is requested, so n==1, will above work?

Overall there is ethdev level implementation of "_by_id" APIs, if driver has '.xstats_get' & '.xstats_get_names' implemented.
Unless driver has more performant way to get these ids, I suggest using the ethdev layer ones, and drop these.
[L.H.] missed that. In that case I will drop this patch in v3

> +	for (i = 0; i < MRVL_NUM_XSTATS; i++) {
> +		if (ids[i] >= MRVL_NUM_XSTATS) {
> +			MRVL_LOG(ERR, "id value is not valid\n");
> +			return -EINVAL;
> +		}
> +		values[i] = xstats[ids[i]].value;
> +	}

Why this loop goes up to 'MRVL_NUM_XSTATS', does it assumes "n == MRVL_NUM_XSTATS", if so this assumption can be wrong.

> +
> +	return n;
> +}
> +
> +/**
> + * DPDK callback to get xstats names by ids.
> + *
> + * @param dev
> + *   Pointer to the device structure.
> + * @param xstats_names
> + *   Pointer to table with xstats names.
> + * @param ids
> + *   Pointer to table with ids.
> + * @param size
> + *   Xstats names table size.
> + * @returns
> + *   Number of names read, negative value otherwise.
> + */
> +static int
> +mrvl_xstats_get_names_by_id(struct rte_eth_dev *dev,
> +			    struct rte_eth_xstat_name *xstats_names,
> +			    const uint64_t *ids, unsigned int size) {
> +	struct rte_eth_xstat_name names[MRVL_NUM_XSTATS];
> +	uint16_t i;
> +
> +	if (size < MRVL_NUM_XSTATS && ids == NULL)
> +		return MRVL_NUM_XSTATS;
> +
> +	if (size > MRVL_NUM_XSTATS)
> +		return -EINVAL;
> +
> +	if (xstats_names == NULL)
> +		return -ENOMEM;
> +
> +	mrvl_xstats_get_names(dev, names, size);
> +
> +	for (i = 0; i < MRVL_NUM_XSTATS; i++) {
> +		if (ids[i] >= MRVL_NUM_XSTATS) {
> +			MRVL_LOG(ERR, "id value is not valid");
> +			return -EINVAL;
> +		}
> +		strncpy(xstats_names[i].name, names[ids[i]].name,
> +			sizeof(xstats_names[i].name));
> +	}
> +
> +	return size;
> +}
> +
>   /**
>    * DPDK callback to get rte_mtr callbacks.
>    *
> @@ -2090,6 +2180,8 @@ static const struct eth_dev_ops mrvl_ops = {
>   	.rss_hash_update = mrvl_rss_hash_update,
>   	.rss_hash_conf_get = mrvl_rss_hash_conf_get,
>   	.filter_ctrl = mrvl_eth_filter_ctrl,
> +	.xstats_get_by_id = mrvl_xstats_get_by_id,
> +	.xstats_get_names_by_id = mrvl_xstats_get_names_by_id,
>   	.mtr_ops_get = mrvl_mtr_ops_get,
>   	.tm_ops_get = mrvl_tm_ops_get,
>   };
>
diff mbox series

Patch

diff --git a/drivers/net/mvpp2/mrvl_ethdev.c b/drivers/net/mvpp2/mrvl_ethdev.c
index e4ec343d5..ba1882266 100644
--- a/drivers/net/mvpp2/mrvl_ethdev.c
+++ b/drivers/net/mvpp2/mrvl_ethdev.c
@@ -173,6 +173,8 @@  static struct {
 	MRVL_XSTATS_TBL_ENTRY(tx_errors)
 };
 
+#define MRVL_NUM_XSTATS RTE_DIM(mrvl_xstats_tbl)
+
 static inline void
 mrvl_fill_shadowq(struct mrvl_shadow_txq *sq, struct rte_mbuf *buf)
 {
@@ -1376,7 +1378,7 @@  mrvl_xstats_get(struct rte_eth_dev *dev,
 		return 0;
 
 	pp2_ppio_get_statistics(priv->ppio, &ppio_stats, 0);
-	for (i = 0; i < n && i < RTE_DIM(mrvl_xstats_tbl); i++) {
+	for (i = 0; i < n && i < MRVL_NUM_XSTATS; i++) {
 		uint64_t val;
 
 		if (mrvl_xstats_tbl[i].size == sizeof(uint32_t))
@@ -1430,9 +1432,9 @@  mrvl_xstats_get_names(struct rte_eth_dev *dev __rte_unused,
 	unsigned int i;
 
 	if (!xstats_names)
-		return RTE_DIM(mrvl_xstats_tbl);
+		return MRVL_NUM_XSTATS;
 
-	for (i = 0; i < size && i < RTE_DIM(mrvl_xstats_tbl); i++)
+	for (i = 0; i < size && i < MRVL_NUM_XSTATS; i++)
 		strlcpy(xstats_names[i].name, mrvl_xstats_tbl[i].name,
 			RTE_ETH_XSTATS_NAME_SIZE);
 
@@ -2015,6 +2017,94 @@  mrvl_eth_filter_ctrl(struct rte_eth_dev *dev __rte_unused,
 	}
 }
 
+/**
+ * DPDK callback to get xstats by id.
+ *
+ * @param dev
+ *   Pointer to the device structure.
+ * @param ids
+ *   Pointer to the ids table.
+ * @param values
+ *   Pointer to the values table.
+ * @param n
+ *   Values table size.
+ * @returns
+ *   Number of read values, negative value otherwise.
+ */
+static int
+mrvl_xstats_get_by_id(struct rte_eth_dev *dev, const uint64_t *ids,
+		      uint64_t *values, unsigned int n)
+{
+	struct rte_eth_xstat xstats[MRVL_NUM_XSTATS];
+	uint16_t i;
+
+	if (n < MRVL_NUM_XSTATS && ids == NULL)
+		return MRVL_NUM_XSTATS;
+
+	if (n > MRVL_NUM_XSTATS)
+		return -EINVAL;
+
+	if (values == NULL)
+		return -ENOMEM;
+
+	mrvl_xstats_get(dev, xstats, n);
+
+	for (i = 0; i < MRVL_NUM_XSTATS; i++) {
+		if (ids[i] >= MRVL_NUM_XSTATS) {
+			MRVL_LOG(ERR, "id value is not valid\n");
+			return -EINVAL;
+		}
+		values[i] = xstats[ids[i]].value;
+	}
+
+	return n;
+}
+
+/**
+ * DPDK callback to get xstats names by ids.
+ *
+ * @param dev
+ *   Pointer to the device structure.
+ * @param xstats_names
+ *   Pointer to table with xstats names.
+ * @param ids
+ *   Pointer to table with ids.
+ * @param size
+ *   Xstats names table size.
+ * @returns
+ *   Number of names read, negative value otherwise.
+ */
+static int
+mrvl_xstats_get_names_by_id(struct rte_eth_dev *dev,
+			    struct rte_eth_xstat_name *xstats_names,
+			    const uint64_t *ids, unsigned int size)
+{
+	struct rte_eth_xstat_name names[MRVL_NUM_XSTATS];
+	uint16_t i;
+
+	if (size < MRVL_NUM_XSTATS && ids == NULL)
+		return MRVL_NUM_XSTATS;
+
+	if (size > MRVL_NUM_XSTATS)
+		return -EINVAL;
+
+	if (xstats_names == NULL)
+		return -ENOMEM;
+
+	mrvl_xstats_get_names(dev, names, size);
+
+	for (i = 0; i < MRVL_NUM_XSTATS; i++) {
+		if (ids[i] >= MRVL_NUM_XSTATS) {
+			MRVL_LOG(ERR, "id value is not valid");
+			return -EINVAL;
+		}
+		strncpy(xstats_names[i].name, names[ids[i]].name,
+			sizeof(xstats_names[i].name));
+	}
+
+	return size;
+}
+
 /**
  * DPDK callback to get rte_mtr callbacks.
  *
@@ -2090,6 +2180,8 @@  static const struct eth_dev_ops mrvl_ops = {
 	.rss_hash_update = mrvl_rss_hash_update,
 	.rss_hash_conf_get = mrvl_rss_hash_conf_get,
 	.filter_ctrl = mrvl_eth_filter_ctrl,
+	.xstats_get_by_id = mrvl_xstats_get_by_id,
+	.xstats_get_names_by_id = mrvl_xstats_get_names_by_id,
 	.mtr_ops_get = mrvl_mtr_ops_get,
 	.tm_ops_get = mrvl_tm_ops_get,
 };