ethdev: telemetry xstats support hide zero

  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(-)
  

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");
  

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");
> 
> .
>
  

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");
>>
>> .
>>
  

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>
>>>> ---

...