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

[Ferruh Yigit <ferruh.yigit@amd.com>]

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