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

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

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.

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

> > --> /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.