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