Re: [PATCH v5 2/2] ethdev: support xstats reset telemetry command

[fengchengwen <fengchengwen@huawei.com>]

On 2023/2/16 20:06, Ferruh Yigit wrote:
> On 2/16/2023 11:53 AM, fengchengwen wrote:
>> On 2023/2/15 11:19, Dongdong Liu wrote:
>>> Hi Chengwen
>>>
>>> On 2023/2/9 10:32, Chengwen Feng wrote:
>>>> The xstats reset is useful for debugging, so add it to the ethdev
>>>> telemetry command lists.
>>>>
>>>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>>> This patch looks good, so
>>> Reviewed-by: Dongdong Liu <liudongdong3@huawei.com>
>>>
>>> A minior question
>>> Do we need to support stats reset ?
>>
>> Stats is contained by xstats, and future direction I think is xstats.
>> So I think we don't need support stats reset.
>>
> 
> I have similar question with Dongdong, readonly values are safe for
> telemetry, but modifying data can be more tricky since we don't have
> locking in ethdev APIs, this can cause concurrency issues.

Yes, it indeed has concurrency issues.

> 
> Overall do we want telemetry go that way and become something that
> alters ethdev data/config?

There are at least two part of data: config and status.
For stats (which belong status data) could help for debugging, I think it's acceptable.

As for concurrency issues. People should know what to do and when to do, just like
the don't invoke config API (e.g. dev_configure/dev_start/...) concurrency.

> 
> 
>> Thanks.
>>
>>>
>>> Thanks,
>>> Dongdong
>>>> ---
>>>>  lib/ethdev/rte_ethdev.c | 31 +++++++++++++++++++++++++++++++
>>>>  1 file changed, 31 insertions(+)
>>>>
>>>> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
>>>> index d25db35f7f..e85c98f307 100644
>>>> --- a/lib/ethdev/rte_ethdev.c
>>>> +++ b/lib/ethdev/rte_ethdev.c
>>>> @@ -5915,6 +5915,35 @@ eth_dev_handle_port_xstats(const char *cmd __rte_unused,
>>>>      return 0;
>>>>  }
>>>>
>>>> +static int
>>>> +eth_dev_handle_port_xstats_reset(const char *cmd __rte_unused,
>>>> +        const char *params,
>>>> +        struct rte_tel_data *d)
>>>> +{
>>>> +    int port_id, 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;
>>>> +
>>>> +    ret = rte_eth_xstats_reset(port_id);
>>>> +    if (ret == 0) {
>>>> +        rte_tel_data_string(d, "success");
>>>> +        RTE_ETHDEV_LOG(NOTICE, "Port %d reset xstats success\n", port_id);
>>>> +    } else {
>>>> +        RTE_ETHDEV_LOG(ERR, "Port %d reset xstats failed! ret: %d\n", port_id, ret);
>>>> +    }
>>>> +
>>>> +    return ret;
>>>> +}
>>>> +
>>>>  #ifndef RTE_EXEC_ENV_WINDOWS
>>>>  static int
>>>>  eth_dev_handle_port_dump_priv(const char *cmd __rte_unused,
>>>> @@ -6329,6 +6358,8 @@ RTE_INIT(ethdev_init_telemetry)
>>>>              "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");
>>>> +    rte_telemetry_register_cmd("/ethdev/xstats_reset", eth_dev_handle_port_xstats_reset,
>>>> +            "Reset the extended stats for a port. Parameters: int port_id");
>>>>  #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");
>>>>
>>> .
> 
> .
>