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 DC60843BDF; Fri, 8 Mar 2024 09:48:20 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id ADABD402E2; Fri, 8 Mar 2024 09:48:20 +0100 (CET) Received: from szxga06-in.huawei.com (szxga06-in.huawei.com [45.249.212.32]) by mails.dpdk.org (Postfix) with ESMTP id 03A2340274 for ; Fri, 8 Mar 2024 09:48:18 +0100 (CET) Received: from mail.maildlp.com (unknown [172.19.162.112]) by szxga06-in.huawei.com (SkyGuard) with ESMTP id 4Trftv5ZJJz1vwFF; Fri, 8 Mar 2024 16:47:35 +0800 (CST) Received: from kwepemm600004.china.huawei.com (unknown [7.193.23.242]) by mail.maildlp.com (Postfix) with ESMTPS id CA72D1404F1; Fri, 8 Mar 2024 16:48:16 +0800 (CST) Received: from [10.67.121.59] (10.67.121.59) by kwepemm600004.china.huawei.com (7.193.23.242) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.1.2507.35; Fri, 8 Mar 2024 16:48:16 +0800 Message-ID: Date: Fri, 8 Mar 2024 16:48:15 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.2.0 Subject: Re: [PATCH v5 2/7] ethdev: add telemetry cmd for registers To: Jie Hai CC: , , Thomas Monjalon , Andrew Rybchenko , Ferruh Yigit References: <20231214015650.3738578-1-haijie1@huawei.com> <20240307030247.599394-1-haijie1@huawei.com> <20240307030247.599394-3-haijie1@huawei.com> From: "lihuisong (C)" In-Reply-To: <20240307030247.599394-3-haijie1@huawei.com> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.67.121.59] X-ClientProxiedBy: dggems706-chm.china.huawei.com (10.3.19.183) To kwepemm600004.china.huawei.com (7.193.23.242) 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 在 2024/3/7 11:02, Jie Hai 写道: > This patch adds a telemetry command for registers dump, > and supports get registers with specified names. > The length of the string exported by telemetry is limited > by MAX_OUTPUT_LEN. Therefore, the filter should be more > precise. > > An example usage is shown below: > --> /ethdev/regs,0,INTR > { > "/ethdev/regs": { > "registers_length": 318, > "registers_width": 4, > "register_offset": "0x0", > "version": "0x1140011", > "group_0": { > "HNS3_CMDQ_INTR_STS_REG": "0x0", > "HNS3_CMDQ_INTR_EN_REG": "0x2", > "HNS3_CMDQ_INTR_GEN_REG": "0x0", > "queue_0_HNS3_TQP_INTR_CTRL_REG": "0x0", > "queue_0_HNS3_TQP_INTR_GL0_REG": "0xa", > "queue_0_HNS3_TQP_INTR_GL1_REG": "0xa", > "queue_0_HNS3_TQP_INTR_GL2_REG": "0x0", > ... > }, > "group_1": { > ... > }, > ... > } > > or as below if the number of registers not exceed the > RTE_TEL_MAX_DICT_ENTRIES: > --> /ethdev/regs,0,ppp > { > "/ethdev/regs": { > "registers_length": 156, > "registers_width": 4, > "register_offset": "0x0", > "version": "0x1140011", > "ppp_key_drop_num": "0x0", > "ppp_rlt_drop_num": "0x0", > "ssu_ppp_mac_key_num_l": "0x1", > "ssu_ppp_mac_key_num_h": "0x0", > "ssu_ppp_host_key_num_l": "0x1", > "ssu_ppp_host_key_num_h": "0x0", > "ppp_ssu_mac_rlt_num_l": "0x1", > ... > } > } > > Signed-off-by: Jie Hai > --- > lib/ethdev/rte_ethdev_telemetry.c | 158 ++++++++++++++++++++++++++++++ > 1 file changed, 158 insertions(+) > > diff --git a/lib/ethdev/rte_ethdev_telemetry.c b/lib/ethdev/rte_ethdev_telemetry.c > index 6b873e7abe68..0c570792fb5f 100644 > --- a/lib/ethdev/rte_ethdev_telemetry.c > +++ b/lib/ethdev/rte_ethdev_telemetry.c > @@ -5,6 +5,7 @@ > #include > #include > > +#include > #include > #include > > @@ -1395,6 +1396,161 @@ eth_dev_handle_port_tm_node_caps(const char *cmd __rte_unused, > return ret; > } > > +static void > +eth_dev_add_u32_hex(struct rte_tel_data *d, const char *name, uint32_t **data) > +{ > + if (*data == NULL) > + return; > + rte_tel_data_add_dict_uint_hex(d, name, **data, 0); > + (*data)++; suggest that move "(*data)++" to the place after called this interface. > +} > + > +static void > +eth_dev_add_u64_hex(struct rte_tel_data *d, const char *name, uint64_t **data) > +{ > + if (*data == NULL) > + return; > + rte_tel_data_add_dict_uint_hex(d, name, **data, 0); > + (*data)++; the same as above. > +} > + > +static int > +eth_dev_store_regs(struct rte_tel_data *d, struct rte_dev_reg_info *reg_info) > +{ > + struct rte_tel_data *groups[RTE_TEL_MAX_DICT_ENTRIES]; > + char group_name[RTE_TEL_MAX_STRING_LEN] = {0}; > + struct rte_tel_data *group = NULL; > + uint32_t *data0 = NULL; > + uint64_t *data1 = NULL; > + uint32_t grp_num = 0; > + int ret = 0; > + uint32_t i; > + > + rte_tel_data_start_dict(d); > + rte_tel_data_add_dict_uint(d, "register_length", reg_info->length); > + rte_tel_data_add_dict_uint(d, "register_width", reg_info->width); > + rte_tel_data_add_dict_uint_hex(d, "register_offset", reg_info->offset, 0); > + rte_tel_data_add_dict_uint_hex(d, "version", reg_info->version, 0); > + > + if (reg_info->width == sizeof(uint32_t)) > + data0 = reg_info->data; > + else > + data1 = reg_info->data; > + > + if (reg_info->length <= RTE_TEL_MAX_DICT_ENTRIES) { > + for (i = 0; i < reg_info->length; i++) { > + eth_dev_add_u32_hex(d, reg_info->names[i].name, &data0); > + eth_dev_add_u64_hex(d, reg_info->names[i].name, &data1); Here adding u32_hex or u64_hex should depend on the reg_info->width, right? Even though it is ok now due to the validity check in eth_dev_add_u32_hex and eth_dev_add_u64_hex. Using the validity check to control the code flow is ok. But u32 and u64 interface are parallel. It's obvious that one of them will be selected for execution here. So suggest that modify it as following logic: " if (reg_info->width == sizeof(uint32_t)) eth_dev_add_u32_hex(d, reg_info->names[i].name, &data0) else eth_dev_add_u64_hex(d, reg_info->names[i].name, &data1); " > + } > + return 0; > + } > + > + for (i = 0; i < reg_info->length; i++) { > + if (i % RTE_TEL_MAX_DICT_ENTRIES == 0) { > + group = rte_tel_data_alloc(); > + if (group == NULL) { > + ret = -ENOMEM; > + goto out; > + } > + groups[grp_num++] = group; > + rte_tel_data_start_dict(group); > + } > + eth_dev_add_u32_hex(group, reg_info->names[i].name, &data0); > + eth_dev_add_u64_hex(group, reg_info->names[i].name, &data1); > + } > + > + for (i = 0; i < grp_num; i++) { > + snprintf(group_name, RTE_TEL_MAX_STRING_LEN, > + "group_%u", i); > + rte_tel_data_add_dict_container(d, group_name, groups[i], 0); > + } > + return 0; > +out: > + for (i = 0; i < grp_num; i++) > + rte_tel_data_free(groups[i]); > + > + return ret; > +} > + > +static int > +eth_dev_get_port_regs(int port_id, struct rte_tel_data *d, char *filter) > +{ > + struct rte_dev_reg_info reg_info; > + uint32_t max_length; > + int ret; > + > + memset(®_info, 0, sizeof(reg_info)); > + reg_info.filter = filter; > + > + ret = rte_eth_dev_get_reg_info_ext(port_id, ®_info); > + if (ret != 0) { > + RTE_ETHDEV_LOG_LINE(ERR, > + "Error getting device reg info: %d", ret); > + return ret; > + } > + > + /* 4 is space for other information of registers. */ > + max_length = RTE_TEL_MAX_DICT_ENTRIES * > + (RTE_TEL_MAX_DICT_ENTRIES - 4); please define a macro for this "4" and add more detail description. > + if (reg_info.length > max_length) { > + RTE_ETHDEV_LOG_LINE(WARNING, > + "Reduced register number to be obtained from %u to %u due to limited memory", > + reg_info.length, max_length); > + reg_info.length = max_length; > + } > + > + reg_info.data = calloc(reg_info.length, reg_info.width); > + if (reg_info.data == NULL) { > + RTE_ETHDEV_LOG_LINE(ERR, > + "Fail to allocate memory for reg_info.data"); > + return -ENOMEM; > + } > + > + reg_info.names = calloc(reg_info.length, sizeof(struct rte_eth_reg_name)); > + if (reg_info.names == NULL) { > + RTE_ETHDEV_LOG_LINE(ERR, > + "Fail to allocate memory for reg_info.names"); > + free(reg_info.data); > + return -ENOMEM; > + } > + > + ret = rte_eth_dev_get_reg_info_ext(port_id, ®_info); > + if (ret != 0) { > + RTE_ETHDEV_LOG_LINE(ERR, > + "Error getting regs from device: %d", ret); > + ret = -EINVAL; > + goto out; > + } > + > + ret = eth_dev_store_regs(d, ®_info); > +out: > + free(reg_info.data); > + free(reg_info.names); > + > + return ret; > +} > + > +static int > +eth_dev_handle_port_regs(const char *cmd __rte_unused, > + const char *params, > + struct rte_tel_data *d) > +{ > + char *filter = NULL; > + uint16_t port_id; > + char *end_param; > + int ret; > + > + ret = eth_dev_parse_port_params(params, &port_id, &end_param, true); > + if (ret != 0) > + return ret; > + > + filter = strtok(end_param, ","); > + if (filter != NULL && strlen(filter) == 0) > + filter = NULL; > + > + return eth_dev_get_port_regs(port_id, d, filter); > +} > + > RTE_INIT(ethdev_init_telemetry) > { > rte_telemetry_register_cmd("/ethdev/list", eth_dev_handle_port_list, > @@ -1436,4 +1592,6 @@ RTE_INIT(ethdev_init_telemetry) > "Returns TM Level Capabilities info for a port. Parameters: int port_id, int level_id (see tm_capability for the max)"); > rte_telemetry_register_cmd("/ethdev/tm_node_capability", eth_dev_handle_port_tm_node_caps, > "Returns TM Node Capabilities info for a port. Parameters: int port_id, int node_id (see tm_capability for the max)"); > + rte_telemetry_register_cmd("/ethdev/regs", eth_dev_handle_port_regs, > + "Returns regs for a port. Parameters: int port_id, string filter"); "Returns all regs or specified type regs for a port. Parameters: int port_id, string filter"? > }