From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 2B67641CB1; Thu, 16 Feb 2023 13:42:41 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 0980540EE3; Thu, 16 Feb 2023 13:42:41 +0100 (CET) Received: from szxga02-in.huawei.com (szxga02-in.huawei.com [45.249.212.188]) by mails.dpdk.org (Postfix) with ESMTP id 2B94E40A8B for ; Thu, 16 Feb 2023 13:42:39 +0100 (CET) Received: from dggpeml500024.china.huawei.com (unknown [172.30.72.57]) by szxga02-in.huawei.com (SkyGuard) with ESMTP id 4PHZKC1bJczRs0V; Thu, 16 Feb 2023 20:39:59 +0800 (CST) Received: from [10.67.100.224] (10.67.100.224) by dggpeml500024.china.huawei.com (7.185.36.10) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.17; Thu, 16 Feb 2023 20:42:34 +0800 Subject: Re: [PATCH v5 2/2] ethdev: support xstats reset telemetry command To: Ferruh Yigit , Dongdong Liu , , CC: , , , References: <20221219090723.29356-1-fengchengwen@huawei.com> <20230209023203.35269-1-fengchengwen@huawei.com> <20230209023203.35269-3-fengchengwen@huawei.com> <6921c549-c980-8911-cebf-2c7ab7872f02@amd.com> From: fengchengwen Message-ID: <92b271ec-3271-1c80-e148-4a53f97bc074@huawei.com> Date: Thu, 16 Feb 2023 20:42:34 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.11.0 MIME-Version: 1.0 In-Reply-To: <6921c549-c980-8911-cebf-2c7ab7872f02@amd.com> Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 8bit X-Originating-IP: [10.67.100.224] X-ClientProxiedBy: dggems703-chm.china.huawei.com (10.3.19.180) To dggpeml500024.china.huawei.com (7.185.36.10) X-CFilter-Loop: Reflected X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org 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 >>> This patch looks good, so >>> Reviewed-by: Dongdong Liu >>> >>> 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"); >>>> >>> . > > . >