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