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