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

[Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>]

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.

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

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

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

> --> /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.
Maybe instead there should be a filter by address
with maybe a context parameter (like "grep -C")?
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.

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

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