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 3AD79A055F; Thu, 20 Oct 2022 13:40:50 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 2341B42C61; Thu, 20 Oct 2022 13:40:50 +0200 (CEST) Received: from mail-lj1-f180.google.com (mail-lj1-f180.google.com [209.85.208.180]) by mails.dpdk.org (Postfix) with ESMTP id D880B42BF1 for ; Thu, 20 Oct 2022 13:40:48 +0200 (CEST) Received: by mail-lj1-f180.google.com with SMTP id m23so25952090lji.2 for ; Thu, 20 Oct 2022 04:40:48 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:subject:cc:to:from:date:from:to:cc:subject:date :message-id:reply-to; bh=kAFc1Fgf7zjtsMJaVIIgdH1EDw1QTWbWvtx80QFlPbY=; b=FFMZB15ASVyg9NsHt07beEOCsDoAm3MKO3e2cYubqlTD8CUp+xXI/w/yhcGmpBs8A7 ecTeN8QYmCtPPwQzlByQwsXxO/7cAOHqJogm51q7YNe0le+qLTrnqYqxdSra7JaaXE9h E+Kp+hgX6mVHKq0qb3HqoN8hM861UylHGn1CVc1s5ciQB07gkvKf0fZVdIxyMK3Ji2wv evdYYdfs2JiY0Cn+YZDGgu2nwTOPxuQrHEOQV0cd+O6qas38+Lg/pBlu6cX12SqxSD/W zu3boPwuVf++8uoFnabqRGWBZ0N4+k/c4NFwZMuj/aRU7TvZOuZhh11gkMIaIWldNAPC 1Viw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:subject:cc:to:from:date:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=kAFc1Fgf7zjtsMJaVIIgdH1EDw1QTWbWvtx80QFlPbY=; b=wOnyMatPuDfBi442BMbIntt4n9TJruEbLDTeHSFMVc0fTT+hPv9U+NjLfzCdaxjIT2 +3gJTQP53KVA0MRJZYGuWa0cLAiJzu+e3wQjRQHaQrehQbAb6OUOXJ7PDiEn9pE4FcS9 e4KPGe+/OrppiQPe2QYcbGmV1dXjR6f1UuMSgDjmMgxOp6CHTYmmBVTdGY8pk1Ggk0f9 rx0xusf8xyU0vPcvlwWK5reziSwE3PSgksmoi0GR4pkzWuZh1jv3Owi/rQ3Zu7ZTf3LO 1n+bHXA0ypwqczAraqW4rnxuiwOp8yrzP5m6nfjTokp4/2V08CBa8VTNo8OTjxeIY73C EI5A== X-Gm-Message-State: ACrzQf3DqFwxzcsRUJvHdrcJTbprcRnrjPN4yMEsUgQ/5u2ybFkvg9B7 Nc5XtwCdinOa0ihotI2drWA= X-Google-Smtp-Source: AMsMyM6euaA75Uvv9njJg4wKMQABEt+lxi8tdGlNN+oxhvnNiepxazghrH49S8ne9KBzPVKFYlflnA== X-Received: by 2002:a2e:8514:0:b0:274:7384:fc5 with SMTP id j20-20020a2e8514000000b0027473840fc5mr803996lji.52.1666266048358; Thu, 20 Oct 2022 04:40:48 -0700 (PDT) Received: from sovereign (broadband-37-110-65-23.ip.moscow.rt.ru. [37.110.65.23]) by smtp.gmail.com with ESMTPSA id d11-20020a05651233cb00b00497a61453a9sm2679953lfg.243.2022.10.20.04.40.47 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 20 Oct 2022 04:40:47 -0700 (PDT) Date: Thu, 20 Oct 2022 14:40:46 +0300 From: Dmitry Kozlyuk To: Amit Prakash Shukla Cc: Anatoly Burakov , , , , , Subject: Re: [PATCH v5 1/2] mem: telemetry support for memseg and element information Message-ID: <20221020144046.13b98d76@sovereign> In-Reply-To: <20220929114313.1346972-1-amitprakashs@marvell.com> References: <20220525103352.1806937-1-amitprakashs@marvell.com> <20220929114313.1346972-1-amitprakashs@marvell.com> X-Mailer: Claws Mail 3.18.0 (GTK+ 2.24.33; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII 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 2022-09-29 17:13 (UTC+0530), Amit Prakash Shukla: > Changes adds telemetry support to display memory occupancy > in memseg and the information of the elements allocated from > a memseg based on arguments provided by user. This patch > adds following endpoints: > > 1. /eal/memseg_list_array > The command displays the memseg list from which the memory > has been allocated. > Example: > --> /eal/memseg_list_array > {"/eal/memseg_list_array": [0, 1]} > > 2. /eal/memseg_list_info, > The command outputs the memsegs, from which the memory is > allocated, for the memseg_list given as input. > Example: > --> /eal/memseg_list_info,1 > {"/eal/memseg_list_info": [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, \ > 12, 13, 14, 15]} MSL has more properties worth reporting. Also note that by default #define RTE_MAX_MEMSEG_PER_LIST 8192 which means that the array will not fit into the output buffer (16KB). Large number of memsegs is quite possible with 2MB hugepages. I suggest to have a request for MSL properties, including length, and this request be a separate one. If this one fails due to insufficient buffer, the user will at least know the range of possible indices. > 3. /eal/memseg_info,, > The command outputs the memseg information based on the > memseg-list and the memseg-id given as input. > Example: > --> /eal/memseg_info,0,10 > {"/eal/memseg_info": {"Memseg_list_index": 0, \ > "Memseg_index": 10, "Memseg_list_len": 64, \ > "Start_addr": "0x260000000", "End_addr": "0x280000000", \ > "Size": 536870912}} "Memseg_list_len" is neither a property or an identifier of a memseg. Important memseg fields are missing, like socket, hugepage_sz, and flags. Note that "Size" displays hugepage_sz, but this is not correct: for external memory memseg is not necessarily a single page. Size and hugepage size fields must be distinct. > > --> /eal/memseg_info,1,15 > {"/eal/memseg_info": {"Memseg_list_index": 1, \ > "Memseg_index": 15, "Memseg_list_len": 64, \ > "Start_addr": "0xb20000000", "End_addr": "0xb40000000", \ > "Size": 536870912}} > > 4. /eal/element_list,,, > The command outputs number of elements in a memseg based > on the heap-id, memseg-list-id and memseg-id given as input. > Example: > --> /eal/element_list,0,0,63 > {"/eal/element_list": {"Element_count": 52}} How does the user learn heap_id? There probably should be /eal/heap_id returning a list of heap IDs. Please use a consistent naming scheme for requests returning ID lists. Currently MSL have "_array" suffix but memsegs and elements don't. > --> /eal/element_list,0,1,15 > {"/eal/element_list": {"Element_count": 52}} > > 5. /eal/element_info,,,, \ > , > The command outputs element information like element start > address, end address, to which memseg it belongs, element > state, element size. User can give a range of elements to be > printed. > Example: > --> /eal/element_info,0,1,15,1,2 > {"/eal/element_info": {"element.1": {"msl_id": 1, \ > "ms_id": 15, "memseg_start_addr": "0xb20000000", \ > "memseg_end_addr": "0xb40000000", \ > "element_start_addr": "0xb201fe680", \ > "element_end_addr": "0xb20bfe700", \ > "element_size": 10485888, "element_state": "Busy"}, \ > "element.2": {"msl_id": 1, "ms_id": 15, \ > "memseg_start_addr": "0xb20000000", \ > "memseg_end_addr": "0xb40000000", \ > "element_start_addr": "0xb20bfe700", \ > "element_end_addr": "0xb215fe780", "element_size": 10485888, \ > "element_state": "Busy"}, "Element_count": 2}} How this request is going to be used? Elements don't have permanent IDs like MSL/memseg index or heap ID. Heap layout may change between /eal/element_list and this request. Maybe instead there should be a filter by address with maybe a context parameter (like "grep -C")? The proposed API is not bad at all by itself, I'm asking to make sure it solves the task in the best way. [...] > +static int > +handle_eal_memseg_info_request(const char *cmd __rte_unused, > + const char *params, struct rte_tel_data *d) > +{ > + struct rte_mem_config *mcfg; > + uint64_t ms_start_addr, ms_end_addr, ms_size; > + struct rte_memseg_list *msl; > + const struct rte_memseg *ms; > + struct rte_fbarray *arr; > + char addr[ADDR_STR]; > + uint32_t ms_list_idx = 0; > + uint32_t ms_idx = 0; > + uint32_t msl_len; > + char dlim[2] = ","; > + char *token; > + char *params_args; > + > + if (params == NULL || strlen(params) == 0) > + return -1; > + > + /* strtok expects char * and param is const char *. Hence on using > + * params as "const char *" compiler throws warning. > + */ > + params_args = strdup(params); Please check the allocation result hear and in the rest of the handlers. It would be nice to have a local helper to parse N integer params, this would reduce and simplify the code: static int parse_params(const char *params, int *vals, size_t vals_n); [...] > RTE_INIT(memory_telemetry) > { > rte_telemetry_register_cmd( > @@ -1279,5 +1699,22 @@ RTE_INIT(memory_telemetry) > rte_telemetry_register_cmd( > EAL_HEAP_INFO_REQ, handle_eal_heap_info_request, > "Returns malloc heap stats. Parameters: int heap_id"); > + rte_telemetry_register_cmd( > + EAL_MEMSEG_LIST_ARR_REQ, > + handle_eal_memseg_list_array_request, > + "Returns hugepage list. Takes no parameters"); "hugepage list" -> "array of memseg list IDs" > + rte_telemetry_register_cmd( > + EAL_MEMSEG_LIST_INFO_REQ, > + handle_eal_memseg_list_info_request, > + "Returns memseg list. Parameters: int memseg_list_id"); "memseg list" -> "memseg list info" > + rte_telemetry_register_cmd( > + EAL_MEMSEG_INFO_REQ, handle_eal_memseg_info_request, > + "Returns memseg info. Parameter: int memseg_list_id,int memseg_id"); > + rte_telemetry_register_cmd(EAL_ELEMENT_LIST_REQ, > + handle_eal_element_list_request, > + "Returns element info. Parameters: int heap_id, int memseg_list_id, int memseg_id"); "element info" -> "array of heap element IDs". > + rte_telemetry_register_cmd(EAL_ELEMENT_INFO_REQ, > + handle_eal_element_info_request, > + "Returns element info. Parameters: int heap_id, memseg_list_id, memseg_id, start_elem_id, end_elem_id"); > } > #endif Please make parameter descriptions consistent ("int x, int y" vs "int x, y").