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

[Bruce Richardson <bruce.richardson@intel.com>]

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