Thanks Dmitry for the feedback. Please find my reply in-line.
> -----Original Message-----
> From: Dmitry Kozlyuk <dmitry.kozl...@gmail.com>
> Sent: Thursday, October 20, 2022 5:11 PM
> To: Amit Prakash Shukla <amitpraka...@marvell.com>
> Cc: Anatoly Burakov <anatoly.bura...@intel.com>; dev@dpdk.org; Jerin
> Jacob Kollanukkaran <jer...@marvell.com>; david.march...@redhat.com;
> bruce.richard...@intel.com; ciara.po...@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.
