[PATCH v5 0/5] support dmadev/ethdev stats reset

[PATCH v5 0/5] support dmadev/ethdev stats reset

[Chengwen Feng]

This patchset contains dmadev/ethdev stats reset, and also support
hide zero for ethdev xstats.

Chengwen Feng (5):
  dmadev: support stats reset telemetry command
  telemetry: fix repeated display when callback don't set dict
  ethdev: support xstats reset telemetry command
  ethdev: telemetry xstats support hide zero
  ethdev: add newline to telemetry log string

 lib/dmadev/rte_dmadev.c   | 40 +++++++++++++++++++++++++++
 lib/ethdev/rte_ethdev.c   | 57 ++++++++++++++++++++++++++++++++-------
 lib/telemetry/telemetry.c |  4 ++-
 3 files changed, 91 insertions(+), 10 deletions(-)

-- 
2.17.1

[PATCH v5 1/5] dmadev: support stats reset telemetry command

[Chengwen Feng]

The stats reset is useful for debugging, so add it to the dmadev
telemetry command lists.

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 lib/dmadev/rte_dmadev.c | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/lib/dmadev/rte_dmadev.c b/lib/dmadev/rte_dmadev.c
index 4da653eec7..93e15d4972 100644
--- a/lib/dmadev/rte_dmadev.c
+++ b/lib/dmadev/rte_dmadev.c
@@ -994,6 +994,44 @@ dmadev_handle_dev_stats(const char *cmd __rte_unused,
 	return 0;
 }
 
+static int
+dmadev_handle_dev_stats_reset(const char *cmd __rte_unused,
+		const char *params,
+		struct rte_tel_data *d __rte_unused)
+{
+	struct rte_dma_info dma_info;
+	int dev_id, ret, vchan_id;
+	const char *vchan_param;
+	char *end_param;
+
+	if (params == NULL || strlen(params) == 0 || !isdigit(*params))
+		return -EINVAL;
+
+	dev_id = strtoul(params, &end_param, 0);
+
+	/* Function info_get validates dev_id so we don't need to. */
+	ret = rte_dma_info_get(dev_id, &dma_info);
+	if (ret < 0)
+		return -EINVAL;
+
+	/* If the device has one vchan the user does not need to supply the
+	 * vchan id and only the device id is needed, no extra parameters.
+	 */
+	if (dma_info.nb_vchans == 1 && *end_param == '\0')
+		vchan_id = 0;
+	else {
+		vchan_param = strtok(end_param, ",");
+		if (!vchan_param || strlen(vchan_param) == 0 || !isdigit(*vchan_param))
+			return -EINVAL;
+
+		vchan_id = strtoul(vchan_param, &end_param, 0);
+	}
+	if (*end_param != '\0')
+		RTE_DMA_LOG(WARNING, "Extra parameters passed to dmadev telemetry command, ignoring");
+
+	return rte_dma_stats_reset(dev_id, vchan_id);
+}
+
 #ifndef RTE_EXEC_ENV_WINDOWS
 static int
 dmadev_handle_dev_dump(const char *cmd __rte_unused,
@@ -1041,6 +1079,8 @@ RTE_INIT(dmadev_init_telemetry)
 			"Returns information for a dmadev. Parameters: int dev_id");
 	rte_telemetry_register_cmd("/dmadev/stats", dmadev_handle_dev_stats,
 			"Returns the stats for a dmadev vchannel. Parameters: int dev_id, vchan_id (Optional if only one vchannel)");
+	rte_telemetry_register_cmd("/dmadev/stats_reset", dmadev_handle_dev_stats_reset,
+			"Reset the stats for a dmadev vchannel. Parameters: int dev_id, vchan_id (Optional if only one vchannel)");
 #ifndef RTE_EXEC_ENV_WINDOWS
 	rte_telemetry_register_cmd("/dmadev/dump", dmadev_handle_dev_dump,
 			"Returns dump information for a dmadev. Parameters: int dev_id");
-- 
2.17.1

[PATCH v5 2/5] telemetry: fix repeated display when callback don't set dict

[Chengwen Feng]

When telemetry callback didn't set dict and return a non-negative
number, the telemetry will repeat to display the last result.

Fixes: 6dd571fd07c3 ("telemetry: introduce new functionality")
Cc: stable@dpdk.org

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 lib/telemetry/telemetry.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lib/telemetry/telemetry.c b/lib/telemetry/telemetry.c
index 8fbb4f3060..a6c84e3499 100644
--- a/lib/telemetry/telemetry.c
+++ b/lib/telemetry/telemetry.c
@@ -334,8 +334,10 @@ static void
 perform_command(telemetry_cb fn, const char *cmd, const char *param, int s)
 {
 	struct rte_tel_data data;
+	int ret;
 
-	int ret = fn(cmd, param, &data);
+	rte_tel_data_start_dict(&data);
+	ret = fn(cmd, param, &data);
 	if (ret < 0) {
 		char out_buf[MAX_CMD_LEN + 10];
 		int used = snprintf(out_buf, sizeof(out_buf), "{\"%.*s\":null}",
-- 
2.17.1

[PATCH v5 3/5] ethdev: support xstats reset telemetry command

[Chengwen Feng]

The xstats reset is useful for debugging, so add it to the ethdev
telemetry command lists.

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 lib/ethdev/rte_ethdev.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index 5d5e18db1e..0774de81b0 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -5915,6 +5915,27 @@ eth_dev_handle_port_xstats(const char *cmd __rte_unused,
 	return 0;
 }
 
+static int
+eth_dev_handle_port_xstats_reset(const char *cmd __rte_unused,
+		const char *params,
+		struct rte_tel_data *d __rte_unused)
+{
+	char *end_param;
+	int port_id;
+
+	if (params == NULL || strlen(params) == 0 || !isdigit(*params))
+		return -1;
+
+	port_id = strtoul(params, &end_param, 0);
+	if (*end_param != '\0')
+		RTE_ETHDEV_LOG(NOTICE,
+			"Extra parameters passed to ethdev telemetry command, ignoring\n");
+	if (!rte_eth_dev_is_valid_port(port_id))
+		return -1;
+
+	return rte_eth_xstats_reset(port_id);
+}
+
 #ifndef RTE_EXEC_ENV_WINDOWS
 static int
 eth_dev_handle_port_dump_priv(const char *cmd __rte_unused,
@@ -6328,6 +6349,8 @@ RTE_INIT(ethdev_init_telemetry)
 			"Returns the common stats for a port. Parameters: int port_id");
 	rte_telemetry_register_cmd("/ethdev/xstats", eth_dev_handle_port_xstats,
 			"Returns the extended stats for a port. Parameters: int port_id");
+	rte_telemetry_register_cmd("/ethdev/xstats_reset", eth_dev_handle_port_xstats_reset,
+			"Reset the extended stats for a port. Parameters: int port_id");
 #ifndef RTE_EXEC_ENV_WINDOWS
 	rte_telemetry_register_cmd("/ethdev/dump_priv", eth_dev_handle_port_dump_priv,
 			"Returns dump private information for a port. Parameters: int port_id");
-- 
2.17.1

[PATCH v5 4/5] ethdev: telemetry xstats support hide zero

[Chengwen Feng]

The number of xstats may be large, after the hide zero option is added,
only non-zero values can be displayed.

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 lib/ethdev/rte_ethdev.c | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index 0774de81b0..2075086eeb 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -5870,20 +5870,33 @@ eth_dev_handle_port_xstats(const char *cmd __rte_unused,
 {
 	struct rte_eth_xstat *eth_xstats;
 	struct rte_eth_xstat_name *xstat_names;
+	char *end_param, *hide_param;
 	int port_id, num_xstats;
+	int hide_zero = 0;
 	int i, ret;
-	char *end_param;
 
 	if (params == NULL || strlen(params) == 0 || !isdigit(*params))
 		return -1;
 
 	port_id = strtoul(params, &end_param, 0);
-	if (*end_param != '\0')
-		RTE_ETHDEV_LOG(NOTICE,
-			"Extra parameters passed to ethdev telemetry command, ignoring");
 	if (!rte_eth_dev_is_valid_port(port_id))
 		return -1;
 
+	if (*end_param != '\0') {
+		hide_param = strtok(end_param, ",");
+		if (!hide_param || strlen(hide_param) == 0 || !isdigit(*hide_param))
+			return -EINVAL;
+		hide_zero = strtoul(hide_param, &end_param, 0);
+		if (*end_param != '\0')
+			RTE_ETHDEV_LOG(NOTICE,
+				"Extra parameters passed to ethdev telemetry command, ignoring\n");
+		if (hide_zero != 0 && hide_zero != 1) {
+			hide_zero = !!hide_zero;
+			RTE_ETHDEV_LOG(NOTICE,
+				"Hide zero parameter is non-boolean, cast to boolean\n");
+		}
+	}
+
 	num_xstats = rte_eth_xstats_get(port_id, NULL, 0);
 	if (num_xstats < 0)
 		return -1;
@@ -5908,9 +5921,12 @@ eth_dev_handle_port_xstats(const char *cmd __rte_unused,
 	}
 
 	rte_tel_data_start_dict(d);
-	for (i = 0; i < num_xstats; i++)
+	for (i = 0; i < num_xstats; i++) {
+		if (hide_zero && eth_xstats[i].value == 0)
+			continue;
 		rte_tel_data_add_dict_u64(d, xstat_names[i].name,
 				eth_xstats[i].value);
+	}
 	free(eth_xstats);
 	return 0;
 }
@@ -6348,7 +6364,7 @@ RTE_INIT(ethdev_init_telemetry)
 	rte_telemetry_register_cmd("/ethdev/stats", eth_dev_handle_port_stats,
 			"Returns the common stats for a port. Parameters: int port_id");
 	rte_telemetry_register_cmd("/ethdev/xstats", eth_dev_handle_port_xstats,
-			"Returns the extended stats for a port. Parameters: int port_id");
+			"Returns the extended stats for a port. Parameters: int port_id, hide_zero (Optional, specify whether to hide zero values)");
 	rte_telemetry_register_cmd("/ethdev/xstats_reset", eth_dev_handle_port_xstats_reset,
 			"Reset the extended stats for a port. Parameters: int port_id");
 #ifndef RTE_EXEC_ENV_WINDOWS
-- 
2.17.1

[PATCH v5 5/5] ethdev: add newline to telemetry log string

[Chengwen Feng]

The telemetry related code may invoke RTE_ETHDEV_LOG to display
information, the newline character is not added automatically to the
RTE_ETHDEV_LOG, therefore, the newline character must be explicitly
added to the log string.

Cc: stable@dpdk.org

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 lib/ethdev/rte_ethdev.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index 2075086eeb..30f79b7d4c 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -5968,7 +5968,7 @@ eth_dev_handle_port_dump_priv(const char *cmd __rte_unused,
 	port_id = strtoul(params, &end_param, 0);
 	if (*end_param != '\0')
 		RTE_ETHDEV_LOG(NOTICE,
-			"Extra parameters passed to ethdev telemetry command, ignoring");
+			"Extra parameters passed to ethdev telemetry command, ignoring\n");
 	if (!rte_eth_dev_is_valid_port(port_id))
 		return -EINVAL;
 
@@ -6010,7 +6010,7 @@ eth_dev_handle_port_link_status(const char *cmd __rte_unused,
 	port_id = strtoul(params, &end_param, 0);
 	if (*end_param != '\0')
 		RTE_ETHDEV_LOG(NOTICE,
-			"Extra parameters passed to ethdev telemetry command, ignoring");
+			"Extra parameters passed to ethdev telemetry command, ignoring\n");
 	if (!rte_eth_dev_is_valid_port(port_id))
 		return -1;
 
@@ -6048,7 +6048,7 @@ eth_dev_handle_port_info(const char *cmd __rte_unused,
 	port_id = strtoul(params, &end_param, 0);
 	if (*end_param != '\0')
 		RTE_ETHDEV_LOG(NOTICE,
-			"Extra parameters passed to ethdev telemetry command, ignoring");
+			"Extra parameters passed to ethdev telemetry command, ignoring\n");
 
 	if (!rte_eth_dev_is_valid_port(port_id))
 		return -EINVAL;
-- 
2.17.1

Re: [PATCH v5 2/5] telemetry: fix repeated display when callback don't set dict

[Bruce Richardson]

On Mon, Dec 19, 2022 at 09:07:20AM +0000, Chengwen Feng wrote:
> When telemetry callback didn't set dict and return a non-negative
> number, the telemetry will repeat to display the last result.
> 
> Fixes: 6dd571fd07c3 ("telemetry: introduce new functionality")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> ---

Hi Chengwen,

I'm a little curious about this bug. Can you describe some steps to
reproduce it as I'm curious as to exactly what is happening. The fix seems
a little strange to me so I'd like to investigate a little more to see if
other approaches might work.

Thanks,
/Bruce

Re: [PATCH v5 2/5] telemetry: fix repeated display when callback don't set dict

[fengchengwen]

On 2022/12/19 17:33, Bruce Richardson wrote:
> On Mon, Dec 19, 2022 at 09:07:20AM +0000, Chengwen Feng wrote:
>> When telemetry callback didn't set dict and return a non-negative
>> number, the telemetry will repeat to display the last result.
>>
>> Fixes: 6dd571fd07c3 ("telemetry: introduce new functionality")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>> ---
> 
> Hi Chengwen,
> 
> I'm a little curious about this bug. Can you describe some steps to
> reproduce it as I'm curious as to exactly what is happening. The fix seems
> a little strange to me so I'd like to investigate a little more to see if
> other approaches might work.

Hi Bruce,

Sorry for late reply.

The steps:
  1. applay "[PATCH v5 1/5] dmadev: support stats reset telemetry command"
  2. compile
  3. start dpdk-dma: dpdk-dma -a DMA.BDF -a NIC.BDF -- -c hw
  4. start telemetry, and execute /dmadev/stats,0, and then /dmadev/stats_reset,0
     the output of /dmadev/stats_reset,0 will be the same of previous cmd "/dmadev/stats,0"
     e.g. my environment:
--> /dmadev/stats,0
{
  "/dmadev/stats": {
    "submitted": 23,
    "completed": 23,
    "errors": 0
  }
}
--> /dmadev/stats_reset,0
{
  "/dmadev/stats_reset": {
    "submitted": 23,
    "completed": 23,
    "errors": 0
  }
}

The rootcause is that the /dmadev/stats_reset don't set the outer parameter "struct rte_tel_data *info"
and return zero.


BTW: although the telemetry mainly used to query, but some reset counter maybe usefull, and it already
exist like: "/eventdev/rxa_stats_reset" and this patchset.


> 
> Thanks,
> /Bruce
> .
> 

Re: [PATCH v5 0/5] support dmadev/ethdev stats reset

[fengchengwen]

Sorry for the patchset version set wrong, it should be 'PATCH v1' not 'PATCH v5'

Will fix in V2

On 2022/12/19 17:07, Chengwen Feng wrote:
> This patchset contains dmadev/ethdev stats reset, and also support
> hide zero for ethdev xstats.
> 
> Chengwen Feng (5):
>   dmadev: support stats reset telemetry command
>   telemetry: fix repeated display when callback don't set dict
>   ethdev: support xstats reset telemetry command
>   ethdev: telemetry xstats support hide zero
>   ethdev: add newline to telemetry log string
> 
>  lib/dmadev/rte_dmadev.c   | 40 +++++++++++++++++++++++++++
>  lib/ethdev/rte_ethdev.c   | 57 ++++++++++++++++++++++++++++++++-------
>  lib/telemetry/telemetry.c |  4 ++-
>  3 files changed, 91 insertions(+), 10 deletions(-)
> 

Re: [PATCH v5 2/5] telemetry: fix repeated display when callback don't set dict

[Bruce Richardson]

On Mon, Dec 26, 2022 at 12:53:57PM +0800, fengchengwen wrote:
> On 2022/12/19 17:33, Bruce Richardson wrote:
> > On Mon, Dec 19, 2022 at 09:07:20AM +0000, Chengwen Feng wrote:
> >> When telemetry callback didn't set dict and return a non-negative
> >> number, the telemetry will repeat to display the last result.
> >>
> >> Fixes: 6dd571fd07c3 ("telemetry: introduce new functionality")
> >> Cc: stable@dpdk.org
> >>
> >> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> >> ---
> > 
> > Hi Chengwen,
> > 
> > I'm a little curious about this bug. Can you describe some steps to
> > reproduce it as I'm curious as to exactly what is happening. The fix seems
> > a little strange to me so I'd like to investigate a little more to see if
> > other approaches might work.
> 
> Hi Bruce,
> 
> Sorry for late reply.
> 
> The steps:
>   1. applay "[PATCH v5 1/5] dmadev: support stats reset telemetry command"
>   2. compile
>   3. start dpdk-dma: dpdk-dma -a DMA.BDF -a NIC.BDF -- -c hw
>   4. start telemetry, and execute /dmadev/stats,0, and then /dmadev/stats_reset,0
>      the output of /dmadev/stats_reset,0 will be the same of previous cmd "/dmadev/stats,0"
>      e.g. my environment:
> --> /dmadev/stats,0
> {
>   "/dmadev/stats": {
>     "submitted": 23,
>     "completed": 23,
>     "errors": 0
>   }
> }
> --> /dmadev/stats_reset,0
> {
>   "/dmadev/stats_reset": {
>     "submitted": 23,
>     "completed": 23,
>     "errors": 0
>   }
> }
> 
> The rootcause is that the /dmadev/stats_reset don't set the outer parameter "struct rte_tel_data *info"
> and return zero.
>
Thanks for the fuller explanation, I'll hopefully test it out myself.

However, in the meantime, looking at the telemetry library code, would the
following change work rather than explicitly always setting the telemetry
data to a dictionary by default? Zeroing the data by default sets it to a
null return which is what you probably want as default rather than an empty
dictionary. (And it's also a smaller diff)

/Bruce

diff --git a/lib/telemetry/telemetry.c b/lib/telemetry/telemetry.c
index 8fbb4f3060..7b905355cd 100644
--- a/lib/telemetry/telemetry.c
+++ b/lib/telemetry/telemetry.c
@@ -333,7 +333,7 @@ output_json(const char *cmd, const struct rte_tel_data *d, int s)
 static void
 perform_command(telemetry_cb fn, const char *cmd, const char *param, int s)
 {
-       struct rte_tel_data data;
+       struct rte_tel_data data = {0};
 
        int ret = fn(cmd, param, &data);
        if (ret < 0) {
 

Re: [PATCH v5 2/5] telemetry: fix repeated display when callback don't set dict

[Bruce Richardson]

On Fri, Jan 06, 2023 at 04:07:45PM +0000, Bruce Richardson wrote:
> On Mon, Dec 26, 2022 at 12:53:57PM +0800, fengchengwen wrote:
> > On 2022/12/19 17:33, Bruce Richardson wrote:
> > > On Mon, Dec 19, 2022 at 09:07:20AM +0000, Chengwen Feng wrote:
> > >> When telemetry callback didn't set dict and return a non-negative
> > >> number, the telemetry will repeat to display the last result.
> > >>
> > >> Fixes: 6dd571fd07c3 ("telemetry: introduce new functionality")
> > >> Cc: stable@dpdk.org
> > >>
> > >> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> > >> ---
> > > 
> > > Hi Chengwen,
> > > 
> > > I'm a little curious about this bug. Can you describe some steps to
> > > reproduce it as I'm curious as to exactly what is happening. The fix seems
> > > a little strange to me so I'd like to investigate a little more to see if
> > > other approaches might work.
> > 
> > Hi Bruce,
> > 
> > Sorry for late reply.
> > 
> > The steps:
> >   1. applay "[PATCH v5 1/5] dmadev: support stats reset telemetry command"
> >   2. compile
> >   3. start dpdk-dma: dpdk-dma -a DMA.BDF -a NIC.BDF -- -c hw
> >   4. start telemetry, and execute /dmadev/stats,0, and then /dmadev/stats_reset,0
> >      the output of /dmadev/stats_reset,0 will be the same of previous cmd "/dmadev/stats,0"
> >      e.g. my environment:
> > --> /dmadev/stats,0
> > {
> >   "/dmadev/stats": {
> >     "submitted": 23,
> >     "completed": 23,
> >     "errors": 0
> >   }
> > }
> > --> /dmadev/stats_reset,0
> > {
> >   "/dmadev/stats_reset": {
> >     "submitted": 23,
> >     "completed": 23,
> >     "errors": 0
> >   }
> > }
> > 
> > The rootcause is that the /dmadev/stats_reset don't set the outer parameter "struct rte_tel_data *info"
> > and return zero.
> >
> Thanks for the fuller explanation, I'll hopefully test it out myself.
> 
> However, in the meantime, looking at the telemetry library code, would the
> following change work rather than explicitly always setting the telemetry
> data to a dictionary by default? Zeroing the data by default sets it to a
> null return which is what you probably want as default rather than an empty
> dictionary. (And it's also a smaller diff)
> 
> /Bruce
> 
> diff --git a/lib/telemetry/telemetry.c b/lib/telemetry/telemetry.c
> index 8fbb4f3060..7b905355cd 100644
> --- a/lib/telemetry/telemetry.c
> +++ b/lib/telemetry/telemetry.c
> @@ -333,7 +333,7 @@ output_json(const char *cmd, const struct rte_tel_data *d, int s)
>  static void
>  perform_command(telemetry_cb fn, const char *cmd, const char *param, int s)
>  {
> -       struct rte_tel_data data;
> +       struct rte_tel_data data = {0};
>  
>         int ret = fn(cmd, param, &data);
>         if (ret < 0) {
>

I've handily reproduced the issue using the instructions you gave above,
thanks for those.

Based on that, and looking a little deeper:

* I think it is an error with the reset function not to initialize and
  complete the return value. I believe that for each telemetry callback we
  should require that the callback fill in a valid value on success. For
  reset, some options could be just a string "OK", or an array just
  containing "[0]", or similar.

* Given that point above, I do agree though that the "data" parameter
  should be properly initialized on entry to the callbacks. However, I feel
  that the correct init should be the empty/null value, as is given in the
  patch above.

Regards,
/Bruce

[PATCH v2 0/5] support dmadev/ethdev stats reset

[Chengwen Feng]

This patchset contains dmadev/ethdev stats reset, and also support
hide zero for ethdev xstats and two telemetry related bugs.

Chengwen Feng (5):
  dmadev: support stats reset telemetry command
  telemetry: fix repeat display when callback don't set dict
  ethdev: add newline to telemetry log string
  ethdev: support xstats reset telemetry command
  ethdev: telemetry xstats support hide zero

 lib/dmadev/rte_dmadev.c   | 46 ++++++++++++++++++++++++++++
 lib/ethdev/rte_ethdev.c   | 63 +++++++++++++++++++++++++++++++++------
 lib/telemetry/telemetry.c |  2 +-
 3 files changed, 101 insertions(+), 10 deletions(-)

-- 
2.17.1

[PATCH v2 1/5] dmadev: support stats reset telemetry command

[Chengwen Feng]

The stats reset is useful for debugging, so add it to the dmadev
telemetry command lists.

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 lib/dmadev/rte_dmadev.c | 46 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/lib/dmadev/rte_dmadev.c b/lib/dmadev/rte_dmadev.c
index 4da653eec7..79593cdbe2 100644
--- a/lib/dmadev/rte_dmadev.c
+++ b/lib/dmadev/rte_dmadev.c
@@ -994,6 +994,50 @@ dmadev_handle_dev_stats(const char *cmd __rte_unused,
 	return 0;
 }
 
+static int
+dmadev_handle_dev_stats_reset(const char *cmd __rte_unused,
+		const char *params,
+		struct rte_tel_data *d)
+{
+	struct rte_dma_info dma_info;
+	int dev_id, ret, vchan_id;
+	const char *vchan_param;
+	char *end_param;
+
+	if (params == NULL || strlen(params) == 0 || !isdigit(*params))
+		return -EINVAL;
+
+	dev_id = strtoul(params, &end_param, 0);
+
+	/* Function info_get validates dev_id so we don't need to. */
+	ret = rte_dma_info_get(dev_id, &dma_info);
+	if (ret < 0)
+		return -EINVAL;
+
+	/* If the device has one vchan the user does not need to supply the
+	 * vchan id and only the device id is needed, no extra parameters.
+	 */
+	if (dma_info.nb_vchans == 1 && *end_param == '\0')
+		vchan_id = 0;
+	else {
+		vchan_param = strtok(end_param, ",");
+		if (!vchan_param || strlen(vchan_param) == 0 || !isdigit(*vchan_param))
+			return -EINVAL;
+
+		vchan_id = strtoul(vchan_param, &end_param, 0);
+	}
+	if (*end_param != '\0')
+		RTE_DMA_LOG(WARNING, "Extra parameters passed to dmadev telemetry command, ignoring");
+
+	ret = rte_dma_stats_reset(dev_id, vchan_id);
+	if (ret == 0) {
+		rte_tel_data_start_dict(d);
+		rte_tel_data_string(d, "success");
+	}
+
+	return ret;
+}
+
 #ifndef RTE_EXEC_ENV_WINDOWS
 static int
 dmadev_handle_dev_dump(const char *cmd __rte_unused,
@@ -1041,6 +1085,8 @@ RTE_INIT(dmadev_init_telemetry)
 			"Returns information for a dmadev. Parameters: int dev_id");
 	rte_telemetry_register_cmd("/dmadev/stats", dmadev_handle_dev_stats,
 			"Returns the stats for a dmadev vchannel. Parameters: int dev_id, vchan_id (Optional if only one vchannel)");
+	rte_telemetry_register_cmd("/dmadev/stats_reset", dmadev_handle_dev_stats_reset,
+			"Reset the stats for a dmadev vchannel. Parameters: int dev_id, vchan_id (Optional if only one vchannel)");
 #ifndef RTE_EXEC_ENV_WINDOWS
 	rte_telemetry_register_cmd("/dmadev/dump", dmadev_handle_dev_dump,
 			"Returns dump information for a dmadev. Parameters: int dev_id");
-- 
2.17.1

[PATCH v2 2/5] telemetry: fix repeat display when callback don't set dict

[Chengwen Feng]

When telemetry callback didn't set dict and return a non-negative
number, the telemetry will repeat to display the last result. This
patch zero the dict to avoid the problem.

Fixes: 6dd571fd07c3 ("telemetry: introduce new functionality")
Cc: stable@dpdk.org

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 lib/telemetry/telemetry.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/telemetry/telemetry.c b/lib/telemetry/telemetry.c
index 8fbb4f3060..7b905355cd 100644
--- a/lib/telemetry/telemetry.c
+++ b/lib/telemetry/telemetry.c
@@ -333,7 +333,7 @@ output_json(const char *cmd, const struct rte_tel_data *d, int s)
 static void
 perform_command(telemetry_cb fn, const char *cmd, const char *param, int s)
 {
-	struct rte_tel_data data;
+	struct rte_tel_data data = {0};
 
 	int ret = fn(cmd, param, &data);
 	if (ret < 0) {
-- 
2.17.1

[PATCH v2 3/5] ethdev: add newline to telemetry log string

[Chengwen Feng]

The telemetry related code may invoke RTE_ETHDEV_LOG to display
information, the newline character is not added automatically to the
RTE_ETHDEV_LOG, therefore, the newline character must be explicitly
added to the log string.

Fixes: 5514319e7b43 ("telemetry: fix passing full params string to command")
Cc: stable@dpdk.org

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 lib/ethdev/rte_ethdev.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index 5d5e18db1e..9eeae61024 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -5880,7 +5880,7 @@ eth_dev_handle_port_xstats(const char *cmd __rte_unused,
 	port_id = strtoul(params, &end_param, 0);
 	if (*end_param != '\0')
 		RTE_ETHDEV_LOG(NOTICE,
-			"Extra parameters passed to ethdev telemetry command, ignoring");
+			"Extra parameters passed to ethdev telemetry command, ignoring\n");
 	if (!rte_eth_dev_is_valid_port(port_id))
 		return -1;
 
@@ -5931,7 +5931,7 @@ eth_dev_handle_port_dump_priv(const char *cmd __rte_unused,
 	port_id = strtoul(params, &end_param, 0);
 	if (*end_param != '\0')
 		RTE_ETHDEV_LOG(NOTICE,
-			"Extra parameters passed to ethdev telemetry command, ignoring");
+			"Extra parameters passed to ethdev telemetry command, ignoring\n");
 	if (!rte_eth_dev_is_valid_port(port_id))
 		return -EINVAL;
 
@@ -5973,7 +5973,7 @@ eth_dev_handle_port_link_status(const char *cmd __rte_unused,
 	port_id = strtoul(params, &end_param, 0);
 	if (*end_param != '\0')
 		RTE_ETHDEV_LOG(NOTICE,
-			"Extra parameters passed to ethdev telemetry command, ignoring");
+			"Extra parameters passed to ethdev telemetry command, ignoring\n");
 	if (!rte_eth_dev_is_valid_port(port_id))
 		return -1;
 
@@ -6011,7 +6011,7 @@ eth_dev_handle_port_info(const char *cmd __rte_unused,
 	port_id = strtoul(params, &end_param, 0);
 	if (*end_param != '\0')
 		RTE_ETHDEV_LOG(NOTICE,
-			"Extra parameters passed to ethdev telemetry command, ignoring");
+			"Extra parameters passed to ethdev telemetry command, ignoring\n");
 
 	if (!rte_eth_dev_is_valid_port(port_id))
 		return -EINVAL;
-- 
2.17.1

[PATCH v2 4/5] ethdev: support xstats reset telemetry command

[Chengwen Feng]

The xstats reset is useful for debugging, so add it to the ethdev
telemetry command lists.

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 lib/ethdev/rte_ethdev.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index 9eeae61024..2fc593b865 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -5915,6 +5915,33 @@ eth_dev_handle_port_xstats(const char *cmd __rte_unused,
 	return 0;
 }
 
+static int
+eth_dev_handle_port_xstats_reset(const char *cmd __rte_unused,
+		const char *params,
+		struct rte_tel_data *d)
+{
+	int port_id, ret;
+	char *end_param;
+
+	if (params == NULL || strlen(params) == 0 || !isdigit(*params))
+		return -1;
+
+	port_id = strtoul(params, &end_param, 0);
+	if (*end_param != '\0')
+		RTE_ETHDEV_LOG(NOTICE,
+			"Extra parameters passed to ethdev telemetry command, ignoring\n");
+	if (!rte_eth_dev_is_valid_port(port_id))
+		return -1;
+
+	ret = rte_eth_xstats_reset(port_id);
+	if (ret == 0) {
+		rte_tel_data_start_dict(d);
+		rte_tel_data_string(d, "success");
+	}
+
+	return ret;
+}
+
 #ifndef RTE_EXEC_ENV_WINDOWS
 static int
 eth_dev_handle_port_dump_priv(const char *cmd __rte_unused,
@@ -6328,6 +6355,8 @@ RTE_INIT(ethdev_init_telemetry)
 			"Returns the common stats for a port. Parameters: int port_id");
 	rte_telemetry_register_cmd("/ethdev/xstats", eth_dev_handle_port_xstats,
 			"Returns the extended stats for a port. Parameters: int port_id");
+	rte_telemetry_register_cmd("/ethdev/xstats_reset", eth_dev_handle_port_xstats_reset,
+			"Reset the extended stats for a port. Parameters: int port_id");
 #ifndef RTE_EXEC_ENV_WINDOWS
 	rte_telemetry_register_cmd("/ethdev/dump_priv", eth_dev_handle_port_dump_priv,
 			"Returns dump private information for a port. Parameters: int port_id");
-- 
2.17.1

[PATCH v2 5/5] ethdev: telemetry xstats support hide zero

[Chengwen Feng]

The number of xstats may be large, after the hide zero option is added,
only non-zero values can be displayed.

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 lib/ethdev/rte_ethdev.c | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index 2fc593b865..77cacc0829 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -5870,20 +5870,33 @@ eth_dev_handle_port_xstats(const char *cmd __rte_unused,
 {
 	struct rte_eth_xstat *eth_xstats;
 	struct rte_eth_xstat_name *xstat_names;
+	char *end_param, *hide_param;
 	int port_id, num_xstats;
+	int hide_zero = 0;
 	int i, ret;
-	char *end_param;
 
 	if (params == NULL || strlen(params) == 0 || !isdigit(*params))
 		return -1;
 
 	port_id = strtoul(params, &end_param, 0);
-	if (*end_param != '\0')
-		RTE_ETHDEV_LOG(NOTICE,
-			"Extra parameters passed to ethdev telemetry command, ignoring\n");
 	if (!rte_eth_dev_is_valid_port(port_id))
 		return -1;
 
+	if (*end_param != '\0') {
+		hide_param = strtok(end_param, ",");
+		if (!hide_param || strlen(hide_param) == 0 || !isdigit(*hide_param))
+			return -EINVAL;
+		hide_zero = strtoul(hide_param, &end_param, 0);
+		if (*end_param != '\0')
+			RTE_ETHDEV_LOG(NOTICE,
+				"Extra parameters passed to ethdev telemetry command, ignoring\n");
+		if (hide_zero != 0 && hide_zero != 1) {
+			hide_zero = !!hide_zero;
+			RTE_ETHDEV_LOG(NOTICE,
+				"Hide zero parameter is non-boolean, cast to boolean\n");
+		}
+	}
+
 	num_xstats = rte_eth_xstats_get(port_id, NULL, 0);
 	if (num_xstats < 0)
 		return -1;
@@ -5908,9 +5921,12 @@ eth_dev_handle_port_xstats(const char *cmd __rte_unused,
 	}
 
 	rte_tel_data_start_dict(d);
-	for (i = 0; i < num_xstats; i++)
+	for (i = 0; i < num_xstats; i++) {
+		if (hide_zero && eth_xstats[i].value == 0)
+			continue;
 		rte_tel_data_add_dict_u64(d, xstat_names[i].name,
 				eth_xstats[i].value);
+	}
 	free(eth_xstats);
 	return 0;
 }
@@ -6354,7 +6370,7 @@ RTE_INIT(ethdev_init_telemetry)
 	rte_telemetry_register_cmd("/ethdev/stats", eth_dev_handle_port_stats,
 			"Returns the common stats for a port. Parameters: int port_id");
 	rte_telemetry_register_cmd("/ethdev/xstats", eth_dev_handle_port_xstats,
-			"Returns the extended stats for a port. Parameters: int port_id");
+			"Returns the extended stats for a port. Parameters: int port_id, hide_zero (Optional, specify whether to hide zero values)");
 	rte_telemetry_register_cmd("/ethdev/xstats_reset", eth_dev_handle_port_xstats_reset,
 			"Reset the extended stats for a port. Parameters: int port_id");
 #ifndef RTE_EXEC_ENV_WINDOWS
-- 
2.17.1

[PATCH v2 0/5] support dmadev/ethdev stats reset

[Chengwen Feng]

This patchset contains dmadev/ethdev stats reset, and also support
hide zero for ethdev xstats and two telemetry related bugs.

Chengwen Feng (5):
  dmadev: support stats reset telemetry command
  telemetry: fix repeat display when callback don't set dict
  ethdev: add newline to telemetry log string
  ethdev: support xstats reset telemetry command
  ethdev: telemetry xstats support hide zero

---
v2: 
* address Bruce's comment on 2/5 patch.
* support output success when stats reset.

 lib/dmadev/rte_dmadev.c   | 46 ++++++++++++++++++++++++++++
 lib/ethdev/rte_ethdev.c   | 63 +++++++++++++++++++++++++++++++++------
 lib/telemetry/telemetry.c |  2 +-
 3 files changed, 101 insertions(+), 10 deletions(-)

-- 
2.17.1

[PATCH v2 1/5] dmadev: support stats reset telemetry command

[Chengwen Feng]

The stats reset is useful for debugging, so add it to the dmadev
telemetry command lists.

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 lib/dmadev/rte_dmadev.c | 46 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/lib/dmadev/rte_dmadev.c b/lib/dmadev/rte_dmadev.c
index 4da653eec7..79593cdbe2 100644
--- a/lib/dmadev/rte_dmadev.c
+++ b/lib/dmadev/rte_dmadev.c
@@ -994,6 +994,50 @@ dmadev_handle_dev_stats(const char *cmd __rte_unused,
 	return 0;
 }
 
+static int
+dmadev_handle_dev_stats_reset(const char *cmd __rte_unused,
+		const char *params,
+		struct rte_tel_data *d)
+{
+	struct rte_dma_info dma_info;
+	int dev_id, ret, vchan_id;
+	const char *vchan_param;
+	char *end_param;
+
+	if (params == NULL || strlen(params) == 0 || !isdigit(*params))
+		return -EINVAL;
+
+	dev_id = strtoul(params, &end_param, 0);
+
+	/* Function info_get validates dev_id so we don't need to. */
+	ret = rte_dma_info_get(dev_id, &dma_info);
+	if (ret < 0)
+		return -EINVAL;
+
+	/* If the device has one vchan the user does not need to supply the
+	 * vchan id and only the device id is needed, no extra parameters.
+	 */
+	if (dma_info.nb_vchans == 1 && *end_param == '\0')
+		vchan_id = 0;
+	else {
+		vchan_param = strtok(end_param, ",");
+		if (!vchan_param || strlen(vchan_param) == 0 || !isdigit(*vchan_param))
+			return -EINVAL;
+
+		vchan_id = strtoul(vchan_param, &end_param, 0);
+	}
+	if (*end_param != '\0')
+		RTE_DMA_LOG(WARNING, "Extra parameters passed to dmadev telemetry command, ignoring");
+
+	ret = rte_dma_stats_reset(dev_id, vchan_id);
+	if (ret == 0) {
+		rte_tel_data_start_dict(d);
+		rte_tel_data_string(d, "success");
+	}
+
+	return ret;
+}
+
 #ifndef RTE_EXEC_ENV_WINDOWS
 static int
 dmadev_handle_dev_dump(const char *cmd __rte_unused,
@@ -1041,6 +1085,8 @@ RTE_INIT(dmadev_init_telemetry)
 			"Returns information for a dmadev. Parameters: int dev_id");
 	rte_telemetry_register_cmd("/dmadev/stats", dmadev_handle_dev_stats,
 			"Returns the stats for a dmadev vchannel. Parameters: int dev_id, vchan_id (Optional if only one vchannel)");
+	rte_telemetry_register_cmd("/dmadev/stats_reset", dmadev_handle_dev_stats_reset,
+			"Reset the stats for a dmadev vchannel. Parameters: int dev_id, vchan_id (Optional if only one vchannel)");
 #ifndef RTE_EXEC_ENV_WINDOWS
 	rte_telemetry_register_cmd("/dmadev/dump", dmadev_handle_dev_dump,
 			"Returns dump information for a dmadev. Parameters: int dev_id");
-- 
2.17.1

[PATCH v2 2/5] telemetry: fix repeat display when callback don't set dict

[Chengwen Feng]

When telemetry callback didn't set dict and return a non-negative
number, the telemetry will repeat to display the last result. This
patch zero the dict to avoid the problem.

Fixes: 6dd571fd07c3 ("telemetry: introduce new functionality")
Cc: stable@dpdk.org

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 lib/telemetry/telemetry.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/telemetry/telemetry.c b/lib/telemetry/telemetry.c
index 8fbb4f3060..7b905355cd 100644
--- a/lib/telemetry/telemetry.c
+++ b/lib/telemetry/telemetry.c
@@ -333,7 +333,7 @@ output_json(const char *cmd, const struct rte_tel_data *d, int s)
 static void
 perform_command(telemetry_cb fn, const char *cmd, const char *param, int s)
 {
-	struct rte_tel_data data;
+	struct rte_tel_data data = {0};
 
 	int ret = fn(cmd, param, &data);
 	if (ret < 0) {
-- 
2.17.1

[PATCH v2 3/5] ethdev: add newline to telemetry log string

[Chengwen Feng]

The telemetry related code may invoke RTE_ETHDEV_LOG to display
information, the newline character is not added automatically to the
RTE_ETHDEV_LOG, therefore, the newline character must be explicitly
added to the log string.

Fixes: 5514319e7b43 ("telemetry: fix passing full params string to command")
Cc: stable@dpdk.org

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 lib/ethdev/rte_ethdev.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index 5d5e18db1e..9eeae61024 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -5880,7 +5880,7 @@ eth_dev_handle_port_xstats(const char *cmd __rte_unused,
 	port_id = strtoul(params, &end_param, 0);
 	if (*end_param != '\0')
 		RTE_ETHDEV_LOG(NOTICE,
-			"Extra parameters passed to ethdev telemetry command, ignoring");
+			"Extra parameters passed to ethdev telemetry command, ignoring\n");
 	if (!rte_eth_dev_is_valid_port(port_id))
 		return -1;
 
@@ -5931,7 +5931,7 @@ eth_dev_handle_port_dump_priv(const char *cmd __rte_unused,
 	port_id = strtoul(params, &end_param, 0);
 	if (*end_param != '\0')
 		RTE_ETHDEV_LOG(NOTICE,
-			"Extra parameters passed to ethdev telemetry command, ignoring");
+			"Extra parameters passed to ethdev telemetry command, ignoring\n");
 	if (!rte_eth_dev_is_valid_port(port_id))
 		return -EINVAL;
 
@@ -5973,7 +5973,7 @@ eth_dev_handle_port_link_status(const char *cmd __rte_unused,
 	port_id = strtoul(params, &end_param, 0);
 	if (*end_param != '\0')
 		RTE_ETHDEV_LOG(NOTICE,
-			"Extra parameters passed to ethdev telemetry command, ignoring");
+			"Extra parameters passed to ethdev telemetry command, ignoring\n");
 	if (!rte_eth_dev_is_valid_port(port_id))
 		return -1;
 
@@ -6011,7 +6011,7 @@ eth_dev_handle_port_info(const char *cmd __rte_unused,
 	port_id = strtoul(params, &end_param, 0);
 	if (*end_param != '\0')
 		RTE_ETHDEV_LOG(NOTICE,
-			"Extra parameters passed to ethdev telemetry command, ignoring");
+			"Extra parameters passed to ethdev telemetry command, ignoring\n");
 
 	if (!rte_eth_dev_is_valid_port(port_id))
 		return -EINVAL;
-- 
2.17.1

[PATCH v2 4/5] ethdev: support xstats reset telemetry command

[Chengwen Feng]

The xstats reset is useful for debugging, so add it to the ethdev
telemetry command lists.

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 lib/ethdev/rte_ethdev.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index 9eeae61024..2fc593b865 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -5915,6 +5915,33 @@ eth_dev_handle_port_xstats(const char *cmd __rte_unused,
 	return 0;
 }
 
+static int
+eth_dev_handle_port_xstats_reset(const char *cmd __rte_unused,
+		const char *params,
+		struct rte_tel_data *d)
+{
+	int port_id, ret;
+	char *end_param;
+
+	if (params == NULL || strlen(params) == 0 || !isdigit(*params))
+		return -1;
+
+	port_id = strtoul(params, &end_param, 0);
+	if (*end_param != '\0')
+		RTE_ETHDEV_LOG(NOTICE,
+			"Extra parameters passed to ethdev telemetry command, ignoring\n");
+	if (!rte_eth_dev_is_valid_port(port_id))
+		return -1;
+
+	ret = rte_eth_xstats_reset(port_id);
+	if (ret == 0) {
+		rte_tel_data_start_dict(d);
+		rte_tel_data_string(d, "success");
+	}
+
+	return ret;
+}
+
 #ifndef RTE_EXEC_ENV_WINDOWS
 static int
 eth_dev_handle_port_dump_priv(const char *cmd __rte_unused,
@@ -6328,6 +6355,8 @@ RTE_INIT(ethdev_init_telemetry)
 			"Returns the common stats for a port. Parameters: int port_id");
 	rte_telemetry_register_cmd("/ethdev/xstats", eth_dev_handle_port_xstats,
 			"Returns the extended stats for a port. Parameters: int port_id");
+	rte_telemetry_register_cmd("/ethdev/xstats_reset", eth_dev_handle_port_xstats_reset,
+			"Reset the extended stats for a port. Parameters: int port_id");
 #ifndef RTE_EXEC_ENV_WINDOWS
 	rte_telemetry_register_cmd("/ethdev/dump_priv", eth_dev_handle_port_dump_priv,
 			"Returns dump private information for a port. Parameters: int port_id");
-- 
2.17.1

[PATCH v2 5/5] ethdev: telemetry xstats support hide zero

[Chengwen Feng]

The number of xstats may be large, after the hide zero option is added,
only non-zero values can be displayed.

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 lib/ethdev/rte_ethdev.c | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index 2fc593b865..77cacc0829 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -5870,20 +5870,33 @@ eth_dev_handle_port_xstats(const char *cmd __rte_unused,
 {
 	struct rte_eth_xstat *eth_xstats;
 	struct rte_eth_xstat_name *xstat_names;
+	char *end_param, *hide_param;
 	int port_id, num_xstats;
+	int hide_zero = 0;
 	int i, ret;
-	char *end_param;
 
 	if (params == NULL || strlen(params) == 0 || !isdigit(*params))
 		return -1;
 
 	port_id = strtoul(params, &end_param, 0);
-	if (*end_param != '\0')
-		RTE_ETHDEV_LOG(NOTICE,
-			"Extra parameters passed to ethdev telemetry command, ignoring\n");
 	if (!rte_eth_dev_is_valid_port(port_id))
 		return -1;
 
+	if (*end_param != '\0') {
+		hide_param = strtok(end_param, ",");
+		if (!hide_param || strlen(hide_param) == 0 || !isdigit(*hide_param))
+			return -EINVAL;
+		hide_zero = strtoul(hide_param, &end_param, 0);
+		if (*end_param != '\0')
+			RTE_ETHDEV_LOG(NOTICE,
+				"Extra parameters passed to ethdev telemetry command, ignoring\n");
+		if (hide_zero != 0 && hide_zero != 1) {
+			hide_zero = !!hide_zero;
+			RTE_ETHDEV_LOG(NOTICE,
+				"Hide zero parameter is non-boolean, cast to boolean\n");
+		}
+	}
+
 	num_xstats = rte_eth_xstats_get(port_id, NULL, 0);
 	if (num_xstats < 0)
 		return -1;
@@ -5908,9 +5921,12 @@ eth_dev_handle_port_xstats(const char *cmd __rte_unused,
 	}
 
 	rte_tel_data_start_dict(d);
-	for (i = 0; i < num_xstats; i++)
+	for (i = 0; i < num_xstats; i++) {
+		if (hide_zero && eth_xstats[i].value == 0)
+			continue;
 		rte_tel_data_add_dict_u64(d, xstat_names[i].name,
 				eth_xstats[i].value);
+	}
 	free(eth_xstats);
 	return 0;
 }
@@ -6354,7 +6370,7 @@ RTE_INIT(ethdev_init_telemetry)
 	rte_telemetry_register_cmd("/ethdev/stats", eth_dev_handle_port_stats,
 			"Returns the common stats for a port. Parameters: int port_id");
 	rte_telemetry_register_cmd("/ethdev/xstats", eth_dev_handle_port_xstats,
-			"Returns the extended stats for a port. Parameters: int port_id");
+			"Returns the extended stats for a port. Parameters: int port_id, hide_zero (Optional, specify whether to hide zero values)");
 	rte_telemetry_register_cmd("/ethdev/xstats_reset", eth_dev_handle_port_xstats_reset,
 			"Reset the extended stats for a port. Parameters: int port_id");
 #ifndef RTE_EXEC_ENV_WINDOWS
-- 
2.17.1

Re: [PATCH v2 0/5] support dmadev/ethdev stats reset

[fengchengwen]

Sorry to repeat send v2, this new version include change log.

On 2023/1/11 20:06, Chengwen Feng wrote:
> This patchset contains dmadev/ethdev stats reset, and also support
> hide zero for ethdev xstats and two telemetry related bugs.
> 
> Chengwen Feng (5):
>   dmadev: support stats reset telemetry command
>   telemetry: fix repeat display when callback don't set dict
>   ethdev: add newline to telemetry log string
>   ethdev: support xstats reset telemetry command
>   ethdev: telemetry xstats support hide zero
> 
> ---
> v2: 
> * address Bruce's comment on 2/5 patch.
> * support output success when stats reset.
> 
>  lib/dmadev/rte_dmadev.c   | 46 ++++++++++++++++++++++++++++
>  lib/ethdev/rte_ethdev.c   | 63 +++++++++++++++++++++++++++++++++------
>  lib/telemetry/telemetry.c |  2 +-
>  3 files changed, 101 insertions(+), 10 deletions(-)
> 

Re: [PATCH v5 2/5] telemetry: fix repeated display when callback don't set dict

[fengchengwen]

Hi Bruce,

On 2023/1/7 1:33, Bruce Richardson wrote:
> On Fri, Jan 06, 2023 at 04:07:45PM +0000, Bruce Richardson wrote:
>> On Mon, Dec 26, 2022 at 12:53:57PM +0800, fengchengwen wrote:
>>> On 2022/12/19 17:33, Bruce Richardson wrote:
>>>> On Mon, Dec 19, 2022 at 09:07:20AM +0000, Chengwen Feng wrote:
>>>>> When telemetry callback didn't set dict and return a non-negative
>>>>> number, the telemetry will repeat to display the last result.
>>>>>
>>>>> Fixes: 6dd571fd07c3 ("telemetry: introduce new functionality")
>>>>> Cc: stable@dpdk.org
>>>>>
>>>>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>>>>> ---
>>>>
>>>> Hi Chengwen,
>>>>
>>>> I'm a little curious about this bug. Can you describe some steps to
>>>> reproduce it as I'm curious as to exactly what is happening. The fix seems
>>>> a little strange to me so I'd like to investigate a little more to see if
>>>> other approaches might work.
>>>
>>> Hi Bruce,
>>>
>>> Sorry for late reply.
>>>
>>> The steps:
>>>   1. applay "[PATCH v5 1/5] dmadev: support stats reset telemetry command"
>>>   2. compile
>>>   3. start dpdk-dma: dpdk-dma -a DMA.BDF -a NIC.BDF -- -c hw
>>>   4. start telemetry, and execute /dmadev/stats,0, and then /dmadev/stats_reset,0
>>>      the output of /dmadev/stats_reset,0 will be the same of previous cmd "/dmadev/stats,0"
>>>      e.g. my environment:
>>> --> /dmadev/stats,0
>>> {
>>>   "/dmadev/stats": {
>>>     "submitted": 23,
>>>     "completed": 23,
>>>     "errors": 0
>>>   }
>>> }
>>> --> /dmadev/stats_reset,0
>>> {
>>>   "/dmadev/stats_reset": {
>>>     "submitted": 23,
>>>     "completed": 23,
>>>     "errors": 0
>>>   }
>>> }
>>>
>>> The rootcause is that the /dmadev/stats_reset don't set the outer parameter "struct rte_tel_data *info"
>>> and return zero.
>>>
>> Thanks for the fuller explanation, I'll hopefully test it out myself.
>>
>> However, in the meantime, looking at the telemetry library code, would the
>> following change work rather than explicitly always setting the telemetry
>> data to a dictionary by default? Zeroing the data by default sets it to a
>> null return which is what you probably want as default rather than an empty
>> dictionary. (And it's also a smaller diff)
>>
>> /Bruce
>>
>> diff --git a/lib/telemetry/telemetry.c b/lib/telemetry/telemetry.c
>> index 8fbb4f3060..7b905355cd 100644
>> --- a/lib/telemetry/telemetry.c
>> +++ b/lib/telemetry/telemetry.c
>> @@ -333,7 +333,7 @@ output_json(const char *cmd, const struct rte_tel_data *d, int s)
>>  static void
>>  perform_command(telemetry_cb fn, const char *cmd, const char *param, int s)
>>  {
>> -       struct rte_tel_data data;
>> +       struct rte_tel_data data = {0};
>>  
>>         int ret = fn(cmd, param, &data);
>>         if (ret < 0) {
>>
> 
> I've handily reproduced the issue using the instructions you gave above,
> thanks for those.
> 
> Based on that, and looking a little deeper:
> 
> * I think it is an error with the reset function not to initialize and
>   complete the return value. I believe that for each telemetry callback we
>   should require that the callback fill in a valid value on success. For
>   reset, some options could be just a string "OK", or an array just
>   containing "[0]", or similar.

good idea, the v2 follow it.

> 
> * Given that point above, I do agree though that the "data" parameter
>   should be properly initialized on entry to the callbacks. However, I feel
>   that the correct init should be the empty/null value, as is given in the
>   patch above.

already fix in v2

Thanks.

> 
> Regards,
> /Bruce
> .
> 

Re: [PATCH v2 1/5] dmadev: support stats reset telemetry command

[Bruce Richardson]

On Wed, Jan 11, 2023 at 12:06:26PM +0000, Chengwen Feng wrote:
> The stats reset is useful for debugging, so add it to the dmadev
> telemetry command lists.
> 
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>

Looks ok to me, just one small comment inline. With that fixed:

Reviewed-by: Bruce Richardson <bruce.richardson@intel.com>

> ---
>  lib/dmadev/rte_dmadev.c | 46 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
> 
> diff --git a/lib/dmadev/rte_dmadev.c b/lib/dmadev/rte_dmadev.c
> index 4da653eec7..79593cdbe2 100644
> --- a/lib/dmadev/rte_dmadev.c
> +++ b/lib/dmadev/rte_dmadev.c
> @@ -994,6 +994,50 @@ dmadev_handle_dev_stats(const char *cmd __rte_unused,
>  	return 0;
>  }
>  
> +static int
> +dmadev_handle_dev_stats_reset(const char *cmd __rte_unused,
> +		const char *params,
> +		struct rte_tel_data *d)
> +{
> +	struct rte_dma_info dma_info;
> +	int dev_id, ret, vchan_id;
> +	const char *vchan_param;
> +	char *end_param;
> +
> +	if (params == NULL || strlen(params) == 0 || !isdigit(*params))
> +		return -EINVAL;
> +
> +	dev_id = strtoul(params, &end_param, 0);
> +
> +	/* Function info_get validates dev_id so we don't need to. */
> +	ret = rte_dma_info_get(dev_id, &dma_info);
> +	if (ret < 0)
> +		return -EINVAL;
> +
> +	/* If the device has one vchan the user does not need to supply the
> +	 * vchan id and only the device id is needed, no extra parameters.
> +	 */
> +	if (dma_info.nb_vchans == 1 && *end_param == '\0')
> +		vchan_id = 0;
> +	else {
> +		vchan_param = strtok(end_param, ",");
> +		if (!vchan_param || strlen(vchan_param) == 0 || !isdigit(*vchan_param))
> +			return -EINVAL;
> +
> +		vchan_id = strtoul(vchan_param, &end_param, 0);
> +	}
> +	if (*end_param != '\0')
> +		RTE_DMA_LOG(WARNING, "Extra parameters passed to dmadev telemetry command, ignoring");
> +
> +	ret = rte_dma_stats_reset(dev_id, vchan_id);
> +	if (ret == 0) {
> +		rte_tel_data_start_dict(d);
This line is unnecessary. "rte_tel_data_string" below sets the overall
datatype as "string", replacing the "dict" type set here. To set the string
inside the dict you have just created use "rte_tel_data_add_dict_string".
[Note: the data returned from a command is always put in a json dict
automatically - that is separate from any dict initialized here.]

> +		rte_tel_data_string(d, "success");
> +	}
> +
> +	return ret;
> +}
> +
>  #ifndef RTE_EXEC_ENV_WINDOWS
>  static int
>  dmadev_handle_dev_dump(const char *cmd __rte_unused,
> @@ -1041,6 +1085,8 @@ RTE_INIT(dmadev_init_telemetry)
>  			"Returns information for a dmadev. Parameters: int dev_id");
>  	rte_telemetry_register_cmd("/dmadev/stats", dmadev_handle_dev_stats,
>  			"Returns the stats for a dmadev vchannel. Parameters: int dev_id, vchan_id (Optional if only one vchannel)");
> +	rte_telemetry_register_cmd("/dmadev/stats_reset", dmadev_handle_dev_stats_reset,
> +			"Reset the stats for a dmadev vchannel. Parameters: int dev_id, vchan_id (Optional if only one vchannel)");
>  #ifndef RTE_EXEC_ENV_WINDOWS
>  	rte_telemetry_register_cmd("/dmadev/dump", dmadev_handle_dev_dump,
>  			"Returns dump information for a dmadev. Parameters: int dev_id");
> -- 
> 2.17.1
> 

Re: [PATCH v2 2/5] telemetry: fix repeat display when callback don't set dict

[Bruce Richardson]

On Wed, Jan 11, 2023 at 12:06:27PM +0000, Chengwen Feng wrote:
> When telemetry callback didn't set dict and return a non-negative

I'd suggest rewording the "didn't set dict", since it's not required that a
telemetry callback do anything with dictionaries. I'd suggest:

"When a telemetry callback doesn't initialize the telemetry data
structure and returns a non-negative number..."

> number, the telemetry will repeat to display the last result. This
> patch zero the dict to avoid the problem.

s/zero the dict/zero the data-structure/

> 
> Fixes: 6dd571fd07c3 ("telemetry: introduce new functionality")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> ---

Reviewed-by: Bruce Richardson <bruce.richardson@intel.com>

>  lib/telemetry/telemetry.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/telemetry/telemetry.c b/lib/telemetry/telemetry.c
> index 8fbb4f3060..7b905355cd 100644
> --- a/lib/telemetry/telemetry.c
> +++ b/lib/telemetry/telemetry.c
> @@ -333,7 +333,7 @@ output_json(const char *cmd, const struct rte_tel_data *d, int s)
>  static void
>  perform_command(telemetry_cb fn, const char *cmd, const char *param, int s)
>  {
> -	struct rte_tel_data data;
> +	struct rte_tel_data data = {0};
>  
>  	int ret = fn(cmd, param, &data);
>  	if (ret < 0) {
> -- 
> 2.17.1
> 

Re: [PATCH v2 5/5] ethdev: telemetry xstats support hide zero

[Bruce Richardson]

On Wed, Jan 11, 2023 at 12:06:30PM +0000, Chengwen Feng wrote:
> The number of xstats may be large, after the hide zero option is added,
> only non-zero values can be displayed.
> 
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> ---
>  lib/ethdev/rte_ethdev.c | 28 ++++++++++++++++++++++------
>  1 file changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> index 2fc593b865..77cacc0829 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -5870,20 +5870,33 @@ eth_dev_handle_port_xstats(const char *cmd __rte_unused,
>  {
>  	struct rte_eth_xstat *eth_xstats;
>  	struct rte_eth_xstat_name *xstat_names;
> +	char *end_param, *hide_param;
>  	int port_id, num_xstats;
> +	int hide_zero = 0;
>  	int i, ret;
> -	char *end_param;
>  
>  	if (params == NULL || strlen(params) == 0 || !isdigit(*params))
>  		return -1;
>  
>  	port_id = strtoul(params, &end_param, 0);
> -	if (*end_param != '\0')
> -		RTE_ETHDEV_LOG(NOTICE,
> -			"Extra parameters passed to ethdev telemetry command, ignoring\n");
>  	if (!rte_eth_dev_is_valid_port(port_id))
>  		return -1;
>  
> +	if (*end_param != '\0') {
> +		hide_param = strtok(end_param, ",");
> +		if (!hide_param || strlen(hide_param) == 0 || !isdigit(*hide_param))
> +			return -EINVAL;
> +		hide_zero = strtoul(hide_param, &end_param, 0);
> +		if (*end_param != '\0')
> +			RTE_ETHDEV_LOG(NOTICE,
> +				"Extra parameters passed to ethdev telemetry command, ignoring\n");
> +		if (hide_zero != 0 && hide_zero != 1) {
> +			hide_zero = !!hide_zero;
> +			RTE_ETHDEV_LOG(NOTICE,
> +				"Hide zero parameter is non-boolean, cast to boolean\n");
> +		}
> +	}
> +

I'm not sure about this adding of an extra flag as a 0/1 value. That would
seem to make it confusing with a queue number or extra port number. For
such an optional parameter, I think a string value would be clearer. A
number of alternatives I'd suggest - in order of my preference (least
preferred to most preferred):

* have the extra parameter as a literal string "hide_zeros" which is
  matched against rather than looking for 0/1, or alternatively

* add a new command /ethdev/xstats_nonzero which does this, the same
  callback can be reusued, just checking the command given, or finally,

* if this is primarily for the benefit of users using the telemetry script
  to interactively view stats, then the functionality could be implemented
  in the script itself rather than in the backend. We could add some
  setting, or extra flag to the display code, to possibly filter out zeros
  in the display.

Thoughts?

/Bruce

[PATCH v3 0/5] support dmadev/ethdev stats reset

[Chengwen Feng]

This patchset contains dmadev/ethdev stats reset, and also support
hide zero for ethdev xstats and two telemetry related bugs.

Chengwen Feng (5):
  dmadev: support stats reset telemetry command
  telemetry: fix repeat display when callback don't init dict
  ethdev: add newline to telemetry log string
  ethdev: support xstats reset telemetry command
  ethdev: telemetry xstats support hide zero

---
v3:
* address Bruce's comment on 1/5 and 2/5 patch, and also for 4/5 patch.
* reword the help info of 5/5 patch.
v2:
* address Bruce's comment on 2/5 patch.
* support output success when stats reset.

 lib/dmadev/rte_dmadev.c   | 43 ++++++++++++++++++++++++++++++
 lib/ethdev/rte_ethdev.c   | 56 ++++++++++++++++++++++++++++++++-------
 lib/telemetry/telemetry.c |  2 +-
 3 files changed, 91 insertions(+), 10 deletions(-)

-- 
2.17.1

[PATCH v3 1/5] dmadev: support stats reset telemetry command

[Chengwen Feng]

The stats reset is useful for debugging, so add it to the dmadev
telemetry command lists.

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
Reviewed-by: Bruce Richardson <bruce.richardson@intel.com>
---
 lib/dmadev/rte_dmadev.c | 43 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/lib/dmadev/rte_dmadev.c b/lib/dmadev/rte_dmadev.c
index 4da653eec7..9960b917b7 100644
--- a/lib/dmadev/rte_dmadev.c
+++ b/lib/dmadev/rte_dmadev.c
@@ -994,6 +994,47 @@ dmadev_handle_dev_stats(const char *cmd __rte_unused,
 	return 0;
 }
 
+static int
+dmadev_handle_dev_stats_reset(const char *cmd __rte_unused,
+		const char *params,
+		struct rte_tel_data *d)
+{
+	struct rte_dma_info dma_info;
+	int dev_id, ret, vchan_id;
+	const char *vchan_param;
+	char *end_param;
+
+	if (params == NULL || strlen(params) == 0 || !isdigit(*params))
+		return -EINVAL;
+
+	dev_id = strtoul(params, &end_param, 0);
+
+	/* Function info_get validates dev_id so we don't need to. */
+	ret = rte_dma_info_get(dev_id, &dma_info);
+	if (ret < 0)
+		return -EINVAL;
+
+	/* If the device has one vchan the user does not need to supply the
+	 * vchan id and only the device id is needed, no extra parameters.
+	 */
+	if (dma_info.nb_vchans == 1 && *end_param == '\0') {
+		vchan_id = 0;
+	} else {
+		vchan_param = strtok(end_param, ",");
+		if (vchan_param == NULL || strlen(vchan_param) == 0 || !isdigit(*vchan_param))
+			return -EINVAL;
+		vchan_id = strtoul(vchan_param, &end_param, 0);
+	}
+	if (*end_param != '\0')
+		RTE_DMA_LOG(WARNING, "Extra parameters passed to dmadev telemetry command, ignoring");
+
+	ret = rte_dma_stats_reset(dev_id, vchan_id);
+	if (ret == 0)
+		rte_tel_data_string(d, "success");
+
+	return ret;
+}
+
 #ifndef RTE_EXEC_ENV_WINDOWS
 static int
 dmadev_handle_dev_dump(const char *cmd __rte_unused,
@@ -1041,6 +1082,8 @@ RTE_INIT(dmadev_init_telemetry)
 			"Returns information for a dmadev. Parameters: int dev_id");
 	rte_telemetry_register_cmd("/dmadev/stats", dmadev_handle_dev_stats,
 			"Returns the stats for a dmadev vchannel. Parameters: int dev_id, vchan_id (Optional if only one vchannel)");
+	rte_telemetry_register_cmd("/dmadev/stats_reset", dmadev_handle_dev_stats_reset,
+			"Reset the stats for a dmadev vchannel. Parameters: int dev_id, vchan_id (Optional if only one vchannel)");
 #ifndef RTE_EXEC_ENV_WINDOWS
 	rte_telemetry_register_cmd("/dmadev/dump", dmadev_handle_dev_dump,
 			"Returns dump information for a dmadev. Parameters: int dev_id");
-- 
2.17.1

[PATCH v3 2/5] telemetry: fix repeat display when callback don't init dict

[Chengwen Feng]

When a telemetry callback doesn't initialize the telemetry data
structure and returns a non-negative number, the telemetry will repeat
to display the last result. This patch zero the data structure to avoid
the problem.

Fixes: 6dd571fd07c3 ("telemetry: introduce new functionality")
Cc: stable@dpdk.org

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
Reviewed-by: Bruce Richardson <bruce.richardson@intel.com>
---
 lib/telemetry/telemetry.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/telemetry/telemetry.c b/lib/telemetry/telemetry.c
index 8fbb4f3060..7b905355cd 100644
--- a/lib/telemetry/telemetry.c
+++ b/lib/telemetry/telemetry.c
@@ -333,7 +333,7 @@ output_json(const char *cmd, const struct rte_tel_data *d, int s)
 static void
 perform_command(telemetry_cb fn, const char *cmd, const char *param, int s)
 {
-	struct rte_tel_data data;
+	struct rte_tel_data data = {0};
 
 	int ret = fn(cmd, param, &data);
 	if (ret < 0) {
-- 
2.17.1

[PATCH v3 3/5] ethdev: add newline to telemetry log string

[Chengwen Feng]

The telemetry related code may invoke RTE_ETHDEV_LOG to display
information, the newline character is not added automatically to the
RTE_ETHDEV_LOG, therefore, the newline character must be explicitly
added to the log string.

Fixes: 5514319e7b43 ("telemetry: fix passing full params string to command")
Cc: stable@dpdk.org

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 lib/ethdev/rte_ethdev.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index 5d5e18db1e..9eeae61024 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -5880,7 +5880,7 @@ eth_dev_handle_port_xstats(const char *cmd __rte_unused,
 	port_id = strtoul(params, &end_param, 0);
 	if (*end_param != '\0')
 		RTE_ETHDEV_LOG(NOTICE,
-			"Extra parameters passed to ethdev telemetry command, ignoring");
+			"Extra parameters passed to ethdev telemetry command, ignoring\n");
 	if (!rte_eth_dev_is_valid_port(port_id))
 		return -1;
 
@@ -5931,7 +5931,7 @@ eth_dev_handle_port_dump_priv(const char *cmd __rte_unused,
 	port_id = strtoul(params, &end_param, 0);
 	if (*end_param != '\0')
 		RTE_ETHDEV_LOG(NOTICE,
-			"Extra parameters passed to ethdev telemetry command, ignoring");
+			"Extra parameters passed to ethdev telemetry command, ignoring\n");
 	if (!rte_eth_dev_is_valid_port(port_id))
 		return -EINVAL;
 
@@ -5973,7 +5973,7 @@ eth_dev_handle_port_link_status(const char *cmd __rte_unused,
 	port_id = strtoul(params, &end_param, 0);
 	if (*end_param != '\0')
 		RTE_ETHDEV_LOG(NOTICE,
-			"Extra parameters passed to ethdev telemetry command, ignoring");
+			"Extra parameters passed to ethdev telemetry command, ignoring\n");
 	if (!rte_eth_dev_is_valid_port(port_id))
 		return -1;
 
@@ -6011,7 +6011,7 @@ eth_dev_handle_port_info(const char *cmd __rte_unused,
 	port_id = strtoul(params, &end_param, 0);
 	if (*end_param != '\0')
 		RTE_ETHDEV_LOG(NOTICE,
-			"Extra parameters passed to ethdev telemetry command, ignoring");
+			"Extra parameters passed to ethdev telemetry command, ignoring\n");
 
 	if (!rte_eth_dev_is_valid_port(port_id))
 		return -EINVAL;
-- 
2.17.1

[PATCH v3 4/5] ethdev: support xstats reset telemetry command

[Chengwen Feng]

The xstats reset is useful for debugging, so add it to the ethdev
telemetry command lists.

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 lib/ethdev/rte_ethdev.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index 9eeae61024..ebcb05b523 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -5915,6 +5915,31 @@ eth_dev_handle_port_xstats(const char *cmd __rte_unused,
 	return 0;
 }
 
+static int
+eth_dev_handle_port_xstats_reset(const char *cmd __rte_unused,
+		const char *params,
+		struct rte_tel_data *d)
+{
+	int port_id, ret;
+	char *end_param;
+
+	if (params == NULL || strlen(params) == 0 || !isdigit(*params))
+		return -1;
+
+	port_id = strtoul(params, &end_param, 0);
+	if (*end_param != '\0')
+		RTE_ETHDEV_LOG(NOTICE,
+			"Extra parameters passed to ethdev telemetry command, ignoring\n");
+	if (!rte_eth_dev_is_valid_port(port_id))
+		return -1;
+
+	ret = rte_eth_xstats_reset(port_id);
+	if (ret == 0)
+		rte_tel_data_string(d, "success");
+
+	return ret;
+}
+
 #ifndef RTE_EXEC_ENV_WINDOWS
 static int
 eth_dev_handle_port_dump_priv(const char *cmd __rte_unused,
@@ -6328,6 +6353,8 @@ RTE_INIT(ethdev_init_telemetry)
 			"Returns the common stats for a port. Parameters: int port_id");
 	rte_telemetry_register_cmd("/ethdev/xstats", eth_dev_handle_port_xstats,
 			"Returns the extended stats for a port. Parameters: int port_id");
+	rte_telemetry_register_cmd("/ethdev/xstats_reset", eth_dev_handle_port_xstats_reset,
+			"Reset the extended stats for a port. Parameters: int port_id");
 #ifndef RTE_EXEC_ENV_WINDOWS
 	rte_telemetry_register_cmd("/ethdev/dump_priv", eth_dev_handle_port_dump_priv,
 			"Returns dump private information for a port. Parameters: int port_id");
-- 
2.17.1

[PATCH v3 5/5] ethdev: telemetry xstats support hide zero

[Chengwen Feng]

The number of xstats may be large, after the hide zero option is added,
only non-zero values can be displayed.

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 lib/ethdev/rte_ethdev.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index ebcb05b523..01bf78490d 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -5870,20 +5870,28 @@ eth_dev_handle_port_xstats(const char *cmd __rte_unused,
 {
 	struct rte_eth_xstat *eth_xstats;
 	struct rte_eth_xstat_name *xstat_names;
+	char *end_param, *hide_param;
 	int port_id, num_xstats;
+	int hide_zero = 0;
 	int i, ret;
-	char *end_param;
 
 	if (params == NULL || strlen(params) == 0 || !isdigit(*params))
 		return -1;
 
 	port_id = strtoul(params, &end_param, 0);
-	if (*end_param != '\0')
-		RTE_ETHDEV_LOG(NOTICE,
-			"Extra parameters passed to ethdev telemetry command, ignoring\n");
 	if (!rte_eth_dev_is_valid_port(port_id))
 		return -1;
 
+	if (*end_param != '\0') {
+		hide_param = strtok(end_param, ",");
+		if (hide_param == NULL || strlen(hide_param) == 0 || !isdigit(*hide_param))
+			return -EINVAL;
+		hide_zero = strtoul(hide_param, &end_param, 0);
+		if (*end_param != '\0')
+			RTE_ETHDEV_LOG(NOTICE,
+				"Extra parameters passed to ethdev telemetry command, ignoring\n");
+	}
+
 	num_xstats = rte_eth_xstats_get(port_id, NULL, 0);
 	if (num_xstats < 0)
 		return -1;
@@ -5908,9 +5916,12 @@ eth_dev_handle_port_xstats(const char *cmd __rte_unused,
 	}
 
 	rte_tel_data_start_dict(d);
-	for (i = 0; i < num_xstats; i++)
+	for (i = 0; i < num_xstats; i++) {
+		if (hide_zero != 0 && eth_xstats[i].value == 0)
+			continue;
 		rte_tel_data_add_dict_u64(d, xstat_names[i].name,
 				eth_xstats[i].value);
+	}
 	free(eth_xstats);
 	return 0;
 }
@@ -6352,7 +6363,7 @@ RTE_INIT(ethdev_init_telemetry)
 	rte_telemetry_register_cmd("/ethdev/stats", eth_dev_handle_port_stats,
 			"Returns the common stats for a port. Parameters: int port_id");
 	rte_telemetry_register_cmd("/ethdev/xstats", eth_dev_handle_port_xstats,
-			"Returns the extended stats for a port. Parameters: int port_id");
+			"Returns the extended stats for a port. Parameters: int port_id, bool hide_zero (Optional, non-zero indicates hide zero xstats-value)");
 	rte_telemetry_register_cmd("/ethdev/xstats_reset", eth_dev_handle_port_xstats_reset,
 			"Reset the extended stats for a port. Parameters: int port_id");
 #ifndef RTE_EXEC_ENV_WINDOWS
-- 
2.17.1

[PATCH v4 0/5] support dmadev/ethdev stats reset

[Chengwen Feng]

This patchset contains dmadev/ethdev stats reset, and also support
hide zero for ethdev xstats and two telemetry related bugs.

Chengwen Feng (5):
  dmadev: support stats reset telemetry command
  telemetry: fix repeat display when callback don't init dict
  ethdev: add newline to telemetry log string
  ethdev: support xstats reset telemetry command
  ethdev: telemetry xstats support hide zero

---
v4:
* solve the internal don't commit for 5/5 patch.
v3:
* address Bruce's comment on 1/5 and 2/5 patch, and also for 4/5 patch.
* reword the help info of 5/5 patch.
v2:
* address Bruce's comment on 2/5 patch.
* support output success when stats reset.

 lib/dmadev/rte_dmadev.c   | 43 ++++++++++++++++++++++++++++++
 lib/ethdev/rte_ethdev.c   | 56 ++++++++++++++++++++++++++++++++-------
 lib/telemetry/telemetry.c |  2 +-
 3 files changed, 91 insertions(+), 10 deletions(-)

-- 
2.17.1

[PATCH v4 1/5] dmadev: support stats reset telemetry command

[Chengwen Feng]

The stats reset is useful for debugging, so add it to the dmadev
telemetry command lists.

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
Reviewed-by: Bruce Richardson <bruce.richardson@intel.com>
---
 lib/dmadev/rte_dmadev.c | 43 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/lib/dmadev/rte_dmadev.c b/lib/dmadev/rte_dmadev.c
index 4da653eec7..9960b917b7 100644
--- a/lib/dmadev/rte_dmadev.c
+++ b/lib/dmadev/rte_dmadev.c
@@ -994,6 +994,47 @@ dmadev_handle_dev_stats(const char *cmd __rte_unused,
 	return 0;
 }
 
+static int
+dmadev_handle_dev_stats_reset(const char *cmd __rte_unused,
+		const char *params,
+		struct rte_tel_data *d)
+{
+	struct rte_dma_info dma_info;
+	int dev_id, ret, vchan_id;
+	const char *vchan_param;
+	char *end_param;
+
+	if (params == NULL || strlen(params) == 0 || !isdigit(*params))
+		return -EINVAL;
+
+	dev_id = strtoul(params, &end_param, 0);
+
+	/* Function info_get validates dev_id so we don't need to. */
+	ret = rte_dma_info_get(dev_id, &dma_info);
+	if (ret < 0)
+		return -EINVAL;
+
+	/* If the device has one vchan the user does not need to supply the
+	 * vchan id and only the device id is needed, no extra parameters.
+	 */
+	if (dma_info.nb_vchans == 1 && *end_param == '\0') {
+		vchan_id = 0;
+	} else {
+		vchan_param = strtok(end_param, ",");
+		if (vchan_param == NULL || strlen(vchan_param) == 0 || !isdigit(*vchan_param))
+			return -EINVAL;
+		vchan_id = strtoul(vchan_param, &end_param, 0);
+	}
+	if (*end_param != '\0')
+		RTE_DMA_LOG(WARNING, "Extra parameters passed to dmadev telemetry command, ignoring");
+
+	ret = rte_dma_stats_reset(dev_id, vchan_id);
+	if (ret == 0)
+		rte_tel_data_string(d, "success");
+
+	return ret;
+}
+
 #ifndef RTE_EXEC_ENV_WINDOWS
 static int
 dmadev_handle_dev_dump(const char *cmd __rte_unused,
@@ -1041,6 +1082,8 @@ RTE_INIT(dmadev_init_telemetry)
 			"Returns information for a dmadev. Parameters: int dev_id");
 	rte_telemetry_register_cmd("/dmadev/stats", dmadev_handle_dev_stats,
 			"Returns the stats for a dmadev vchannel. Parameters: int dev_id, vchan_id (Optional if only one vchannel)");
+	rte_telemetry_register_cmd("/dmadev/stats_reset", dmadev_handle_dev_stats_reset,
+			"Reset the stats for a dmadev vchannel. Parameters: int dev_id, vchan_id (Optional if only one vchannel)");
 #ifndef RTE_EXEC_ENV_WINDOWS
 	rte_telemetry_register_cmd("/dmadev/dump", dmadev_handle_dev_dump,
 			"Returns dump information for a dmadev. Parameters: int dev_id");
-- 
2.17.1

[PATCH v4 2/5] telemetry: fix repeat display when callback don't init dict

[Chengwen Feng]

When a telemetry callback doesn't initialize the telemetry data
structure and returns a non-negative number, the telemetry will repeat
to display the last result. This patch zero the data structure to avoid
the problem.

Fixes: 6dd571fd07c3 ("telemetry: introduce new functionality")
Cc: stable@dpdk.org

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
Reviewed-by: Bruce Richardson <bruce.richardson@intel.com>
---
 lib/telemetry/telemetry.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/telemetry/telemetry.c b/lib/telemetry/telemetry.c
index 8fbb4f3060..7b905355cd 100644
--- a/lib/telemetry/telemetry.c
+++ b/lib/telemetry/telemetry.c
@@ -333,7 +333,7 @@ output_json(const char *cmd, const struct rte_tel_data *d, int s)
 static void
 perform_command(telemetry_cb fn, const char *cmd, const char *param, int s)
 {
-	struct rte_tel_data data;
+	struct rte_tel_data data = {0};
 
 	int ret = fn(cmd, param, &data);
 	if (ret < 0) {
-- 
2.17.1

[PATCH v4 3/5] ethdev: add newline to telemetry log string

[Chengwen Feng]

The telemetry related code may invoke RTE_ETHDEV_LOG to display
information, the newline character is not added automatically to the
RTE_ETHDEV_LOG, therefore, the newline character must be explicitly
added to the log string.

Fixes: 5514319e7b43 ("telemetry: fix passing full params string to command")
Cc: stable@dpdk.org

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 lib/ethdev/rte_ethdev.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index 5d5e18db1e..9eeae61024 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -5880,7 +5880,7 @@ eth_dev_handle_port_xstats(const char *cmd __rte_unused,
 	port_id = strtoul(params, &end_param, 0);
 	if (*end_param != '\0')
 		RTE_ETHDEV_LOG(NOTICE,
-			"Extra parameters passed to ethdev telemetry command, ignoring");
+			"Extra parameters passed to ethdev telemetry command, ignoring\n");
 	if (!rte_eth_dev_is_valid_port(port_id))
 		return -1;
 
@@ -5931,7 +5931,7 @@ eth_dev_handle_port_dump_priv(const char *cmd __rte_unused,
 	port_id = strtoul(params, &end_param, 0);
 	if (*end_param != '\0')
 		RTE_ETHDEV_LOG(NOTICE,
-			"Extra parameters passed to ethdev telemetry command, ignoring");
+			"Extra parameters passed to ethdev telemetry command, ignoring\n");
 	if (!rte_eth_dev_is_valid_port(port_id))
 		return -EINVAL;
 
@@ -5973,7 +5973,7 @@ eth_dev_handle_port_link_status(const char *cmd __rte_unused,
 	port_id = strtoul(params, &end_param, 0);
 	if (*end_param != '\0')
 		RTE_ETHDEV_LOG(NOTICE,
-			"Extra parameters passed to ethdev telemetry command, ignoring");
+			"Extra parameters passed to ethdev telemetry command, ignoring\n");
 	if (!rte_eth_dev_is_valid_port(port_id))
 		return -1;
 
@@ -6011,7 +6011,7 @@ eth_dev_handle_port_info(const char *cmd __rte_unused,
 	port_id = strtoul(params, &end_param, 0);
 	if (*end_param != '\0')
 		RTE_ETHDEV_LOG(NOTICE,
-			"Extra parameters passed to ethdev telemetry command, ignoring");
+			"Extra parameters passed to ethdev telemetry command, ignoring\n");
 
 	if (!rte_eth_dev_is_valid_port(port_id))
 		return -EINVAL;
-- 
2.17.1

[PATCH v4 4/5] ethdev: support xstats reset telemetry command

[Chengwen Feng]

The xstats reset is useful for debugging, so add it to the ethdev
telemetry command lists.

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 lib/ethdev/rte_ethdev.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index 9eeae61024..ebcb05b523 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -5915,6 +5915,31 @@ eth_dev_handle_port_xstats(const char *cmd __rte_unused,
 	return 0;
 }
 
+static int
+eth_dev_handle_port_xstats_reset(const char *cmd __rte_unused,
+		const char *params,
+		struct rte_tel_data *d)
+{
+	int port_id, ret;
+	char *end_param;
+
+	if (params == NULL || strlen(params) == 0 || !isdigit(*params))
+		return -1;
+
+	port_id = strtoul(params, &end_param, 0);
+	if (*end_param != '\0')
+		RTE_ETHDEV_LOG(NOTICE,
+			"Extra parameters passed to ethdev telemetry command, ignoring\n");
+	if (!rte_eth_dev_is_valid_port(port_id))
+		return -1;
+
+	ret = rte_eth_xstats_reset(port_id);
+	if (ret == 0)
+		rte_tel_data_string(d, "success");
+
+	return ret;
+}
+
 #ifndef RTE_EXEC_ENV_WINDOWS
 static int
 eth_dev_handle_port_dump_priv(const char *cmd __rte_unused,
@@ -6328,6 +6353,8 @@ RTE_INIT(ethdev_init_telemetry)
 			"Returns the common stats for a port. Parameters: int port_id");
 	rte_telemetry_register_cmd("/ethdev/xstats", eth_dev_handle_port_xstats,
 			"Returns the extended stats for a port. Parameters: int port_id");
+	rte_telemetry_register_cmd("/ethdev/xstats_reset", eth_dev_handle_port_xstats_reset,
+			"Reset the extended stats for a port. Parameters: int port_id");
 #ifndef RTE_EXEC_ENV_WINDOWS
 	rte_telemetry_register_cmd("/ethdev/dump_priv", eth_dev_handle_port_dump_priv,
 			"Returns dump private information for a port. Parameters: int port_id");
-- 
2.17.1

[PATCH v4 5/5] ethdev: telemetry xstats support hide zero

[Chengwen Feng]

The number of xstats may be large, after the hide zero option is added,
only non-zero values can be displayed.

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 lib/ethdev/rte_ethdev.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index ebcb05b523..a8ba3f2280 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -5870,20 +5870,28 @@ eth_dev_handle_port_xstats(const char *cmd __rte_unused,
 {
 	struct rte_eth_xstat *eth_xstats;
 	struct rte_eth_xstat_name *xstat_names;
+	char *end_param, *hide_param;
 	int port_id, num_xstats;
+	int hide_zero = 0;
 	int i, ret;
-	char *end_param;
 
 	if (params == NULL || strlen(params) == 0 || !isdigit(*params))
 		return -1;
 
 	port_id = strtoul(params, &end_param, 0);
-	if (*end_param != '\0')
-		RTE_ETHDEV_LOG(NOTICE,
-			"Extra parameters passed to ethdev telemetry command, ignoring\n");
 	if (!rte_eth_dev_is_valid_port(port_id))
 		return -1;
 
+	if (*end_param != '\0') {
+		hide_param = strtok(end_param, ",");
+		if (hide_param == NULL || strlen(hide_param) == 0 || !isdigit(*hide_param))
+			return -EINVAL;
+		hide_zero = strtoul(hide_param, &end_param, 0);
+		if (*end_param != '\0')
+			RTE_ETHDEV_LOG(NOTICE,
+				"Extra parameters passed to ethdev telemetry command, ignoring\n");
+	}
+
 	num_xstats = rte_eth_xstats_get(port_id, NULL, 0);
 	if (num_xstats < 0)
 		return -1;
@@ -5908,9 +5916,12 @@ eth_dev_handle_port_xstats(const char *cmd __rte_unused,
 	}
 
 	rte_tel_data_start_dict(d);
-	for (i = 0; i < num_xstats; i++)
+	for (i = 0; i < num_xstats; i++) {
+		if (hide_zero != 0 && eth_xstats[i].value == 0)
+			continue;
 		rte_tel_data_add_dict_u64(d, xstat_names[i].name,
 				eth_xstats[i].value);
+	}
 	free(eth_xstats);
 	return 0;
 }
@@ -6352,7 +6363,7 @@ RTE_INIT(ethdev_init_telemetry)
 	rte_telemetry_register_cmd("/ethdev/stats", eth_dev_handle_port_stats,
 			"Returns the common stats for a port. Parameters: int port_id");
 	rte_telemetry_register_cmd("/ethdev/xstats", eth_dev_handle_port_xstats,
-			"Returns the extended stats for a port. Parameters: int port_id");
+			"Returns the extended stats for a port. Parameters: int port_id, bool hide_zero (Optional, non-zero indicates hide zero xstats)");
 	rte_telemetry_register_cmd("/ethdev/xstats_reset", eth_dev_handle_port_xstats_reset,
 			"Reset the extended stats for a port. Parameters: int port_id");
 #ifndef RTE_EXEC_ENV_WINDOWS
-- 
2.17.1

Re: [PATCH v2 5/5] ethdev: telemetry xstats support hide zero

[fengchengwen]

Hi Bruce,

On 2023/1/11 22:08, Bruce Richardson wrote:
> On Wed, Jan 11, 2023 at 12:06:30PM +0000, Chengwen Feng wrote:
>> The number of xstats may be large, after the hide zero option is added,
>> only non-zero values can be displayed.
>>
>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>> ---
>>  lib/ethdev/rte_ethdev.c | 28 ++++++++++++++++++++++------
>>  1 file changed, 22 insertions(+), 6 deletions(-)
>>
>> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
>> index 2fc593b865..77cacc0829 100644
>> --- a/lib/ethdev/rte_ethdev.c
>> +++ b/lib/ethdev/rte_ethdev.c
>> @@ -5870,20 +5870,33 @@ eth_dev_handle_port_xstats(const char *cmd __rte_unused,
>>  {
>>  	struct rte_eth_xstat *eth_xstats;
>>  	struct rte_eth_xstat_name *xstat_names;
>> +	char *end_param, *hide_param;
>>  	int port_id, num_xstats;
>> +	int hide_zero = 0;
>>  	int i, ret;
>> -	char *end_param;
>>  
>>  	if (params == NULL || strlen(params) == 0 || !isdigit(*params))
>>  		return -1;
>>  
>>  	port_id = strtoul(params, &end_param, 0);
>> -	if (*end_param != '\0')
>> -		RTE_ETHDEV_LOG(NOTICE,
>> -			"Extra parameters passed to ethdev telemetry command, ignoring\n");
>>  	if (!rte_eth_dev_is_valid_port(port_id))
>>  		return -1;
>>  
>> +	if (*end_param != '\0') {
>> +		hide_param = strtok(end_param, ",");
>> +		if (!hide_param || strlen(hide_param) == 0 || !isdigit(*hide_param))
>> +			return -EINVAL;
>> +		hide_zero = strtoul(hide_param, &end_param, 0);
>> +		if (*end_param != '\0')
>> +			RTE_ETHDEV_LOG(NOTICE,
>> +				"Extra parameters passed to ethdev telemetry command, ignoring\n");
>> +		if (hide_zero != 0 && hide_zero != 1) {
>> +			hide_zero = !!hide_zero;
>> +			RTE_ETHDEV_LOG(NOTICE,
>> +				"Hide zero parameter is non-boolean, cast to boolean\n");
>> +		}
>> +	}
>> +
> 
> I'm not sure about this adding of an extra flag as a 0/1 value. That would
> seem to make it confusing with a queue number or extra port number. For
> such an optional parameter, I think a string value would be clearer. A
> number of alternatives I'd suggest - in order of my preference (least
> preferred to most preferred):
> 
> * have the extra parameter as a literal string "hide_zeros" which is
>   matched against rather than looking for 0/1, or alternatively

add string "hide_zeros" requires more characters, it may not user friendly.

> 
> * add a new command /ethdev/xstats_nonzero which does this, the same
>   callback can be reusued, just checking the command given, or finally,

The new command may cause confusion.

> 
> * if this is primarily for the benefit of users using the telemetry script
>   to interactively view stats, then the functionality could be implemented
>   in the script itself rather than in the backend. We could add some
>   setting, or extra flag to the display code, to possibly filter out zeros
>   in the display.

This may causes the display logic to be coupled with the command name.

> 
> Thoughts?

I prefer add optional parameter, and the optional parameter should be simpler, just a number here.

And for clearer expression, I reword the help string in V4:
Returns the extended stats for a port. Parameters: int port_id, bool hide_zero (Optional, non-zero indicates hide zero xstats)

Thanks.

> 
> /Bruce
> .
> 

Re: [PATCH v4 0/5] support dmadev/ethdev stats reset

[Thomas Monjalon]

20/01/2023 04:34, Chengwen Feng:
> This patchset contains dmadev/ethdev stats reset, and also support
> hide zero for ethdev xstats and two telemetry related bugs.
> 
> Chengwen Feng (5):
>   dmadev: support stats reset telemetry command
>   telemetry: fix repeat display when callback don't init dict
>   ethdev: add newline to telemetry log string
>   ethdev: support xstats reset telemetry command
>   ethdev: telemetry xstats support hide zero

Ping for review.

Re: [PATCH v4 2/5] telemetry: fix repeat display when callback don't init dict

[Bruce Richardson]

On Fri, Jan 20, 2023 at 03:34:53AM +0000, Chengwen Feng wrote:
> When a telemetry callback doesn't initialize the telemetry data
> structure and returns a non-negative number, the telemetry will repeat
> to display the last result. This patch zero the data structure to avoid
> the problem.
> 
> Fixes: 6dd571fd07c3 ("telemetry: introduce new functionality")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> Reviewed-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>  lib/telemetry/telemetry.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/telemetry/telemetry.c b/lib/telemetry/telemetry.c
> index 8fbb4f3060..7b905355cd 100644
> --- a/lib/telemetry/telemetry.c
> +++ b/lib/telemetry/telemetry.c
> @@ -333,7 +333,7 @@ output_json(const char *cmd, const struct rte_tel_data *d, int s)
>  static void
>  perform_command(telemetry_cb fn, const char *cmd, const char *param, int s)
>  {
> -	struct rte_tel_data data;
> +	struct rte_tel_data data = {0};
>  
>  	int ret = fn(cmd, param, &data);
>  	if (ret < 0) {
> -- 

Hi Chengwen,

this patch is not directly relevant to the rest of the patchset and is a
necessary fix. Can you perhaps submit this fix separately so it can be
merged, even when the rest of the patchset is looking for reviews?

Thanks,
/Bruce

Re: [PATCH v4 0/5] support dmadev/ethdev stats reset

[Bruce Richardson]

On Fri, Jan 20, 2023 at 03:34:51AM +0000, Chengwen Feng wrote:
> This patchset contains dmadev/ethdev stats reset, and also support
> hide zero for ethdev xstats and two telemetry related bugs.
> 
> Chengwen Feng (5):
>   dmadev: support stats reset telemetry command
>   telemetry: fix repeat display when callback don't init dict
>   ethdev: add newline to telemetry log string
>   ethdev: support xstats reset telemetry command
>   ethdev: telemetry xstats support hide zero
> 
On one level, I can understand how this may be useful, but on the other
side, we also support having multiple different clients connecting to
telemetry simultaneously. Would it not be problematic to have stats
suddenly being reset by one connection without the others being aware of
it?

/Bruce

Re: [PATCH v4 2/5] telemetry: fix repeat display when callback don't init dict

[fengchengwen]

On 2023/2/8 22:15, Bruce Richardson wrote:
> On Fri, Jan 20, 2023 at 03:34:53AM +0000, Chengwen Feng wrote:
>> When a telemetry callback doesn't initialize the telemetry data
>> structure and returns a non-negative number, the telemetry will repeat
>> to display the last result. This patch zero the data structure to avoid
>> the problem.
>>
>> Fixes: 6dd571fd07c3 ("telemetry: introduce new functionality")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>> Reviewed-by: Bruce Richardson <bruce.richardson@intel.com>
>> ---
>>  lib/telemetry/telemetry.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/lib/telemetry/telemetry.c b/lib/telemetry/telemetry.c
>> index 8fbb4f3060..7b905355cd 100644
>> --- a/lib/telemetry/telemetry.c
>> +++ b/lib/telemetry/telemetry.c
>> @@ -333,7 +333,7 @@ output_json(const char *cmd, const struct rte_tel_data *d, int s)
>>  static void
>>  perform_command(telemetry_cb fn, const char *cmd, const char *param, int s)
>>  {
>> -	struct rte_tel_data data;
>> +	struct rte_tel_data data = {0};
>>  
>>  	int ret = fn(cmd, param, &data);
>>  	if (ret < 0) {
>> -- 
> 
> Hi Chengwen,
> 
> this patch is not directly relevant to the rest of the patchset and is a
> necessary fix. Can you perhaps submit this fix separately so it can be
> merged, even when the rest of the patchset is looking for reviews?

done, thanks.

> 
> Thanks,
> /Bruce
> 
> .
> 

Re: [PATCH v4 0/5] support dmadev/ethdev stats reset

[fengchengwen]

On 2023/2/8 22:17, Bruce Richardson wrote:
> On Fri, Jan 20, 2023 at 03:34:51AM +0000, Chengwen Feng wrote:
>> This patchset contains dmadev/ethdev stats reset, and also support
>> hide zero for ethdev xstats and two telemetry related bugs.
>>
>> Chengwen Feng (5):
>>   dmadev: support stats reset telemetry command
>>   telemetry: fix repeat display when callback don't init dict
>>   ethdev: add newline to telemetry log string
>>   ethdev: support xstats reset telemetry command
>>   ethdev: telemetry xstats support hide zero
>>
> On one level, I can understand how this may be useful, but on the other
> side, we also support having multiple different clients connecting to
> telemetry simultaneously. Would it not be problematic to have stats
> suddenly being reset by one connection without the others being aware of
> it?

It's a problem. Anyone who can connect can execute the commands.
For this modify case, I suggest adding LOG for later refer. and will fix in V5.

Thanks.

> 
> /Bruce
> .
> 

[PATCH v5 0/2] support dmadev/ethdev stats reset

[Chengwen Feng]

This patchset contains dmadev/ethdev stats reset.

Chengwen Feng (2):
  dmadev: support stats reset telemetry command
  ethdev: support xstats reset telemetry command

---
v5:
* address Bruce's comments:
  support LOG with stats reset result.
  reassemble the patchset.
v4:
* solve the internal don't commit for 5/5 patch.
v3:
* address Bruce's comment on 1/5 and 2/5 patch, and also for 4/5 patch.
* reword the help info of 5/5 patch.
v2:
* address Bruce's comment on 2/5 patch.
* support output success when stats reset.

 lib/dmadev/rte_dmadev.c | 47 +++++++++++++++++++++++++++++++++++++++++
 lib/ethdev/rte_ethdev.c | 31 +++++++++++++++++++++++++++
 2 files changed, 78 insertions(+)

-- 
2.17.1

[PATCH v5 1/2] dmadev: support stats reset telemetry command

[Chengwen Feng]

The stats reset is useful for debugging, so add it to the dmadev
telemetry command lists.

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
Reviewed-by: Bruce Richardson <bruce.richardson@intel.com>
---
 lib/dmadev/rte_dmadev.c | 47 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/lib/dmadev/rte_dmadev.c b/lib/dmadev/rte_dmadev.c
index 8c095e1f35..acd4cc9750 100644
--- a/lib/dmadev/rte_dmadev.c
+++ b/lib/dmadev/rte_dmadev.c
@@ -994,6 +994,51 @@ dmadev_handle_dev_stats(const char *cmd __rte_unused,
 	return 0;
 }
 
+static int
+dmadev_handle_dev_stats_reset(const char *cmd __rte_unused,
+		const char *params,
+		struct rte_tel_data *d)
+{
+	struct rte_dma_info dma_info;
+	int dev_id, ret, vchan_id;
+	const char *vchan_param;
+	char *end_param;
+
+	if (params == NULL || strlen(params) == 0 || !isdigit(*params))
+		return -EINVAL;
+
+	dev_id = strtoul(params, &end_param, 0);
+
+	/* Function info_get validates dev_id so we don't need to. */
+	ret = rte_dma_info_get(dev_id, &dma_info);
+	if (ret < 0)
+		return -EINVAL;
+
+	/* If the device has one vchan the user does not need to supply the
+	 * vchan id and only the device id is needed, no extra parameters.
+	 */
+	if (dma_info.nb_vchans == 1 && *end_param == '\0') {
+		vchan_id = 0;
+	} else {
+		vchan_param = strtok(end_param, ",");
+		if (vchan_param == NULL || strlen(vchan_param) == 0 || !isdigit(*vchan_param))
+			return -EINVAL;
+		vchan_id = strtoul(vchan_param, &end_param, 0);
+	}
+	if (*end_param != '\0')
+		RTE_DMA_LOG(WARNING, "Extra parameters passed to dmadev telemetry command, ignoring");
+
+	ret = rte_dma_stats_reset(dev_id, vchan_id);
+	if (ret == 0) {
+		rte_tel_data_string(d, "success");
+		RTE_DMA_LOG(NOTICE, "Device %d reset stats success", dev_id);
+	} else {
+		RTE_DMA_LOG(ERR, "Device %d reset stats failed! ret: %d", dev_id, ret);
+	}
+
+	return ret;
+}
+
 #ifndef RTE_EXEC_ENV_WINDOWS
 static int
 dmadev_handle_dev_dump(const char *cmd __rte_unused,
@@ -1041,6 +1086,8 @@ RTE_INIT(dmadev_init_telemetry)
 			"Returns information for a dmadev. Parameters: int dev_id");
 	rte_telemetry_register_cmd("/dmadev/stats", dmadev_handle_dev_stats,
 			"Returns the stats for a dmadev vchannel. Parameters: int dev_id, vchan_id (Optional if only one vchannel)");
+	rte_telemetry_register_cmd("/dmadev/stats_reset", dmadev_handle_dev_stats_reset,
+			"Reset the stats for a dmadev vchannel. Parameters: int dev_id, vchan_id (Optional if only one vchannel)");
 #ifndef RTE_EXEC_ENV_WINDOWS
 	rte_telemetry_register_cmd("/dmadev/dump", dmadev_handle_dev_dump,
 			"Returns dump information for a dmadev. Parameters: int dev_id");
-- 
2.17.1

[PATCH v5 2/2] ethdev: support xstats reset telemetry command

[Chengwen Feng]

The xstats reset is useful for debugging, so add it to the ethdev
telemetry command lists.

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 lib/ethdev/rte_ethdev.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index d25db35f7f..e85c98f307 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -5915,6 +5915,35 @@ eth_dev_handle_port_xstats(const char *cmd __rte_unused,
 	return 0;
 }
 
+static int
+eth_dev_handle_port_xstats_reset(const char *cmd __rte_unused,
+		const char *params,
+		struct rte_tel_data *d)
+{
+	int port_id, ret;
+	char *end_param;
+
+	if (params == NULL || strlen(params) == 0 || !isdigit(*params))
+		return -1;
+
+	port_id = strtoul(params, &end_param, 0);
+	if (*end_param != '\0')
+		RTE_ETHDEV_LOG(NOTICE,
+			"Extra parameters passed to ethdev telemetry command, ignoring\n");
+	if (!rte_eth_dev_is_valid_port(port_id))
+		return -1;
+
+	ret = rte_eth_xstats_reset(port_id);
+	if (ret == 0) {
+		rte_tel_data_string(d, "success");
+		RTE_ETHDEV_LOG(NOTICE, "Port %d reset xstats success\n", port_id);
+	} else {
+		RTE_ETHDEV_LOG(ERR, "Port %d reset xstats failed! ret: %d\n", port_id, ret);
+	}
+
+	return ret;
+}
+
 #ifndef RTE_EXEC_ENV_WINDOWS
 static int
 eth_dev_handle_port_dump_priv(const char *cmd __rte_unused,
@@ -6329,6 +6358,8 @@ RTE_INIT(ethdev_init_telemetry)
 			"Returns the common stats for a port. Parameters: int port_id");
 	rte_telemetry_register_cmd("/ethdev/xstats", eth_dev_handle_port_xstats,
 			"Returns the extended stats for a port. Parameters: int port_id");
+	rte_telemetry_register_cmd("/ethdev/xstats_reset", eth_dev_handle_port_xstats_reset,
+			"Reset the extended stats for a port. Parameters: int port_id");
 #ifndef RTE_EXEC_ENV_WINDOWS
 	rte_telemetry_register_cmd("/ethdev/dump_priv", eth_dev_handle_port_dump_priv,
 			"Returns dump private information for a port. Parameters: int port_id");
-- 
2.17.1

Re: [PATCH v5 2/2] ethdev: support xstats reset telemetry command

[Dongdong Liu]

Hi Chengwen

On 2023/2/9 10:32, Chengwen Feng wrote:
> The xstats reset is useful for debugging, so add it to the ethdev
> telemetry command lists.
>
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
This patch looks good, so
Reviewed-by: Dongdong Liu <liudongdong3@huawei.com>

A minior question
Do we need to support stats reset ?

Thanks,
Dongdong
> ---
>  lib/ethdev/rte_ethdev.c | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
>
> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> index d25db35f7f..e85c98f307 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -5915,6 +5915,35 @@ eth_dev_handle_port_xstats(const char *cmd __rte_unused,
>  	return 0;
>  }
>
> +static int
> +eth_dev_handle_port_xstats_reset(const char *cmd __rte_unused,
> +		const char *params,
> +		struct rte_tel_data *d)
> +{
> +	int port_id, ret;
> +	char *end_param;
> +
> +	if (params == NULL || strlen(params) == 0 || !isdigit(*params))
> +		return -1;
> +
> +	port_id = strtoul(params, &end_param, 0);
> +	if (*end_param != '\0')
> +		RTE_ETHDEV_LOG(NOTICE,
> +			"Extra parameters passed to ethdev telemetry command, ignoring\n");
> +	if (!rte_eth_dev_is_valid_port(port_id))
> +		return -1;
> +
> +	ret = rte_eth_xstats_reset(port_id);
> +	if (ret == 0) {
> +		rte_tel_data_string(d, "success");
> +		RTE_ETHDEV_LOG(NOTICE, "Port %d reset xstats success\n", port_id);
> +	} else {
> +		RTE_ETHDEV_LOG(ERR, "Port %d reset xstats failed! ret: %d\n", port_id, ret);
> +	}
> +
> +	return ret;
> +}
> +
>  #ifndef RTE_EXEC_ENV_WINDOWS
>  static int
>  eth_dev_handle_port_dump_priv(const char *cmd __rte_unused,
> @@ -6329,6 +6358,8 @@ RTE_INIT(ethdev_init_telemetry)
>  			"Returns the common stats for a port. Parameters: int port_id");
>  	rte_telemetry_register_cmd("/ethdev/xstats", eth_dev_handle_port_xstats,
>  			"Returns the extended stats for a port. Parameters: int port_id");
> +	rte_telemetry_register_cmd("/ethdev/xstats_reset", eth_dev_handle_port_xstats_reset,
> +			"Reset the extended stats for a port. Parameters: int port_id");
>  #ifndef RTE_EXEC_ENV_WINDOWS
>  	rte_telemetry_register_cmd("/ethdev/dump_priv", eth_dev_handle_port_dump_priv,
>  			"Returns dump private information for a port. Parameters: int port_id");
>

Re: [PATCH v5 2/2] ethdev: support xstats reset telemetry command

[fengchengwen]

On 2023/2/15 11:19, Dongdong Liu wrote:
> Hi Chengwen
> 
> On 2023/2/9 10:32, Chengwen Feng wrote:
>> The xstats reset is useful for debugging, so add it to the ethdev
>> telemetry command lists.
>>
>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> This patch looks good, so
> Reviewed-by: Dongdong Liu <liudongdong3@huawei.com>
> 
> A minior question
> Do we need to support stats reset ?

Stats is contained by xstats, and future direction I think is xstats.
So I think we don't need support stats reset.

Thanks.

> 
> Thanks,
> Dongdong
>> ---
>>  lib/ethdev/rte_ethdev.c | 31 +++++++++++++++++++++++++++++++
>>  1 file changed, 31 insertions(+)
>>
>> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
>> index d25db35f7f..e85c98f307 100644
>> --- a/lib/ethdev/rte_ethdev.c
>> +++ b/lib/ethdev/rte_ethdev.c
>> @@ -5915,6 +5915,35 @@ eth_dev_handle_port_xstats(const char *cmd __rte_unused,
>>      return 0;
>>  }
>>
>> +static int
>> +eth_dev_handle_port_xstats_reset(const char *cmd __rte_unused,
>> +        const char *params,
>> +        struct rte_tel_data *d)
>> +{
>> +    int port_id, ret;
>> +    char *end_param;
>> +
>> +    if (params == NULL || strlen(params) == 0 || !isdigit(*params))
>> +        return -1;
>> +
>> +    port_id = strtoul(params, &end_param, 0);
>> +    if (*end_param != '\0')
>> +        RTE_ETHDEV_LOG(NOTICE,
>> +            "Extra parameters passed to ethdev telemetry command, ignoring\n");
>> +    if (!rte_eth_dev_is_valid_port(port_id))
>> +        return -1;
>> +
>> +    ret = rte_eth_xstats_reset(port_id);
>> +    if (ret == 0) {
>> +        rte_tel_data_string(d, "success");
>> +        RTE_ETHDEV_LOG(NOTICE, "Port %d reset xstats success\n", port_id);
>> +    } else {
>> +        RTE_ETHDEV_LOG(ERR, "Port %d reset xstats failed! ret: %d\n", port_id, ret);
>> +    }
>> +
>> +    return ret;
>> +}
>> +
>>  #ifndef RTE_EXEC_ENV_WINDOWS
>>  static int
>>  eth_dev_handle_port_dump_priv(const char *cmd __rte_unused,
>> @@ -6329,6 +6358,8 @@ RTE_INIT(ethdev_init_telemetry)
>>              "Returns the common stats for a port. Parameters: int port_id");
>>      rte_telemetry_register_cmd("/ethdev/xstats", eth_dev_handle_port_xstats,
>>              "Returns the extended stats for a port. Parameters: int port_id");
>> +    rte_telemetry_register_cmd("/ethdev/xstats_reset", eth_dev_handle_port_xstats_reset,
>> +            "Reset the extended stats for a port. Parameters: int port_id");
>>  #ifndef RTE_EXEC_ENV_WINDOWS
>>      rte_telemetry_register_cmd("/ethdev/dump_priv", eth_dev_handle_port_dump_priv,
>>              "Returns dump private information for a port. Parameters: int port_id");
>>
> .

Re: [PATCH v5 2/2] ethdev: support xstats reset telemetry command

[Ferruh Yigit]

On 2/16/2023 11:53 AM, fengchengwen wrote:
> On 2023/2/15 11:19, Dongdong Liu wrote:
>> Hi Chengwen
>>
>> On 2023/2/9 10:32, Chengwen Feng wrote:
>>> The xstats reset is useful for debugging, so add it to the ethdev
>>> telemetry command lists.
>>>
>>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>> This patch looks good, so
>> Reviewed-by: Dongdong Liu <liudongdong3@huawei.com>
>>
>> A minior question
>> Do we need to support stats reset ?
> 
> Stats is contained by xstats, and future direction I think is xstats.
> So I think we don't need support stats reset.
> 

I have similar question with Dongdong, readonly values are safe for
telemetry, but modifying data can be more tricky since we don't have
locking in ethdev APIs, this can cause concurrency issues.

Overall do we want telemetry go that way and become something that
alters ethdev data/config?


> Thanks.
> 
>>
>> Thanks,
>> Dongdong
>>> ---
>>>  lib/ethdev/rte_ethdev.c | 31 +++++++++++++++++++++++++++++++
>>>  1 file changed, 31 insertions(+)
>>>
>>> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
>>> index d25db35f7f..e85c98f307 100644
>>> --- a/lib/ethdev/rte_ethdev.c
>>> +++ b/lib/ethdev/rte_ethdev.c
>>> @@ -5915,6 +5915,35 @@ eth_dev_handle_port_xstats(const char *cmd __rte_unused,
>>>      return 0;
>>>  }
>>>
>>> +static int
>>> +eth_dev_handle_port_xstats_reset(const char *cmd __rte_unused,
>>> +        const char *params,
>>> +        struct rte_tel_data *d)
>>> +{
>>> +    int port_id, ret;
>>> +    char *end_param;
>>> +
>>> +    if (params == NULL || strlen(params) == 0 || !isdigit(*params))
>>> +        return -1;
>>> +
>>> +    port_id = strtoul(params, &end_param, 0);
>>> +    if (*end_param != '\0')
>>> +        RTE_ETHDEV_LOG(NOTICE,
>>> +            "Extra parameters passed to ethdev telemetry command, ignoring\n");
>>> +    if (!rte_eth_dev_is_valid_port(port_id))
>>> +        return -1;
>>> +
>>> +    ret = rte_eth_xstats_reset(port_id);
>>> +    if (ret == 0) {
>>> +        rte_tel_data_string(d, "success");
>>> +        RTE_ETHDEV_LOG(NOTICE, "Port %d reset xstats success\n", port_id);
>>> +    } else {
>>> +        RTE_ETHDEV_LOG(ERR, "Port %d reset xstats failed! ret: %d\n", port_id, ret);
>>> +    }
>>> +
>>> +    return ret;
>>> +}
>>> +
>>>  #ifndef RTE_EXEC_ENV_WINDOWS
>>>  static int
>>>  eth_dev_handle_port_dump_priv(const char *cmd __rte_unused,
>>> @@ -6329,6 +6358,8 @@ RTE_INIT(ethdev_init_telemetry)
>>>              "Returns the common stats for a port. Parameters: int port_id");
>>>      rte_telemetry_register_cmd("/ethdev/xstats", eth_dev_handle_port_xstats,
>>>              "Returns the extended stats for a port. Parameters: int port_id");
>>> +    rte_telemetry_register_cmd("/ethdev/xstats_reset", eth_dev_handle_port_xstats_reset,
>>> +            "Reset the extended stats for a port. Parameters: int port_id");
>>>  #ifndef RTE_EXEC_ENV_WINDOWS
>>>      rte_telemetry_register_cmd("/ethdev/dump_priv", eth_dev_handle_port_dump_priv,
>>>              "Returns dump private information for a port. Parameters: int port_id");
>>>
>> .

Re: [PATCH v5 2/2] ethdev: support xstats reset telemetry command

[fengchengwen]

On 2023/2/16 20:06, Ferruh Yigit wrote:
> On 2/16/2023 11:53 AM, fengchengwen wrote:
>> On 2023/2/15 11:19, Dongdong Liu wrote:
>>> Hi Chengwen
>>>
>>> On 2023/2/9 10:32, Chengwen Feng wrote:
>>>> The xstats reset is useful for debugging, so add it to the ethdev
>>>> telemetry command lists.
>>>>
>>>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>>> This patch looks good, so
>>> Reviewed-by: Dongdong Liu <liudongdong3@huawei.com>
>>>
>>> A minior question
>>> Do we need to support stats reset ?
>>
>> Stats is contained by xstats, and future direction I think is xstats.
>> So I think we don't need support stats reset.
>>
> 
> I have similar question with Dongdong, readonly values are safe for
> telemetry, but modifying data can be more tricky since we don't have
> locking in ethdev APIs, this can cause concurrency issues.

Yes, it indeed has concurrency issues.

> 
> Overall do we want telemetry go that way and become something that
> alters ethdev data/config?

There are at least two part of data: config and status.
For stats (which belong status data) could help for debugging, I think it's acceptable.

As for concurrency issues. People should know what to do and when to do, just like
the don't invoke config API (e.g. dev_configure/dev_start/...) concurrency.

> 
> 
>> Thanks.
>>
>>>
>>> Thanks,
>>> Dongdong
>>>> ---
>>>>  lib/ethdev/rte_ethdev.c | 31 +++++++++++++++++++++++++++++++
>>>>  1 file changed, 31 insertions(+)
>>>>
>>>> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
>>>> index d25db35f7f..e85c98f307 100644
>>>> --- a/lib/ethdev/rte_ethdev.c
>>>> +++ b/lib/ethdev/rte_ethdev.c
>>>> @@ -5915,6 +5915,35 @@ eth_dev_handle_port_xstats(const char *cmd __rte_unused,
>>>>      return 0;
>>>>  }
>>>>
>>>> +static int
>>>> +eth_dev_handle_port_xstats_reset(const char *cmd __rte_unused,
>>>> +        const char *params,
>>>> +        struct rte_tel_data *d)
>>>> +{
>>>> +    int port_id, ret;
>>>> +    char *end_param;
>>>> +
>>>> +    if (params == NULL || strlen(params) == 0 || !isdigit(*params))
>>>> +        return -1;
>>>> +
>>>> +    port_id = strtoul(params, &end_param, 0);
>>>> +    if (*end_param != '\0')
>>>> +        RTE_ETHDEV_LOG(NOTICE,
>>>> +            "Extra parameters passed to ethdev telemetry command, ignoring\n");
>>>> +    if (!rte_eth_dev_is_valid_port(port_id))
>>>> +        return -1;
>>>> +
>>>> +    ret = rte_eth_xstats_reset(port_id);
>>>> +    if (ret == 0) {
>>>> +        rte_tel_data_string(d, "success");
>>>> +        RTE_ETHDEV_LOG(NOTICE, "Port %d reset xstats success\n", port_id);
>>>> +    } else {
>>>> +        RTE_ETHDEV_LOG(ERR, "Port %d reset xstats failed! ret: %d\n", port_id, ret);
>>>> +    }
>>>> +
>>>> +    return ret;
>>>> +}
>>>> +
>>>>  #ifndef RTE_EXEC_ENV_WINDOWS
>>>>  static int
>>>>  eth_dev_handle_port_dump_priv(const char *cmd __rte_unused,
>>>> @@ -6329,6 +6358,8 @@ RTE_INIT(ethdev_init_telemetry)
>>>>              "Returns the common stats for a port. Parameters: int port_id");
>>>>      rte_telemetry_register_cmd("/ethdev/xstats", eth_dev_handle_port_xstats,
>>>>              "Returns the extended stats for a port. Parameters: int port_id");
>>>> +    rte_telemetry_register_cmd("/ethdev/xstats_reset", eth_dev_handle_port_xstats_reset,
>>>> +            "Reset the extended stats for a port. Parameters: int port_id");
>>>>  #ifndef RTE_EXEC_ENV_WINDOWS
>>>>      rte_telemetry_register_cmd("/ethdev/dump_priv", eth_dev_handle_port_dump_priv,
>>>>              "Returns dump private information for a port. Parameters: int port_id");
>>>>
>>> .
> 
> .
> 

Re: [PATCH v5 2/2] ethdev: support xstats reset telemetry command

[Bruce Richardson]

On Thu, Feb 16, 2023 at 08:42:34PM +0800, fengchengwen wrote:
> On 2023/2/16 20:06, Ferruh Yigit wrote:
> > On 2/16/2023 11:53 AM, fengchengwen wrote:
> >> On 2023/2/15 11:19, Dongdong Liu wrote:
> >>> Hi Chengwen
> >>>
> >>> On 2023/2/9 10:32, Chengwen Feng wrote:
> >>>> The xstats reset is useful for debugging, so add it to the ethdev
> >>>> telemetry command lists.
> >>>>
> >>>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> >>> This patch looks good, so
> >>> Reviewed-by: Dongdong Liu <liudongdong3@huawei.com>
> >>>
> >>> A minior question
> >>> Do we need to support stats reset ?
> >>
> >> Stats is contained by xstats, and future direction I think is xstats.
> >> So I think we don't need support stats reset.
> >>
> > 
> > I have similar question with Dongdong, readonly values are safe for
> > telemetry, but modifying data can be more tricky since we don't have
> > locking in ethdev APIs, this can cause concurrency issues.
> 
> Yes, it indeed has concurrency issues.
> 
> > 
> > Overall do we want telemetry go that way and become something that
> > alters ethdev data/config?
> 
> There are at least two part of data: config and status.
> For stats (which belong status data) could help for debugging, I think it's acceptable.
> 
> As for concurrency issues. People should know what to do and when to do, just like
> the don't invoke config API (e.g. dev_configure/dev_start/...) concurrency.
> 
While this is probably ok for now, I think in next release we should look
to add some sort of support for locking for destructive ops in a future
release. For example, we could:

1. Add support for marking a callback as "destructive" and only allow it to
be called if only one connection is present or

2. Make it possible for callbacks to query the number of connections so
that the callback itself is non-destructive in more than one connection is
open.

[Both of these will require locking support so that new connections aren't
openned when the callback is in-flight!]

Any other thoughts or suggestions?

/Bruce

Re: [PATCH v5 2/2] ethdev: support xstats reset telemetry command

[Bruce Richardson]

On Thu, Feb 16, 2023 at 12:54:20PM +0000, Bruce Richardson wrote:
> On Thu, Feb 16, 2023 at 08:42:34PM +0800, fengchengwen wrote:
> > On 2023/2/16 20:06, Ferruh Yigit wrote:
> > > On 2/16/2023 11:53 AM, fengchengwen wrote:
> > >> On 2023/2/15 11:19, Dongdong Liu wrote:
> > >>> Hi Chengwen
> > >>>
> > >>> On 2023/2/9 10:32, Chengwen Feng wrote:
> > >>>> The xstats reset is useful for debugging, so add it to the ethdev
> > >>>> telemetry command lists.
> > >>>>
> > >>>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> > >>> This patch looks good, so
> > >>> Reviewed-by: Dongdong Liu <liudongdong3@huawei.com>
> > >>>
> > >>> A minior question
> > >>> Do we need to support stats reset ?
> > >>
> > >> Stats is contained by xstats, and future direction I think is xstats.
> > >> So I think we don't need support stats reset.
> > >>
> > > 
> > > I have similar question with Dongdong, readonly values are safe for
> > > telemetry, but modifying data can be more tricky since we don't have
> > > locking in ethdev APIs, this can cause concurrency issues.
> > 
> > Yes, it indeed has concurrency issues.
> > 
> > > 
> > > Overall do we want telemetry go that way and become something that
> > > alters ethdev data/config?
> > 
> > There are at least two part of data: config and status.
> > For stats (which belong status data) could help for debugging, I think it's acceptable.
> > 
> > As for concurrency issues. People should know what to do and when to do, just like
> > the don't invoke config API (e.g. dev_configure/dev_start/...) concurrency.
> > 
> While this is probably ok for now, I think in next release we should look
> to add some sort of support for locking for destructive ops in a future
> release. For example, we could:
> 
> 1. Add support for marking a callback as "destructive" and only allow it to
> be called if only one connection is present or
> 
> 2. Make it possible for callbacks to query the number of connections so
> that the callback itself is non-destructive in more than one connection is
> open.
> 
> [Both of these will require locking support so that new connections aren't
> openned when the callback is in-flight!]
> 
> Any other thoughts or suggestions?
> 
Actually, another thought - we could also make the max-connections
configurable at runtime, so that the user can enforce only a single
connection.

/Bruce

Re: [PATCH v5 2/2] ethdev: support xstats reset telemetry command

[fengchengwen]

On 2023/2/16 20:54, Bruce Richardson wrote:
> On Thu, Feb 16, 2023 at 08:42:34PM +0800, fengchengwen wrote:
>> On 2023/2/16 20:06, Ferruh Yigit wrote:
>>> On 2/16/2023 11:53 AM, fengchengwen wrote:
>>>> On 2023/2/15 11:19, Dongdong Liu wrote:
>>>>> Hi Chengwen
>>>>>
>>>>> On 2023/2/9 10:32, Chengwen Feng wrote:
>>>>>> The xstats reset is useful for debugging, so add it to the ethdev
>>>>>> telemetry command lists.
>>>>>>
>>>>>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>>>>> This patch looks good, so
>>>>> Reviewed-by: Dongdong Liu <liudongdong3@huawei.com>
>>>>>
>>>>> A minior question
>>>>> Do we need to support stats reset ?
>>>>
>>>> Stats is contained by xstats, and future direction I think is xstats.
>>>> So I think we don't need support stats reset.
>>>>
>>>
>>> I have similar question with Dongdong, readonly values are safe for
>>> telemetry, but modifying data can be more tricky since we don't have
>>> locking in ethdev APIs, this can cause concurrency issues.
>>
>> Yes, it indeed has concurrency issues.
>>
>>>
>>> Overall do we want telemetry go that way and become something that
>>> alters ethdev data/config?
>>
>> There are at least two part of data: config and status.
>> For stats (which belong status data) could help for debugging, I think it's acceptable.
>>
>> As for concurrency issues. People should know what to do and when to do, just like
>> the don't invoke config API (e.g. dev_configure/dev_start/...) concurrency.
>>
> While this is probably ok for now, I think in next release we should look
> to add some sort of support for locking for destructive ops in a future
> release. For example, we could:
> 
> 1. Add support for marking a callback as "destructive" and only allow it to
> be called if only one connection is present or
> 
> 2. Make it possible for callbacks to query the number of connections so
> that the callback itself is non-destructive in more than one connection is
> open.
> 
> [Both of these will require locking support so that new connections aren't
> openned when the callback is in-flight!]

Except telemetry, the application may have other console could execute DPDK API.
So I think trying to keep it simple, it's up to the user to invoke.

> 
> Any other thoughts or suggestions?
> 
> /Bruce
> .
> 

Re: [PATCH v5 2/2] ethdev: support xstats reset telemetry command

[Thomas Monjalon]

17/02/2023 10:44, fengchengwen:
> On 2023/2/16 20:54, Bruce Richardson wrote:
> > On Thu, Feb 16, 2023 at 08:42:34PM +0800, fengchengwen wrote:
> >> On 2023/2/16 20:06, Ferruh Yigit wrote:
> >>> On 2/16/2023 11:53 AM, fengchengwen wrote:
> >>>> On 2023/2/15 11:19, Dongdong Liu wrote:
> >>>>> Hi Chengwen
> >>>>>
> >>>>> On 2023/2/9 10:32, Chengwen Feng wrote:
> >>>>>> The xstats reset is useful for debugging, so add it to the ethdev
> >>>>>> telemetry command lists.
> >>>>>>
> >>>>>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> >>>>> This patch looks good, so
> >>>>> Reviewed-by: Dongdong Liu <liudongdong3@huawei.com>
> >>>>>
> >>>>> A minior question
> >>>>> Do we need to support stats reset ?
> >>>>
> >>>> Stats is contained by xstats, and future direction I think is xstats.
> >>>> So I think we don't need support stats reset.
> >>>>
> >>>
> >>> I have similar question with Dongdong, readonly values are safe for
> >>> telemetry, but modifying data can be more tricky since we don't have
> >>> locking in ethdev APIs, this can cause concurrency issues.
> >>
> >> Yes, it indeed has concurrency issues.
> >>
> >>>
> >>> Overall do we want telemetry go that way and become something that
> >>> alters ethdev data/config?
> >>
> >> There are at least two part of data: config and status.
> >> For stats (which belong status data) could help for debugging, I think it's acceptable.
> >>
> >> As for concurrency issues. People should know what to do and when to do, just like
> >> the don't invoke config API (e.g. dev_configure/dev_start/...) concurrency.
> >>
> > While this is probably ok for now, I think in next release we should look
> > to add some sort of support for locking for destructive ops in a future
> > release. For example, we could:
> > 
> > 1. Add support for marking a callback as "destructive" and only allow it to
> > be called if only one connection is present or
> > 
> > 2. Make it possible for callbacks to query the number of connections so
> > that the callback itself is non-destructive in more than one connection is
> > open.
> > 
> > [Both of these will require locking support so that new connections aren't
> > openned when the callback is in-flight!]
> 
> Except telemetry, the application may have other console could execute DPDK API.
> So I think trying to keep it simple, it's up to the user to invoke.

No, the user should not be responsible for concurrency issues.
We can ask the app developper to take care,
but not to the user who has no control on what happens in the app.

On a more general note, I feel the expansion of telemetry is not controlled enough.
I would like to stop on adding more telemetry until we have a clear guideline
about what is telemetry for and how to use it.

Re: [PATCH v5 2/2] ethdev: support xstats reset telemetry command

[fengchengwen]

On 2023/2/20 21:05, Thomas Monjalon wrote:
> 17/02/2023 10:44, fengchengwen:
>> On 2023/2/16 20:54, Bruce Richardson wrote:
>>> On Thu, Feb 16, 2023 at 08:42:34PM +0800, fengchengwen wrote:
>>>> On 2023/2/16 20:06, Ferruh Yigit wrote:
>>>>> On 2/16/2023 11:53 AM, fengchengwen wrote:
>>>>>> On 2023/2/15 11:19, Dongdong Liu wrote:
>>>>>>> Hi Chengwen
>>>>>>>
>>>>>>> On 2023/2/9 10:32, Chengwen Feng wrote:
>>>>>>>> The xstats reset is useful for debugging, so add it to the ethdev
>>>>>>>> telemetry command lists.
>>>>>>>>
>>>>>>>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>>>>>>> This patch looks good, so
>>>>>>> Reviewed-by: Dongdong Liu <liudongdong3@huawei.com>
>>>>>>>
>>>>>>> A minior question
>>>>>>> Do we need to support stats reset ?
>>>>>>
>>>>>> Stats is contained by xstats, and future direction I think is xstats.
>>>>>> So I think we don't need support stats reset.
>>>>>>
>>>>>
>>>>> I have similar question with Dongdong, readonly values are safe for
>>>>> telemetry, but modifying data can be more tricky since we don't have
>>>>> locking in ethdev APIs, this can cause concurrency issues.
>>>>
>>>> Yes, it indeed has concurrency issues.
>>>>
>>>>>
>>>>> Overall do we want telemetry go that way and become something that
>>>>> alters ethdev data/config?
>>>>
>>>> There are at least two part of data: config and status.
>>>> For stats (which belong status data) could help for debugging, I think it's acceptable.
>>>>
>>>> As for concurrency issues. People should know what to do and when to do, just like
>>>> the don't invoke config API (e.g. dev_configure/dev_start/...) concurrency.
>>>>
>>> While this is probably ok for now, I think in next release we should look
>>> to add some sort of support for locking for destructive ops in a future
>>> release. For example, we could:
>>>
>>> 1. Add support for marking a callback as "destructive" and only allow it to
>>> be called if only one connection is present or
>>>
>>> 2. Make it possible for callbacks to query the number of connections so
>>> that the callback itself is non-destructive in more than one connection is
>>> open.
>>>
>>> [Both of these will require locking support so that new connections aren't
>>> openned when the callback is in-flight!]
>>
>> Except telemetry, the application may have other console could execute DPDK API.
>> So I think trying to keep it simple, it's up to the user to invoke.
> 
> No, the user should not be responsible for concurrency issues.
> We can ask the app developper to take care,
> but not to the user who has no control on what happens in the app.
> 
> On a more general note, I feel the expansion of telemetry is not controlled enough.
> I would like to stop on adding more telemetry until we have a clear guideline
> about what is telemetry for and how to use it.

Hi Thomas,

Should this be discussed on TB?

Thanks.

> 
> 
> .
> 

Re: [PATCH v5 2/2] ethdev: support xstats reset telemetry command

[Thomas Monjalon]

03/07/2023 05:58, fengchengwen:
> 
> On 2023/2/20 21:05, Thomas Monjalon wrote:
> > 17/02/2023 10:44, fengchengwen:
> >> On 2023/2/16 20:54, Bruce Richardson wrote:
> >>> On Thu, Feb 16, 2023 at 08:42:34PM +0800, fengchengwen wrote:
> >>>> On 2023/2/16 20:06, Ferruh Yigit wrote:
> >>>>> On 2/16/2023 11:53 AM, fengchengwen wrote:
> >>>>>> On 2023/2/15 11:19, Dongdong Liu wrote:
> >>>>>>> Hi Chengwen
> >>>>>>>
> >>>>>>> On 2023/2/9 10:32, Chengwen Feng wrote:
> >>>>>>>> The xstats reset is useful for debugging, so add it to the ethdev
> >>>>>>>> telemetry command lists.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> >>>>>>> This patch looks good, so
> >>>>>>> Reviewed-by: Dongdong Liu <liudongdong3@huawei.com>
> >>>>>>>
> >>>>>>> A minior question
> >>>>>>> Do we need to support stats reset ?
> >>>>>>
> >>>>>> Stats is contained by xstats, and future direction I think is xstats.
> >>>>>> So I think we don't need support stats reset.
> >>>>>>
> >>>>>
> >>>>> I have similar question with Dongdong, readonly values are safe for
> >>>>> telemetry, but modifying data can be more tricky since we don't have
> >>>>> locking in ethdev APIs, this can cause concurrency issues.
> >>>>
> >>>> Yes, it indeed has concurrency issues.
> >>>>
> >>>>>
> >>>>> Overall do we want telemetry go that way and become something that
> >>>>> alters ethdev data/config?
> >>>>
> >>>> There are at least two part of data: config and status.
> >>>> For stats (which belong status data) could help for debugging, I think it's acceptable.
> >>>>
> >>>> As for concurrency issues. People should know what to do and when to do, just like
> >>>> the don't invoke config API (e.g. dev_configure/dev_start/...) concurrency.
> >>>>
> >>> While this is probably ok for now, I think in next release we should look
> >>> to add some sort of support for locking for destructive ops in a future
> >>> release. For example, we could:
> >>>
> >>> 1. Add support for marking a callback as "destructive" and only allow it to
> >>> be called if only one connection is present or
> >>>
> >>> 2. Make it possible for callbacks to query the number of connections so
> >>> that the callback itself is non-destructive in more than one connection is
> >>> open.
> >>>
> >>> [Both of these will require locking support so that new connections aren't
> >>> openned when the callback is in-flight!]
> >>
> >> Except telemetry, the application may have other console could execute DPDK API.
> >> So I think trying to keep it simple, it's up to the user to invoke.
> > 
> > No, the user should not be responsible for concurrency issues.
> > We can ask the app developper to take care,
> > but not to the user who has no control on what happens in the app.
> > 
> > On a more general note, I feel the expansion of telemetry is not controlled enough.
> > I would like to stop on adding more telemetry until we have a clear guideline
> > about what is telemetry for and how to use it.
> 
> Hi Thomas,
> 
> Should this be discussed on TB?

What would be your question exactly?

RE: [PATCH v5 2/2] ethdev: support xstats reset telemetry command

[Morten Brørup]

> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Monday, 3 July 2023 09.20
> 
> 03/07/2023 05:58, fengchengwen:
> >
> > On 2023/2/20 21:05, Thomas Monjalon wrote:
> > > 17/02/2023 10:44, fengchengwen:
> > >> On 2023/2/16 20:54, Bruce Richardson wrote:
> > >>> On Thu, Feb 16, 2023 at 08:42:34PM +0800, fengchengwen wrote:
> > >>>> On 2023/2/16 20:06, Ferruh Yigit wrote:
> > >>>>> On 2/16/2023 11:53 AM, fengchengwen wrote:
> > >>>>>> On 2023/2/15 11:19, Dongdong Liu wrote:
> > >>>>>>> Hi Chengwen
> > >>>>>>>
> > >>>>>>> On 2023/2/9 10:32, Chengwen Feng wrote:
> > >>>>>>>> The xstats reset is useful for debugging, so add it to the
> ethdev
> > >>>>>>>> telemetry command lists.
> > >>>>>>>>
> > >>>>>>>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> > >>>>>>> This patch looks good, so
> > >>>>>>> Reviewed-by: Dongdong Liu <liudongdong3@huawei.com>
> > >>>>>>>
> > >>>>>>> A minior question
> > >>>>>>> Do we need to support stats reset ?
> > >>>>>>
> > >>>>>> Stats is contained by xstats, and future direction I think is
> xstats.
> > >>>>>> So I think we don't need support stats reset.
> > >>>>>>
> > >>>>>
> > >>>>> I have similar question with Dongdong, readonly values are safe
> for
> > >>>>> telemetry, but modifying data can be more tricky since we don't
> have
> > >>>>> locking in ethdev APIs, this can cause concurrency issues.
> > >>>>
> > >>>> Yes, it indeed has concurrency issues.
> > >>>>
> > >>>>>
> > >>>>> Overall do we want telemetry go that way and become something
> that
> > >>>>> alters ethdev data/config?
> > >>>>
> > >>>> There are at least two part of data: config and status.
> > >>>> For stats (which belong status data) could help for debugging, I
> think it's acceptable.
> > >>>>
> > >>>> As for concurrency issues. People should know what to do and when
> to do, just like
> > >>>> the don't invoke config API (e.g. dev_configure/dev_start/...)
> concurrency.
> > >>>>
> > >>> While this is probably ok for now, I think in next release we
> should look
> > >>> to add some sort of support for locking for destructive ops in a
> future
> > >>> release. For example, we could:
> > >>>
> > >>> 1. Add support for marking a callback as "destructive" and only
> allow it to
> > >>> be called if only one connection is present or
> > >>>
> > >>> 2. Make it possible for callbacks to query the number of
> connections so
> > >>> that the callback itself is non-destructive in more than one
> connection is
> > >>> open.
> > >>>
> > >>> [Both of these will require locking support so that new
> connections aren't
> > >>> openned when the callback is in-flight!]
> > >>
> > >> Except telemetry, the application may have other console could
> execute DPDK API.
> > >> So I think trying to keep it simple, it's up to the user to invoke.
> > >
> > > No, the user should not be responsible for concurrency issues.
> > > We can ask the app developper to take care,
> > > but not to the user who has no control on what happens in the app.
> > >
> > > On a more general note, I feel the expansion of telemetry is not
> controlled enough.
> > > I would like to stop on adding more telemetry until we have a clear
> guideline
> > > about what is telemetry for and how to use it.
> >
> > Hi Thomas,
> >
> > Should this be discussed on TB?
> 
> What would be your question exactly?

A general comment about telemetry:

If an application exposes telemetry through an end user facing API, e.g. http(s) REST, it would be nice if non-read-only telemetry paths are easy to identify by following some DPDK standard convention, so the application does not need to manually maintain an allow-list of read-only paths.

Bruce's documentation about trace/log/telemetry/dump might also need to be updated regarding non-read-only telemetry actions.

Re: [PATCH v5 2/2] ethdev: support xstats reset telemetry command

[fengchengwen]

Hi Thomas and Morten,

On 2023/7/3 21:44, Morten Brørup wrote:
>> From: Thomas Monjalon [mailto:thomas@monjalon.net]
>> Sent: Monday, 3 July 2023 09.20
>>
>> 03/07/2023 05:58, fengchengwen:
>>>
>>> On 2023/2/20 21:05, Thomas Monjalon wrote:
>>>> 17/02/2023 10:44, fengchengwen:
>>>>> On 2023/2/16 20:54, Bruce Richardson wrote:
>>>>>> On Thu, Feb 16, 2023 at 08:42:34PM +0800, fengchengwen wrote:
>>>>>>> On 2023/2/16 20:06, Ferruh Yigit wrote:
>>>>>>>> On 2/16/2023 11:53 AM, fengchengwen wrote:
>>>>>>>>> On 2023/2/15 11:19, Dongdong Liu wrote:
>>>>>>>>>> Hi Chengwen
>>>>>>>>>>
>>>>>>>>>> On 2023/2/9 10:32, Chengwen Feng wrote:
>>>>>>>>>>> The xstats reset is useful for debugging, so add it to the
>> ethdev
>>>>>>>>>>> telemetry command lists.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>>>>>>>>>> This patch looks good, so
>>>>>>>>>> Reviewed-by: Dongdong Liu <liudongdong3@huawei.com>
>>>>>>>>>>
>>>>>>>>>> A minior question
>>>>>>>>>> Do we need to support stats reset ?
>>>>>>>>>
>>>>>>>>> Stats is contained by xstats, and future direction I think is
>> xstats.
>>>>>>>>> So I think we don't need support stats reset.
>>>>>>>>>
>>>>>>>>
>>>>>>>> I have similar question with Dongdong, readonly values are safe
>> for
>>>>>>>> telemetry, but modifying data can be more tricky since we don't
>> have
>>>>>>>> locking in ethdev APIs, this can cause concurrency issues.
>>>>>>>
>>>>>>> Yes, it indeed has concurrency issues.
>>>>>>>
>>>>>>>>
>>>>>>>> Overall do we want telemetry go that way and become something
>> that
>>>>>>>> alters ethdev data/config?
>>>>>>>
>>>>>>> There are at least two part of data: config and status.
>>>>>>> For stats (which belong status data) could help for debugging, I
>> think it's acceptable.
>>>>>>>
>>>>>>> As for concurrency issues. People should know what to do and when
>> to do, just like
>>>>>>> the don't invoke config API (e.g. dev_configure/dev_start/...)
>> concurrency.
>>>>>>>
>>>>>> While this is probably ok for now, I think in next release we
>> should look
>>>>>> to add some sort of support for locking for destructive ops in a
>> future
>>>>>> release. For example, we could:
>>>>>>
>>>>>> 1. Add support for marking a callback as "destructive" and only
>> allow it to
>>>>>> be called if only one connection is present or
>>>>>>
>>>>>> 2. Make it possible for callbacks to query the number of
>> connections so
>>>>>> that the callback itself is non-destructive in more than one
>> connection is
>>>>>> open.
>>>>>>
>>>>>> [Both of these will require locking support so that new
>> connections aren't
>>>>>> openned when the callback is in-flight!]
>>>>>
>>>>> Except telemetry, the application may have other console could
>> execute DPDK API.
>>>>> So I think trying to keep it simple, it's up to the user to invoke.
>>>>
>>>> No, the user should not be responsible for concurrency issues.
>>>> We can ask the app developper to take care,
>>>> but not to the user who has no control on what happens in the app.
>>>>
>>>> On a more general note, I feel the expansion of telemetry is not
>> controlled enough.
>>>> I would like to stop on adding more telemetry until we have a clear
>> guideline
>>>> about what is telemetry for and how to use it.
>>>
>>> Hi Thomas,
>>>
>>> Should this be discussed on TB?
>>
>> What would be your question exactly?
> 
> A general comment about telemetry:
> 
> If an application exposes telemetry through an end user facing API, e.g. http(s) REST, it would be nice if non-read-only telemetry paths are easy to identify by following some DPDK standard convention, so the application does not need to manually maintain an allow-list of read-only paths.

+1 for this point.

> 
> Bruce's documentation about trace/log/telemetry/dump might also need to be updated regarding non-read-only telemetry actions.

I just check Bruce's patch [1], and notice that the telemetry callback must be 'read-only': (Telemetry callbacks should not modify any program state, but be "read-only").

From internal product usage, we think xstats-reset is valid to identify problem, but this callback is not read-only.

We think telemetry callback should not limit to 'read-only'. Perhaps we could develop some strategy to better manage non-read-only callbacks (just like Morten's advise).

[1]: https://patchwork.dpdk.org/project/dpdk/patch/20230620170728.74117-3-bruce.richardson@intel.com/

> 
> 
> .
> 

Re: [PATCH v5 2/2] ethdev: support xstats reset telemetry command

[Stephen Hemminger]

On Tue, 4 Jul 2023 14:41:27 +0800
fengchengwen <fengchengwen@huawei.com> wrote:

> > A general comment about telemetry:
> > 
> > If an application exposes telemetry through an end user facing API, e.g. http(s) REST, it would be nice if non-read-only telemetry paths are easy to identify by following some DPDK standard convention, so the application does not need to manually maintain an allow-list of read-only paths.  
> 
> +1 for this point.
> 
> > 
> > Bruce's documentation about trace/log/telemetry/dump might also need to be updated regarding non-read-only telemetry actions.  
> 
> I just check Bruce's patch [1], and notice that the telemetry callback must be 'read-only': (Telemetry callbacks should not modify any program state, but be "read-only").
> 
> From internal product usage, we think xstats-reset is valid to identify problem, but this callback is not read-only.
> 
> We think telemetry callback should not limit to 'read-only'. Perhaps we could develop some strategy to better manage non-read-only callbacks (just like Morten's advise).
> 
> [1]: https://patchwork.dpdk.org/project/dpdk/patch/20230620170728.74117-3-bruce.richardson@intel.com/

This patch set had functional test failures, and there never was a good conclusion about allowing telemetry to do reset.

If you want to keep going with it, go through the feedback and resubmit.