RE: [EXT] Re: [PATCH v2] mem: telemetry support for memseg and element information

[Amit Prakash Shukla <amitprakashs@marvell.com>]

> -----Original Message-----
> From: Bruce Richardson <bruce.richardson@intel.com>
> Sent: Monday, May 23, 2022 7:14 PM
> To: Amit Prakash Shukla <amitprakashs@marvell.com>
> Cc: Anatoly Burakov <anatoly.burakov@intel.com>; Ciara Power
> <ciara.power@intel.com>; dev@dpdk.org; Jerin Jacob Kollanukkaran
> <jerinj@marvell.com>
> Subject: Re: [EXT] Re: [PATCH v2] mem: telemetry support for memseg and
> element information
> 
> 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 <bruce.richardson@intel.com>
> > > Sent: Monday, May 23, 2022 4:45 PM
> > > To: Amit Prakash Shukla <amitprakashs@marvell.com>
> > > Cc: Anatoly Burakov <anatoly.burakov@intel.com>; Ciara Power
> > > <ciara.power@intel.com>; dev@dpdk.org; Jerin Jacob Kollanukkaran
> > > <jerinj@marvell.com>
> > > 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,<memseg-list-id> 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,<memseg-list-id>"}
> > > >
> > > > --> /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.

Sure.

> 
> Are the memsegs only identified by number inside each memseg list? Are
> there no names that could be used instead?

Each memseg-list contains a fbarray and fbarray contains list of memsegs.
Memsegs in fbarray are identified by numbers.

> 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? 

Have kept memseg_list_list and memseg_list_info separate for users to identify, 
while requesting memseg info, which memseg list a memseg belongs to.

>How many memsegs are there likely
> to be?

Max memsegs per list is 8192.
#define RTE_MAX_MEMSEG_LISTS 128
#define RTE_MAX_MEMSEG_PER_LIST 8192 

> > >
> > > > 3. /eal/memseg_info,<memseg-list-id>:<memseg-id>
> > > > 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,<memseg-list-id>: \
> > > > <memseg-id>"}
> > > >
> > > > --> /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,<heap-id>:<memseg-list-id>:<memseg-id>
> > > > 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,<heap-id>:  \
> > > > <memseg-list-id>:<memseg-id>"}
> > > >
> > > > --> /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,<heap-id>:<memseg-list-id>:<memseg-id>:  \
> > > >    <elem-start-id>-<elem-end-id>
> > > > 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,<heap-id>:  \
> > > > <memseg-list-id>:<memseg-id>: <elem-start-id>-<elem-end-id>"}
> > > >
> >
> > The last 2 arguments "<elem-start-id>-<elem-end-id>" 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

Thanks,
Amit Shukla