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

[fengchengwen <fengchengwen@huawei.com>]

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