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 A5C6BA00C2; Mon, 23 May 2022 15:43:48 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 8D6694014F; Mon, 23 May 2022 15:43:48 +0200 (CEST) Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by mails.dpdk.org (Postfix) with ESMTP id 9BD6840141 for ; Mon, 23 May 2022 15:43:46 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1653313426; x=1684849426; h=date:from:to:cc:subject:message-id:references: mime-version:in-reply-to; bh=ItxMhVXH5jnZishgX5esusen8zMsoqn6KlLEkmbpL8A=; b=bwLVFcC/4lX0cjzZX4X9KKjyUOfJUMURS0D3/AvO0DtZJ3eYajMY1mj3 NfTYmgOO3cJzS9fEJuLPlXK4rJsAUWghnMhlQdXO+qjPyZVyy4TdcK2/p DDqIk5/BIZroEqVird9jwA5o9hsKYKQEMju7V51wOQRbgTrdEe94VsG77 PrYoh5iPqzr5yIx0rtOaJMlaH3aMD/kWNHu+BEkTxOt49/YPV7CAdJMQU gGVWpxlg53uwrPDUNVUxWxfpzUsakgo7GKSjyBV8fFvlSGr1jzvuDzIRf eH7Zr2UDHl+QeH5C+S9vTGywRyW0ngW6xYKeJvJyWEvFUBIgrBIbAZwrJ w==; X-IronPort-AV: E=McAfee;i="6400,9594,10355"; a="253099157" X-IronPort-AV: E=Sophos;i="5.91,246,1647327600"; d="scan'208";a="253099157" Received: from orsmga007.jf.intel.com ([10.7.209.58]) by fmsmga106.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 23 May 2022 06:43:45 -0700 X-IronPort-AV: E=Sophos;i="5.91,246,1647327600"; d="scan'208";a="572105470" Received: from bricha3-mobl.ger.corp.intel.com ([10.55.133.25]) by orsmga007-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-SHA; 23 May 2022 06:43:43 -0700 Date: Mon, 23 May 2022 14:43:40 +0100 From: Bruce Richardson To: Amit Prakash Shukla Cc: Anatoly Burakov , Ciara Power , "dev@dpdk.org" , Jerin Jacob Kollanukkaran Subject: Re: [EXT] Re: [PATCH v2] mem: telemetry support for memseg and element information Message-ID: References: <20220519063038.637836-1-amitprakashs@marvell.com> <20220519185712.879487-1-amitprakashs@marvell.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: 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 Mon, May 23, 2022 at 01:35:06PM +0000, Amit Prakash Shukla wrote: > Thanks Bruce for the review suggestions. I make the changes as suggested in next version of the patch. > > > -----Original Message----- > > From: Bruce Richardson > > Sent: Monday, May 23, 2022 4:45 PM > > To: Amit Prakash Shukla > > Cc: Anatoly Burakov ; Ciara Power > > ; dev@dpdk.org; Jerin Jacob Kollanukkaran > > > > Subject: [EXT] Re: [PATCH v2] mem: telemetry support for memseg and > > element information > > > > External Email > > > > ---------------------------------------------------------------------- > > On Fri, May 20, 2022 at 12:27:12AM +0530, Amit Prakash Shukla wrote: > > > 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/active_memseg_list > > > The command displays the memseg list from which the memory has been > > > allocated. > > > Example: > > > --> /eal/active_memseg_list > > > {"/eal/active_memseg_list": [0, 1]} > > > > > > 2. /eal/memseg_list, > > > The command outputs the memsegs, from which the memory is allocated, > > > for the memseg_list given as input. Command also supports help. > > > Example: > > > --> /eal/memseg_list,help > > > {"/eal/memseg_list": "/eal/memseg_list,"} > > > > > > --> /eal/memseg_list,1 > > > {"/eal/memseg_list": [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, \ 12, 13, > > > 14, 15]} > > > > > > > This is really confusing because, if I understand this correctly, we have a > > conflict of terms here - in telemetry "list" is generally used to get the possible > > values of ids at the top level, with the info and other commands used to get > > the next level of detail down, while the initial command here returns details > > on the memseg lists, i.e. it should really be "memseg_list_list" command, i.e. > > list the memseg lists. Can we perhaps come up with a different term for the > > memseg list, because right now I think the above commands should be > > "memseg_list_list" and "memseg_list_info"? > > > > Sure, will change the naming. > Have a think about it too, because my suggested naming is still rather unwieldy and not very nice. I'm not sure what the best naming here is. Are the memsegs only identified by number inside each memseg list? Are there no names that could be used instead? Could you merge the memseg_list_list and memseg_list_info into one to print out a list of all memsegs in one go across multiple lists? How many memsegs are there likely to be? > > > > > 3. /eal/memseg_info,: > > > The command outputs the memseg information based on the memseg-list > > > and the memseg-id given as input. Command also supports help. > > > Example: > > > --> /eal/memseg_info,help > > > {"/eal/memseg_info": "/eal/memseg_info,: \ > > > "} > > > > > > --> /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}} > > > > > > --> /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}} > > > > > > > For telemetry library, the parameters should all be comma-separated rather > > than colon-separated. > > > > Sure, will change it to comma-separated. > > > > 4. /eal/elem_list,:: > > > The command outputs number of elements in a memseg based on the > > > heap-id, memseg-list-id and memseg-id given as input. > > > Command also supports help. > > > Example: > > > --> /eal/elem_list,help > > > {"/eal/elem_list": "/eal/elem_list,: \ > > > :"} > > > > > > --> /eal/elem_list,0:0:63 > > > {"/eal/elem_list": {"Element_count": 52}} > > > > > > --> /eal/elem_list,0:1:15 > > > {"/eal/elem_list": {"Element_count": 52}} > > > > > > 5. /eal/elem_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. Command also supports > > > help. > > > Example: > > > --> /eal/elem_info,help > > > {"/eal/elem_info": "/eal/elem_info,: \ > > > :: -"} > > > > > The last 2 arguments "-" is to print range of elements. Can I use hyphen here ? > I'm not sure I like printing based off a range, especially since doing so can lead to very large outputs. I would still tend towards separating with a comma here, if you only support a single range at a time. I would only support using a "-" in the specifier, if you supported multiple range options in one go, e.g. as is done with DPDK -l EAL flag. > > > --> /eal/elem_info,0:1:15:1-2 > > > {"/eal/elem_info": {"elem_info.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"}, \ > > > "elem_info.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}} > > > > > > > The "elem" name is ambiguous, I think. Are these malloc elements or some > > other type of elements. > > > > These are malloc elements. I will change the naming. > > > > > > Increased telemetry output buffer to 64K to support large size > > > telemetry data output. > > > > > > > That's a 4x increase in max size. Is it really necessary? Is telemetry the best > > way to output this info, and do you see users really needing it? > > This change is not required now. The code has been internally optimized. I will revert this change. Thanks, /Bruce