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 22C3E41C8E; Tue, 14 Feb 2023 02:58:38 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id C49D640ED9; Tue, 14 Feb 2023 02:58:37 +0100 (CET) Received: from szxga01-in.huawei.com (szxga01-in.huawei.com [45.249.212.187]) by mails.dpdk.org (Postfix) with ESMTP id DDBE840E2D for ; Tue, 14 Feb 2023 02:58:35 +0100 (CET) Received: from dggpeml500024.china.huawei.com (unknown [172.30.72.53]) by szxga01-in.huawei.com (SkyGuard) with ESMTP id 4PG49Z6ctkzrRsw; Tue, 14 Feb 2023 09:58:10 +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; Tue, 14 Feb 2023 09:57:01 +0800 Subject: Re: [PATCH v2] ethdev: telemetry xstats support hide zero To: Ferruh Yigit , , CC: , , References: <20230209030755.48028-1-fengchengwen@huawei.com> <20230213023456.45972-1-fengchengwen@huawei.com> <044c4354-15c0-17fb-0628-019a148d1804@amd.com> From: fengchengwen Message-ID: <03145738-7e5d-88f4-daed-8036916cb55e@huawei.com> Date: Tue, 14 Feb 2023 09:57:01 +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: <044c4354-15c0-17fb-0628-019a148d1804@amd.com> Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 7bit X-Originating-IP: [10.67.100.224] X-ClientProxiedBy: dggems706-chm.china.huawei.com (10.3.19.183) 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/14 2:18, Ferruh Yigit wrote: > On 2/13/2023 2:34 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. >> >> So display xstats with hide zero: >> /ethdev/xstats,0,hide_zero=true >> and without hide zero (same as the original): >> /ethdev/xstats,0 >> >> Signed-off-by: Chengwen Feng >> >> --- >> v2: using hide_zero=true optional parameter which address Ferruh's >> comments. >> >> --- >> lib/ethdev/rte_ethdev.c | 38 ++++++++++++++++++++++++++++++++------ >> 1 file changed, 32 insertions(+), 6 deletions(-) >> >> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c >> index d25db35f7f..7e70292fc6 100644 >> --- a/lib/ethdev/rte_ethdev.c >> +++ b/lib/ethdev/rte_ethdev.c >> @@ -15,6 +15,7 @@ >> #include >> #include >> #include >> +#include >> #include >> #include >> #include >> @@ -5863,6 +5864,18 @@ eth_dev_handle_port_stats(const char *cmd __rte_unused, >> return 0; >> } >> >> +#define TEL_XSTATS_HIDE_ZERO_KEY "hide_zero" >> +#define TEL_XSTATS_HIDE_ZERO_VAL "true" >> + >> +static int >> +eth_dev_parse_hide_zero(const char *key, const char *value, void *extra_args) >> +{ >> + RTE_SET_USED(key); >> + if (strcmp(value, TEL_XSTATS_HIDE_ZERO_VAL) == 0) > > > kvargs allows to provide parameter without value, like > '/ethdev/xstats,0,hide_zero' > > In this case 'value' will be NULL and this cause a crash in dpdk > application. > Better to check that 'value' is not NULL before using it. Done it v3. > >> + *(bool *)extra_args = true; >> + return 0; >> +} >> + >> static int >> eth_dev_handle_port_xstats(const char *cmd __rte_unused, >> const char *params, >> @@ -5870,20 +5883,30 @@ eth_dev_handle_port_xstats(const char *cmd __rte_unused, >> { >> struct rte_eth_xstat *eth_xstats; >> struct rte_eth_xstat_name *xstat_names; >> + struct rte_kvargs *kvlist; >> int port_id, num_xstats; >> - int i, ret; >> + bool hide_zero = false; >> char *end_param; >> + int i, ret; >> >> 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') { >> + kvlist = rte_kvargs_parse(end_param, NULL); >> + if (kvlist != NULL) >> + rte_kvargs_process(kvlist, TEL_XSTATS_HIDE_ZERO_KEY, >> + eth_dev_parse_hide_zero, &hide_zero); >> + if (kvlist == NULL || !hide_zero || kvlist->count > 1) > > Instead of "kvlist->count > 1" can be better to use 'valid_keys' > argument of 'rte_kvargs_parse()' to detect extra parameters, this way > multiple 'hide_zero' parameters or future expansion of args won't be > blocked. > > > Similarly, '!hide_zero' check prevents "hide_zero=false" usage, it is > not documented but this is expected to be supported intuitively, perhaps > can be better to support "hide_zero=false" fully, document it and remove > '!hide_zero' from check? Both done in v3. Thanks. > >> + RTE_ETHDEV_LOG(NOTICE, >> + "Unknown extra parameters passed to ethdev telemetry command, ignoring\n"); >> + rte_kvargs_free(kvlist); >> + } >> + >> num_xstats = rte_eth_xstats_get(port_id, NULL, 0); >> if (num_xstats < 0) >> return -1; >> @@ -5908,9 +5931,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 && 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 +6354,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,hide_zero=true(Optional for 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"); > > . >