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 <fengcheng...@huawei.com>
>>> This patch looks good, so
>>> Reviewed-by: Dongdong Liu <liudongdo...@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");
>>>>
>>> .
>
> .
>
