[PATCH] ethdev: telemetry xstats support hide zero

[PATCH] 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 d25db35f7f..8f79ae45d5 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");
 	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");
+	}
+
 	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_uint(d, xstat_names[i].name,
 					   eth_xstats[i].value);
+	}
 	free(eth_xstats);
 	return 0;
 }
@@ -6328,7 +6339,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)");
 #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] ethdev: telemetry xstats support hide zero

[Ferruh Yigit]

On 2/9/2023 3:07 AM, Chengwen Feng wrote:
> The number of xstats may be large, after the hide zero option is added,
> only non-zero values can be displayed.
> 

Hi Chengwen,

No objection on the functionality, we have similar config option in
testpmd too, but I have some question on telemetry side of things:

1) optional parameters, I don't know if there is a defined way to handle
this in telemetry but with current method only one optional parameter
can be supported and it has to be the last one. In the feature if a new
mandatory option required, this changes the user interface. To prevent
this, is it possible to use "key=value" syntax, like
"/ethdev/xstats,0,hide_zero=true"

This way order or existence of multiple options doesn't matter.


2) Where should be this functionality, it is possible to filter out zero
values either in dpdk side or telemetry client side.
Just for this one I think both options are OK, but if there is a
possibility to have multiple and complex filtering, I think that should
go to the client side since this is not really task of the dpdk library.


> 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 d25db35f7f..8f79ae45d5 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");
>  	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");
> +	}
> +
>  	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_uint(d, xstat_names[i].name,
>  					   eth_xstats[i].value);
> +	}
>  	free(eth_xstats);
>  	return 0;
>  }
> @@ -6328,7 +6339,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)");
>  #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] ethdev: telemetry xstats support hide zero

[fengchengwen]

On 2023/2/10 7:30, Ferruh Yigit wrote:
> On 2/9/2023 3:07 AM, Chengwen Feng wrote:
>> The number of xstats may be large, after the hide zero option is added,
>> only non-zero values can be displayed.
>>
> 
> Hi Chengwen,
> 
> No objection on the functionality, we have similar config option in
> testpmd too, but I have some question on telemetry side of things:
> 
> 1) optional parameters, I don't know if there is a defined way to handle
> this in telemetry but with current method only one optional parameter
> can be supported and it has to be the last one. In the feature if a new
> mandatory option required, this changes the user interface. To prevent
> this, is it possible to use "key=value" syntax, like
> "/ethdev/xstats,0,hide_zero=true"
> 
> This way order or existence of multiple options doesn't matter.

Yes, KV (just like PMD's runtime parameters) is more flexible for multiple optional parameters.
AFAIK, only some dmadev commands have optional parameters (which using this patch way to identify).

Until there are more parameters, I think we can keep the status quo.

> 
> 
> 2) Where should be this functionality, it is possible to filter out zero
> values either in dpdk side or telemetry client side.
> Just for this one I think both options are OK, but if there is a
> possibility to have multiple and complex filtering, I think that should
> go to the client side since this is not really task of the dpdk library.

Agree.
This patch just target who using telemetry.py directly, it's valuable in this scenario.
If clients supports filtering, they could use original way (don't add options).

Thanks.

> 
> 
>> 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 d25db35f7f..8f79ae45d5 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");
>>  	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");
>> +	}
>> +
>>  	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_uint(d, xstat_names[i].name,
>>  					   eth_xstats[i].value);
>> +	}
>>  	free(eth_xstats);
>>  	return 0;
>>  }
>> @@ -6328,7 +6339,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)");
>>  #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] ethdev: telemetry xstats support hide zero

[Ferruh Yigit]

On 2/10/2023 1:23 AM, fengchengwen wrote:
> On 2023/2/10 7:30, Ferruh Yigit wrote:
>> On 2/9/2023 3:07 AM, Chengwen Feng wrote:
>>> The number of xstats may be large, after the hide zero option is added,
>>> only non-zero values can be displayed.
>>>
>>
>> Hi Chengwen,
>>
>> No objection on the functionality, we have similar config option in
>> testpmd too, but I have some question on telemetry side of things:
>>
>> 1) optional parameters, I don't know if there is a defined way to handle
>> this in telemetry but with current method only one optional parameter
>> can be supported and it has to be the last one. In the feature if a new
>> mandatory option required, this changes the user interface. To prevent
>> this, is it possible to use "key=value" syntax, like
>> "/ethdev/xstats,0,hide_zero=true"
>>
>> This way order or existence of multiple options doesn't matter.
> 
> Yes, KV (just like PMD's runtime parameters) is more flexible for multiple optional parameters.
> AFAIK, only some dmadev commands have optional parameters (which using this patch way to identify).
> 
> Until there are more parameters, I think we can keep the status quo.
> 

I think better to start using it with first optional parameter, which is
this patch, otherwise it will be pushing the work to next contributor.

>>
>>
>> 2) Where should be this functionality, it is possible to filter out zero
>> values either in dpdk side or telemetry client side.
>> Just for this one I think both options are OK, but if there is a
>> possibility to have multiple and complex filtering, I think that should
>> go to the client side since this is not really task of the dpdk library.
> 
> Agree.
> This patch just target who using telemetry.py directly, it's valuable in this scenario.
> If clients supports filtering, they could use original way (don't add options).
> 

OK, I don't have strong option, if there is no objection we can have
this functionality in dpdk side.

> Thanks.
> 
>>
>>
>>> 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 d25db35f7f..8f79ae45d5 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");
>>>  	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");
>>> +	}
>>> +
>>>  	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_uint(d, xstat_names[i].name,
>>>  					   eth_xstats[i].value);
>>> +	}
>>>  	free(eth_xstats);
>>>  	return 0;
>>>  }
>>> @@ -6328,7 +6339,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)");
>>>  #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");
>>
>> .
>>

[PATCH v2] 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.

So display xstats with hide zero:
	/ethdev/xstats,0,hide_zero=true
and without hide zero (same as the original):
	/ethdev/xstats,0

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>

---
v2: using hide_zero=true optional parameter which address Ferruh's
comments.

---
 lib/ethdev/rte_ethdev.c | 38 ++++++++++++++++++++++++++++++++------
 1 file changed, 32 insertions(+), 6 deletions(-)

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index d25db35f7f..7e70292fc6 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -15,6 +15,7 @@
 #include <bus_driver.h>
 #include <rte_log.h>
 #include <rte_interrupts.h>
+#include <rte_kvargs.h>
 #include <rte_memcpy.h>
 #include <rte_common.h>
 #include <rte_mempool.h>
@@ -5863,6 +5864,18 @@ eth_dev_handle_port_stats(const char *cmd __rte_unused,
 	return 0;
 }
 
+#define TEL_XSTATS_HIDE_ZERO_KEY	"hide_zero"
+#define TEL_XSTATS_HIDE_ZERO_VAL	"true"
+
+static int
+eth_dev_parse_hide_zero(const char *key, const char *value, void *extra_args)
+{
+	RTE_SET_USED(key);
+	if (strcmp(value, TEL_XSTATS_HIDE_ZERO_VAL) == 0)
+		*(bool *)extra_args = true;
+	return 0;
+}
+
 static int
 eth_dev_handle_port_xstats(const char *cmd __rte_unused,
 		const char *params,
@@ -5870,20 +5883,30 @@ eth_dev_handle_port_xstats(const char *cmd __rte_unused,
 {
 	struct rte_eth_xstat *eth_xstats;
 	struct rte_eth_xstat_name *xstat_names;
+	struct rte_kvargs *kvlist;
 	int port_id, num_xstats;
-	int i, ret;
+	bool hide_zero = false;
 	char *end_param;
+	int i, ret;
 
 	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') {
+		kvlist = rte_kvargs_parse(end_param, NULL);
+		if (kvlist != NULL)
+			rte_kvargs_process(kvlist, TEL_XSTATS_HIDE_ZERO_KEY,
+					   eth_dev_parse_hide_zero, &hide_zero);
+		if (kvlist == NULL || !hide_zero || kvlist->count > 1)
+			RTE_ETHDEV_LOG(NOTICE,
+				"Unknown extra parameters passed to ethdev telemetry command, ignoring\n");
+		rte_kvargs_free(kvlist);
+	}
+
 	num_xstats = rte_eth_xstats_get(port_id, NULL, 0);
 	if (num_xstats < 0)
 		return -1;
@@ -5908,9 +5931,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_uint(d, xstat_names[i].name,
 					   eth_xstats[i].value);
+	}
 	free(eth_xstats);
 	return 0;
 }
@@ -6328,7 +6354,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=true(Optional for indicates hide zero xstats)");
 #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] ethdev: telemetry xstats support hide zero

[fengchengwen]

On 2023/2/10 19:55, Ferruh Yigit wrote:
> On 2/10/2023 1:23 AM, fengchengwen wrote:
>> On 2023/2/10 7:30, Ferruh Yigit wrote:
>>> On 2/9/2023 3:07 AM, Chengwen Feng wrote:
>>>> The number of xstats may be large, after the hide zero option is added,
>>>> only non-zero values can be displayed.
>>>>
>>>
>>> Hi Chengwen,
>>>
>>> No objection on the functionality, we have similar config option in
>>> testpmd too, but I have some question on telemetry side of things:
>>>
>>> 1) optional parameters, I don't know if there is a defined way to handle
>>> this in telemetry but with current method only one optional parameter
>>> can be supported and it has to be the last one. In the feature if a new
>>> mandatory option required, this changes the user interface. To prevent
>>> this, is it possible to use "key=value" syntax, like
>>> "/ethdev/xstats,0,hide_zero=true"
>>>
>>> This way order or existence of multiple options doesn't matter.
>>
>> Yes, KV (just like PMD's runtime parameters) is more flexible for multiple optional parameters.
>> AFAIK, only some dmadev commands have optional parameters (which using this patch way to identify).
>>
>> Until there are more parameters, I think we can keep the status quo.
>>
> 
> I think better to start using it with first optional parameter, which is
> this patch, otherwise it will be pushing the work to next contributor.

v2 implements it, please review, thanks.

> 
>>>
>>>
>>> 2) Where should be this functionality, it is possible to filter out zero
>>> values either in dpdk side or telemetry client side.
>>> Just for this one I think both options are OK, but if there is a
>>> possibility to have multiple and complex filtering, I think that should
>>> go to the client side since this is not really task of the dpdk library.
>>
>> Agree.
>> This patch just target who using telemetry.py directly, it's valuable in this scenario.
>> If clients supports filtering, they could use original way (don't add options).
>>
> 
> OK, I don't have strong option, if there is no objection we can have
> this functionality in dpdk side.
> 
>> Thanks.
>>
>>>
>>>
>>>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>>>> ---

...

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

[Ferruh Yigit]

On 2/13/2023 2:34 AM, Chengwen Feng wrote:
> The number of xstats may be large, after the hide zero option is added,
> only non-zero values can be displayed.
> 
> So display xstats with hide zero:
> 	/ethdev/xstats,0,hide_zero=true
> and without hide zero (same as the original):
> 	/ethdev/xstats,0
> 
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> 
> ---
> v2: using hide_zero=true optional parameter which address Ferruh's
> comments.
> 
> ---
>  lib/ethdev/rte_ethdev.c | 38 ++++++++++++++++++++++++++++++++------
>  1 file changed, 32 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> index d25db35f7f..7e70292fc6 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -15,6 +15,7 @@
>  #include <bus_driver.h>
>  #include <rte_log.h>
>  #include <rte_interrupts.h>
> +#include <rte_kvargs.h>
>  #include <rte_memcpy.h>
>  #include <rte_common.h>
>  #include <rte_mempool.h>
> @@ -5863,6 +5864,18 @@ eth_dev_handle_port_stats(const char *cmd __rte_unused,
>  	return 0;
>  }
>  
> +#define TEL_XSTATS_HIDE_ZERO_KEY	"hide_zero"
> +#define TEL_XSTATS_HIDE_ZERO_VAL	"true"
> +
> +static int
> +eth_dev_parse_hide_zero(const char *key, const char *value, void *extra_args)
> +{
> +	RTE_SET_USED(key);
> +	if (strcmp(value, TEL_XSTATS_HIDE_ZERO_VAL) == 0)


kvargs allows to provide parameter without value, like
'/ethdev/xstats,0,hide_zero'

In this case 'value' will be NULL and this cause a crash in dpdk
application.
Better to check that 'value' is not NULL before using it.

> +		*(bool *)extra_args = true;
> +	return 0;
> +}
> +
>  static int
>  eth_dev_handle_port_xstats(const char *cmd __rte_unused,
>  		const char *params,
> @@ -5870,20 +5883,30 @@ eth_dev_handle_port_xstats(const char *cmd __rte_unused,
>  {
>  	struct rte_eth_xstat *eth_xstats;
>  	struct rte_eth_xstat_name *xstat_names;
> +	struct rte_kvargs *kvlist;
>  	int port_id, num_xstats;
> -	int i, ret;
> +	bool hide_zero = false;
>  	char *end_param;
> +	int i, ret;
>  
>  	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') {
> +		kvlist = rte_kvargs_parse(end_param, NULL);
> +		if (kvlist != NULL)
> +			rte_kvargs_process(kvlist, TEL_XSTATS_HIDE_ZERO_KEY,
> +					   eth_dev_parse_hide_zero, &hide_zero);
> +		if (kvlist == NULL || !hide_zero || kvlist->count > 1)

Instead of "kvlist->count > 1" can be better to use 'valid_keys'
argument of 'rte_kvargs_parse()' to detect extra parameters, this way
multiple 'hide_zero' parameters or future expansion of args won't be
blocked.


Similarly, '!hide_zero' check prevents "hide_zero=false" usage, it is
not documented but this is expected to be supported intuitively, perhaps
can be better to support "hide_zero=false" fully, document it and remove
'!hide_zero' from check?

> +			RTE_ETHDEV_LOG(NOTICE,
> +				"Unknown extra parameters passed to ethdev telemetry command, ignoring\n");
> +		rte_kvargs_free(kvlist);
> +	}
> +
>  	num_xstats = rte_eth_xstats_get(port_id, NULL, 0);
>  	if (num_xstats < 0)
>  		return -1;
> @@ -5908,9 +5931,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_uint(d, xstat_names[i].name,
>  					   eth_xstats[i].value);
> +	}
>  	free(eth_xstats);
>  	return 0;
>  }
> @@ -6328,7 +6354,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=true(Optional for indicates hide zero xstats)");
>  #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");

[PATCH v3] 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.

So display xstats with hide zero:
	/ethdev/xstats,0,hide_zero=true
and without hide zero:
	/ethdev/xstats,0,hide_zero=false  or
	/ethdev/xstats,0

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>

---
v3: fix only hide_zero cause segment fault, support hide_zero=false,
    and these address Ferruh's comments.
v2: using hide_zero=true optional parameter which address Ferruh's
    comments.

---
 lib/ethdev/rte_ethdev.c | 43 +++++++++++++++++++++++++++++++++++------
 1 file changed, 37 insertions(+), 6 deletions(-)

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index d25db35f7f..fa98af1f91 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -15,6 +15,7 @@
 #include <bus_driver.h>
 #include <rte_log.h>
 #include <rte_interrupts.h>
+#include <rte_kvargs.h>
 #include <rte_memcpy.h>
 #include <rte_common.h>
 #include <rte_mempool.h>
@@ -5863,27 +5864,54 @@ eth_dev_handle_port_stats(const char *cmd __rte_unused,
 	return 0;
 }
 
+static int
+eth_dev_parse_hide_zero(const char *key, const char *value, void *extra_args)
+{
+	RTE_SET_USED(key);
+
+	if (value == NULL)
+		return -1;
+
+	if (strcmp(value, "true") == 0)
+		*(bool *)extra_args = true;
+	else if (strcmp(value, "false") == 0)
+		*(bool *)extra_args = false;
+	else
+		return -1;
+
+	return 0;
+}
+
 static int
 eth_dev_handle_port_xstats(const char *cmd __rte_unused,
 		const char *params,
 		struct rte_tel_data *d)
 {
+	const char *const valid_keys[] = { "hide_zero", NULL };
 	struct rte_eth_xstat *eth_xstats;
 	struct rte_eth_xstat_name *xstat_names;
+	struct rte_kvargs *kvlist;
 	int port_id, num_xstats;
-	int i, ret;
+	bool hide_zero = false;
 	char *end_param;
+	int i, ret;
 
 	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') {
+		kvlist = rte_kvargs_parse(end_param, valid_keys);
+		ret = rte_kvargs_process(kvlist, NULL, eth_dev_parse_hide_zero, &hide_zero);
+		if (kvlist == NULL || ret != 0)
+			RTE_ETHDEV_LOG(NOTICE,
+				"Unknown extra parameters passed to ethdev telemetry command, ignoring\n");
+		rte_kvargs_free(kvlist);
+	}
+
 	num_xstats = rte_eth_xstats_get(port_id, NULL, 0);
 	if (num_xstats < 0)
 		return -1;
@@ -5908,9 +5936,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_uint(d, xstat_names[i].name,
 					   eth_xstats[i].value);
+	}
 	free(eth_xstats);
 	return 0;
 }
@@ -6328,7 +6359,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=true|false(Optional for indicates hide zero xstats)");
 #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 v2] ethdev: telemetry xstats support hide zero

[fengchengwen]

On 2023/2/14 2:18, Ferruh Yigit wrote:
> On 2/13/2023 2:34 AM, Chengwen Feng wrote:
>> The number of xstats may be large, after the hide zero option is added,
>> only non-zero values can be displayed.
>>
>> So display xstats with hide zero:
>> 	/ethdev/xstats,0,hide_zero=true
>> and without hide zero (same as the original):
>> 	/ethdev/xstats,0
>>
>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>>
>> ---
>> v2: using hide_zero=true optional parameter which address Ferruh's
>> comments.
>>
>> ---
>>  lib/ethdev/rte_ethdev.c | 38 ++++++++++++++++++++++++++++++++------
>>  1 file changed, 32 insertions(+), 6 deletions(-)
>>
>> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
>> index d25db35f7f..7e70292fc6 100644
>> --- a/lib/ethdev/rte_ethdev.c
>> +++ b/lib/ethdev/rte_ethdev.c
>> @@ -15,6 +15,7 @@
>>  #include <bus_driver.h>
>>  #include <rte_log.h>
>>  #include <rte_interrupts.h>
>> +#include <rte_kvargs.h>
>>  #include <rte_memcpy.h>
>>  #include <rte_common.h>
>>  #include <rte_mempool.h>
>> @@ -5863,6 +5864,18 @@ eth_dev_handle_port_stats(const char *cmd __rte_unused,
>>  	return 0;
>>  }
>>  
>> +#define TEL_XSTATS_HIDE_ZERO_KEY	"hide_zero"
>> +#define TEL_XSTATS_HIDE_ZERO_VAL	"true"
>> +
>> +static int
>> +eth_dev_parse_hide_zero(const char *key, const char *value, void *extra_args)
>> +{
>> +	RTE_SET_USED(key);
>> +	if (strcmp(value, TEL_XSTATS_HIDE_ZERO_VAL) == 0)
> 
> 
> kvargs allows to provide parameter without value, like
> '/ethdev/xstats,0,hide_zero'
> 
> In this case 'value' will be NULL and this cause a crash in dpdk
> application.
> Better to check that 'value' is not NULL before using it.

Done it v3.

> 
>> +		*(bool *)extra_args = true;
>> +	return 0;
>> +}
>> +
>>  static int
>>  eth_dev_handle_port_xstats(const char *cmd __rte_unused,
>>  		const char *params,
>> @@ -5870,20 +5883,30 @@ eth_dev_handle_port_xstats(const char *cmd __rte_unused,
>>  {
>>  	struct rte_eth_xstat *eth_xstats;
>>  	struct rte_eth_xstat_name *xstat_names;
>> +	struct rte_kvargs *kvlist;
>>  	int port_id, num_xstats;
>> -	int i, ret;
>> +	bool hide_zero = false;
>>  	char *end_param;
>> +	int i, ret;
>>  
>>  	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') {
>> +		kvlist = rte_kvargs_parse(end_param, NULL);
>> +		if (kvlist != NULL)
>> +			rte_kvargs_process(kvlist, TEL_XSTATS_HIDE_ZERO_KEY,
>> +					   eth_dev_parse_hide_zero, &hide_zero);
>> +		if (kvlist == NULL || !hide_zero || kvlist->count > 1)
> 
> Instead of "kvlist->count > 1" can be better to use 'valid_keys'
> argument of 'rte_kvargs_parse()' to detect extra parameters, this way
> multiple 'hide_zero' parameters or future expansion of args won't be
> blocked.
> 
> 
> Similarly, '!hide_zero' check prevents "hide_zero=false" usage, it is
> not documented but this is expected to be supported intuitively, perhaps
> can be better to support "hide_zero=false" fully, document it and remove
> '!hide_zero' from check?

Both done in v3.

Thanks.

> 
>> +			RTE_ETHDEV_LOG(NOTICE,
>> +				"Unknown extra parameters passed to ethdev telemetry command, ignoring\n");
>> +		rte_kvargs_free(kvlist);
>> +	}
>> +
>>  	num_xstats = rte_eth_xstats_get(port_id, NULL, 0);
>>  	if (num_xstats < 0)
>>  		return -1;
>> @@ -5908,9 +5931,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_uint(d, xstat_names[i].name,
>>  					   eth_xstats[i].value);
>> +	}
>>  	free(eth_xstats);
>>  	return 0;
>>  }
>> @@ -6328,7 +6354,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=true(Optional for indicates hide zero xstats)");
>>  #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 v3] ethdev: telemetry xstats support hide zero

[Ferruh Yigit]

On 2/14/2023 1:35 AM, Chengwen Feng wrote:
> The number of xstats may be large, after the hide zero option is added,
> only non-zero values can be displayed.
> 
> So display xstats with hide zero:
> 	/ethdev/xstats,0,hide_zero=true
> and without hide zero:
> 	/ethdev/xstats,0,hide_zero=false  or
> 	/ethdev/xstats,0
> 
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>


Reviewed-by: Ferruh Yigit <ferruh.yigit@amd.com>


Applied to dpdk-next-net/main, thanks.