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 0562242456; Sun, 22 Jan 2023 18:50:07 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 02156400EF; Sun, 22 Jan 2023 18:50:06 +0100 (CET) Received: from forward501b.mail.yandex.net (forward501b.mail.yandex.net [178.154.239.145]) by mails.dpdk.org (Postfix) with ESMTP id A5371400D4 for ; Sun, 22 Jan 2023 18:50:04 +0100 (CET) Received: from iva4-143b1447cf50.qloud-c.yandex.net (iva4-143b1447cf50.qloud-c.yandex.net [IPv6:2a02:6b8:c0c:7511:0:640:143b:1447]) by forward501b.mail.yandex.net (Yandex) with ESMTP id C8DDA5EA94; Sun, 22 Jan 2023 20:50:03 +0300 (MSK) Received: by iva4-143b1447cf50.qloud-c.yandex.net (smtp/Yandex) with ESMTPSA id 0otLcBvfmiE1-a4kgg3Zh; Sun, 22 Jan 2023 20:50:03 +0300 X-Yandex-Fwd: 1 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=yandex.ru; s=mail; t=1674409803; bh=TFFc8b/MAY5pgR3Dvi31UFpHidqhP+rfvoIoFBcaRyc=; h=From:In-Reply-To:Cc:Date:References:To:Subject:Message-ID; b=hjWZ4ErOQu+CO54lN2ERiis1tVsvRVy7eJkjk3dPTerUvMMwZs1cwAo6hQH6an3Dn nvrn897GYDnkG8TdUb+zlRn8ZZ8g3zg8j+qf9rLyJGycsTzJmoRzIFevKrJoRfy1he +qI+WGs9UhEnPYo3BdeAZBbH2wdg74mZ3HF/Ora8= Authentication-Results: iva4-143b1447cf50.qloud-c.yandex.net; dkim=pass header.i=@yandex.ru Message-ID: <222fb4ba-3fe8-19ba-a673-c5ba6e85f8ce@yandex.ru> Date: Sun, 22 Jan 2023 17:49:59 +0000 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.4.2 Subject: Re: [PATCH v2 2/2] ring: add ring info telemetry cmd Content-Language: en-US To: Jie Hai , honnappa.nagarahalli@arm.com, dev@dpdk.org Cc: liudongdong3@huawei.com, ciara.power@intel.com References: <20230117091049.20194-1-haijie1@huawei.com> <20230117130333.8707-1-haijie1@huawei.com> <20230117130333.8707-3-haijie1@huawei.com> From: Konstantin Ananyev In-Reply-To: <20230117130333.8707-3-haijie1@huawei.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit 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 > This patch supports dump of the info of ring by its name. > An example using this command is shown below: > > --> /ring/info,MP_mb_pool_0 > { > "/ring/info": { > "name": "MP_mb_pool_0", > "socket": 0, > "flags": 0, > "producer_type": "MP", > "consumer_type": "MC", > "size": 262144, > "mask": 262143, > "capacity": 262143, > "used_count": 147173, > "consumer_tail": 8283, > "consumer_head": 8283, > "producer_tail": 155456, > "producer_head": 155456, > "mz_name": "RG_MP_mb_pool_0", > "mz_len": 2097920, > "mz_hugepage_sz": 1073741824, > "mz_socket_id": 0, > "mz_flags": 0 > } > } > > Signed-off-by: Jie Hai > --- > lib/ring/rte_ring.c | 88 +++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 88 insertions(+) > > diff --git a/lib/ring/rte_ring.c b/lib/ring/rte_ring.c > index bb1dafd4d1ca..82f3d6a6cd60 100644 > --- a/lib/ring/rte_ring.c > +++ b/lib/ring/rte_ring.c > @@ -45,6 +45,9 @@ EAL_REGISTER_TAILQ(rte_ring_tailq) > /* by default set head/tail distance as 1/8 of ring capacity */ > #define HTD_MAX_DEF 8 > > +/* size of name of producer/consumer synchronization modes */ > +#define SYNC_MODE_NAME_SZ 16 > + > /* return the size of memory occupied by a ring */ > ssize_t > rte_ring_get_memsize_elem(unsigned int esize, unsigned int count) > @@ -454,8 +457,93 @@ ring_handle_list(const char *cmd __rte_unused, > return 0; > } > > +static void > +ring_get_sync_name_by_type(struct rte_ring *r, char *prod, char *cons) > +{ > + switch (r->prod.sync_type) { > + case RTE_RING_SYNC_MT: > + strcpy(prod, "MP"); > + break; > + case RTE_RING_SYNC_ST: > + strcpy(prod, "SP"); > + break; > + case RTE_RING_SYNC_MT_RTS: > + strcpy(prod, "MP_RTS"); > + break; > + case RTE_RING_SYNC_MT_HTS: > + strcpy(prod, "MP_HTS"); > + break; > + default: > + strcpy(prod, "Unknown"); > + } It is probably not the best option to blindly copy strings somewhere. I think it would be better to introduce function like that: static const char * ring_prod_sync_type_to_name(enum rte_ring_sync_type type) { switch(type) { case RTE_RING_SYNC_MT: return "MP"; case RTE_RING_SYNC_ST: return "SP"; ... } return "Unknown"; } Same for _cons_ type and use them accordingly. > + > + switch (r->cons.sync_type) { > + case RTE_RING_SYNC_MT: > + strcpy(cons, "MC"); > + break; > + case RTE_RING_SYNC_ST: > + strcpy(cons, "SC"); > + break; > + case RTE_RING_SYNC_MT_RTS: > + strcpy(cons, "MC_RTS"); > + break; > + case RTE_RING_SYNC_MT_HTS: > + strcpy(cons, "MC_HTS"); > + break; > + default: > + strcpy(cons, "Unknown"); > + } > +} > + > +static int > +ring_handle_info(const char *cmd __rte_unused, const char *params, > + struct rte_tel_data *d) > +{ > + char prod_type[SYNC_MODE_NAME_SZ]; > + char cons_type[SYNC_MODE_NAME_SZ]; > + const struct rte_memzone *mz; > + char name[RTE_RING_NAMESIZE]; > + struct rte_ring *r; > + > + if (params == NULL || strlen(params) == 0 || > + strlen(params) >= RTE_RING_NAMESIZE) > + return -EINVAL; > + > + strlcpy(name, params, RTE_RING_NAMESIZE); That copy looks absolutely redundant, you can do just rte_ring_lookup(params) instead. > + r = rte_ring_lookup(name); > + if (r == NULL) > + return -EINVAL; > + > + rte_tel_data_start_dict(d); > + rte_tel_data_add_dict_string(d, "name", r->name); Do I get it right that it could be executed from specific telemetry thread? If so, we probably shouldn't release rte_mcfg_tailq_read_lock while accessing ring data. > + rte_tel_data_add_dict_int(d, "socket", r->memzone->socket_id); You do print it below, when printing memzone related data. > + rte_tel_data_add_dict_int(d, "flags", r->flags); > + ring_get_sync_name_by_type(r, prod_type, cons_type); > + rte_tel_data_add_dict_string(d, "producer_type", prod_type); > + rte_tel_data_add_dict_string(d, "consumer_type", cons_type); > + rte_tel_data_add_dict_u64(d, "size", r->size); > + rte_tel_data_add_dict_u64(d, "mask", r->mask); > + rte_tel_data_add_dict_u64(d, "capacity", r->capacity); > + rte_tel_data_add_dict_u64(d, "used_count", rte_ring_count(r)); > + rte_tel_data_add_dict_u64(d, "consumer_tail", r->cons.tail); > + rte_tel_data_add_dict_u64(d, "consumer_head", r->cons.head); > + rte_tel_data_add_dict_u64(d, "producer_tail", r->prod.tail); > + rte_tel_data_add_dict_u64(d, "producer_head", r->prod.head); > + > + mz = r->memzone;`` Would it make sense to check that mz != NULL here? I know that it shouldn't be NULL for valid ring created by rte_ring_create(), but still probably no harm. > + rte_tel_data_add_dict_string(d, "mz_name", mz->name); > + rte_tel_data_add_dict_int(d, "mz_len", mz->len); > + rte_tel_data_add_dict_int(d, "mz_hugepage_sz", mz->hugepage_sz); > + rte_tel_data_add_dict_int(d, "mz_socket_id", mz->socket_id); > + rte_tel_data_add_dict_int(d, "mz_flags", mz->flags); > + > + return 0; > +} > + > RTE_INIT(ring_init_telemetry) > { > rte_telemetry_register_cmd("/ring/list", ring_handle_list, > "Returns list of available ring. Takes no parameters"); > + rte_telemetry_register_cmd("/ring/info", ring_handle_info, > + "Returns ring info. Parameters: ring_name."); > }