RE: [EXT] Re: [PATCH v5 1/2] mem: telemetry support for memseg and element information

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

Thanks Dmitry for the feedback. Please find my reply in-line.

> -----Original Message-----
> From: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> Sent: Thursday, October 20, 2022 5:11 PM
> To: Amit Prakash Shukla <amitprakashs@marvell.com>
> Cc: Anatoly Burakov <anatoly.burakov@intel.com>; dev@dpdk.org; Jerin
> Jacob Kollanukkaran <jerinj@marvell.com>; david.marchand@redhat.com;
> bruce.richardson@intel.com; ciara.power@intel.com
> Subject: [EXT] Re: [PATCH v5 1/2] mem: telemetry support for memseg and
> element information
> 
> External Email
> 
> ----------------------------------------------------------------------
> 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,<memseg-list-id>
> > 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.

Agreed. Will implement new request for MSL properties in v6.

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

Idea to print Memseg_list_len was to display total number of memsegs in the MSL, 
But I agree with you that it makes more sense to be printed in new request for
MSL properties.

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

Sure, will add it in v6.

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

Request for list of active heap Id's is already present. 
" /eal/heap_list"

> 
> Please use a consistent naming scheme for requests returning ID lists.
> Currently MSL have "_array" suffix but memsegs and elements don't.

Sure

> 
> > --> /eal/element_list,0,1,15
> > {"/eal/element_list": {"Element_count": 52}}
> >
> > 5. /eal/element_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.
> > 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.

Idea here was to print information related to memory element. This information
Can be printed for a single element or for a range of elements.

> Maybe instead there should be a filter by address with maybe a context
> parameter (like "grep -C")?

You mean that the user shall be able to grep for a memory address to check
which element it belongs to ? If my understanding is correct, can I implement
it as new request post rc2 and keep this request as-is?

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

Sure, will do it in v6.

> 
> 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);
> 
Sure.

> [...]
> >  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").
Sure.