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