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

[Bruce Richardson <bruce.richardson@intel.com>]

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.

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