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

[David Marchand <david.marchand@redhat.com>]

On Thu, May 19, 2022 at 8:31 AM Amit Prakash Shukla
<amitprakashs@marvell.com> wrote:
> diff --git a/lib/eal/common/eal_common_memory.c b/lib/eal/common/eal_common_memory.c
> index 688dc615d7..80dde5f0f4 100644
> --- a/lib/eal/common/eal_common_memory.c
> +++ b/lib/eal/common/eal_common_memory.c
> @@ -18,6 +18,7 @@
>  #include <rte_log.h>
>  #ifndef RTE_EXEC_ENV_WINDOWS
>  #include <rte_telemetry.h>
> +#include <telemetry_data.h>

You can remove this include after you remove later call to: memset(c,
0, sizeof(struct rte_tel_data));


>  #endif
>
>  #include "eal_memalloc.h"
> @@ -26,6 +27,7 @@
>  #include "eal_memcfg.h"
>  #include "eal_options.h"
>  #include "malloc_heap.h"
> +#include "malloc_elem.h"
>
>  /*
>   * Try to mmap *size bytes in /dev/zero. If it is successful, return the
> @@ -1113,11 +1115,17 @@ rte_eal_memory_init(void)
>  }
>
>  #ifndef RTE_EXEC_ENV_WINDOWS
> -#define EAL_MEMZONE_LIST_REQ   "/eal/memzone_list"
> -#define EAL_MEMZONE_INFO_REQ   "/eal/memzone_info"
> -#define EAL_HEAP_LIST_REQ      "/eal/heap_list"
> -#define EAL_HEAP_INFO_REQ      "/eal/heap_info"
> -#define ADDR_STR               15
> +#define EAL_MEMZONE_LIST_REQ           "/eal/memzone_list"
> +#define EAL_MEMZONE_INFO_REQ           "/eal/memzone_info"
> +#define EAL_HEAP_LIST_REQ              "/eal/heap_list"
> +#define EAL_HEAP_INFO_REQ              "/eal/heap_info"
> +#define EAL_MEMSEG_REQ                 "/eal/memseg_list"
> +#define EAL_MEMSEG_INFO_REQ            "/eal/memseg_info"
> +#define EAL_ACTIVE_MEMSEG_LIST_REQ     "/eal/active_memseg_list"
> +#define EAL_ELEMENT_LIST_REQ           "/eal/elem_list"
> +#define EAL_ELEMENT_INFO_REQ           "/eal/elem_info"
> +#define ADDR_STR                       15
> +
>
>  /* Telemetry callback handler to return heap stats for requested heap id. */
>  static int
> @@ -1252,7 +1260,6 @@ memzone_list_cb(const struct rte_memzone *mz __rte_unused,
>         rte_tel_data_add_array_int(d, mz_idx);
>  }
>
> -

Unrelated change.


>  /* Telemetry callback handler to list the memzones reserved. */
>  static int
>  handle_eal_memzone_list_request(const char *cmd __rte_unused,
> @@ -1265,6 +1272,425 @@ handle_eal_memzone_list_request(const char *cmd __rte_unused,
>         return 0;
>  }
>
> +static int
> +handle_eal_active_memseg_request(const char *cmd __rte_unused,
> +                                const char *params __rte_unused,
> +                                struct rte_tel_data *d)
> +{
> +       struct rte_mem_config *mcfg;
> +       int i;
> +
> +       rte_tel_data_start_array(d, RTE_TEL_INT_VAL);
> +
> +       rte_mcfg_mem_read_lock();
> +       mcfg = rte_eal_get_configuration()->mem_config;
> +
> +       for (i = 0; i < RTE_MAX_MEMSEG_LISTS; i++) {
> +               struct rte_memseg_list *msl = &mcfg->memsegs[i];
> +               if (msl->memseg_arr.count == 0)
> +                       continue;
> +
> +               rte_tel_data_add_array_int(d, i);
> +       }
> +       rte_mcfg_mem_read_unlock();
> +
> +       return 0;
> +}
> +
> +static int
> +handle_eal_memseg_request(const char *cmd __rte_unused, const char *params,
> +                         struct rte_tel_data *d)
> +{
> +       struct rte_mem_config *mcfg;
> +       struct rte_memseg_list *msl;
> +       struct rte_fbarray *arr;
> +       uint32_t ms_list_idx;
> +       char *params_args;
> +       int ms_idx;
> +
> +       if (params == NULL || strlen(params) == 0)
> +               return -1;
> +
> +       if (strncasecmp(params, "help", strlen(params)) == 0) {
> +               char buff[RTE_TEL_MAX_SINGLE_STRING_LEN];
> +               snprintf(buff, RTE_TEL_MAX_SINGLE_STRING_LEN,
> +                        "%s,<memseg-list-id>", EAL_MEMSEG_REQ);
> +               rte_tel_data_string(d, buff);
> +               return 0;
> +       }
> +
> +       /* strtok expects char * and param is const char *. Hence on using
> +        * params as "const char *" compiler throws warning.
> +        */

This is probably copy/paste from other handlers, there is no strtok call here.
Please remove this comment and unneeded strdup'd params_args below.


I can see that this code replies for invalid input:

Calling /eal/memseg_list,z
{"/eal/memseg_list": [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13,
14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30,
31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47,
48, 49, 50, 51, 52, 53, 54, 55, 56, 57, 58, 59, 60, 61, 62, 63, 64,
65, 66, 67, 68, 69, 70, 71, 72, 73, 74, 75, 76, 77, 78, 79, 80, 81,
82, 83, 84, 85, 86, 87, 88, 89, 90, 91, 92, 93, 94, 95, 96, 97, 98,
99, 100, 101, 102, 103, 104, 105, 106, 107, 108, 109, 110, 111, 112,
113, 114, 115, 116, 117, 118, 119, 120, 121, 122, 123, 124, 125, 126,
127, 128, 129, 130, 131, 132, 133, 134, 135, 136, 137, 138, 139, 140,
141, 142, 143, 144, 145, 146, 147, 148, 149, 150, 151, 152, 153, 154,
155, 156, 157, 158, 159, 160, 161, 162, 163, 164, 165, 166, 167, 168,
169, 170, 171, 172, 173, 174, 175, 176, 177, 178, 179, 180, 181, 182,
183, 184, 185, 186, 187, 188, 189, 190, 191, 192, 193, 194, 195, 196,
197, 198, 199, 200, 201, 202, 203, 204, 205, 206, 207, 208, 209, 210,
211, 212, 213, 214, 215, 216, 217, 218, 219, 220, 221, 222, 223, 224,
225, 226, 227, 228, 229, 230, 231, 232, 233, 234, 235, 236, 237, 238,
239, 240, 241, 242, 243, 244, 245, 246, 247, 248, 249, 250, 251, 252,
253, 254, 255, 256, 257, 258, 259, 260, 261, 262, 263, 264, 265, 266,
267, 268, 269, 270, 271, 272, 273, 274, 275, 276, 277, 278, 279, 280,
281, 282, 283, 284, 285, 286, 287, 288, 289, 290, 291, 292, 293, 294,
295, 296, 297, 298, 299, 300, 301, 302, 303, 304, 305, 306, 307, 308,
309, 310, 311, 312, 313, 314, 315, 316, 317, 318, 319, 320, 321, 322,
323, 324, 325, 326, 327, 328, 329, 330, 331, 332, 333, 334, 335, 336,
337, 338, 339, 340, 341, 342, 343, 344, 345, 346, 347, 348, 349, 350,
351, 352, 353, 354, 355, 356, 357, 358, 359, 360, 361, 362, 363, 364,
365, 366, 367, 368, 369, 370, 371, 372, 373, 374, 375, 376, 377, 378,
379, 380, 381, 382, 383, 384, 385, 386, 387, 388, 389, 390, 391, 392,
393, 394, 395, 396, 397, 398, 399, 400, 401, 402, 403, 404, 405, 406,
407, 408, 409, 410, 411, 412, 413, 414, 415, 416, 417, 418, 419, 420,
421, 422, 423, 424, 425, 426, 427, 428, 429, 430, 431, 432, 433, 434,
435, 436, 437, 438, 439, 440, 441, 442, 443, 444, 445, 446, 447, 448,
449, 450, 451, 452, 453, 454, 455, 456, 457, 458, 459, 460, 461, 462,
463, 464, 465, 466, 467, 468, 469, 470, 471, 472, 473, 474, 475, 476,
477, 478, 479, 480, 481, 482, 483, 484, 485, 486, 487, 488, 489, 490,
491, 492, 493, 494, 495, 496, 497, 498, 499, 500, 501, 502, 503, 504,
505, 506, 507, 508, 509, 510, 511]}

It might be worth adding a isdigit() check here.


> +       params_args = strdup(params);
> +       ms_list_idx = strtoul(params_args, NULL, 10);
> +       if (ms_list_idx >= RTE_MAX_MEMSEG_LISTS) {
> +               free(params_args);
> +               return -1;
> +       }
> +
> +       free(params_args);
> +       rte_tel_data_start_array(d, RTE_TEL_INT_VAL);
> +
> +       rte_mcfg_mem_read_lock();
> +       mcfg = rte_eal_get_configuration()->mem_config;
> +       msl = &mcfg->memsegs[ms_list_idx];
> +       if (msl->memseg_arr.count == 0)
> +               goto done;
> +
> +       arr = &msl->memseg_arr;
> +
> +       ms_idx = rte_fbarray_find_next_used(arr, 0);
> +       while (ms_idx >= 0) {
> +               rte_tel_data_add_array_int(d, ms_idx);
> +               ms_idx = rte_fbarray_find_next_used(arr, ms_idx + 1);
> +       }
> +
> +done:
> +       rte_mcfg_mem_read_unlock();
> +
> +       return 0;
> +}


CI reports build issues, please remove casts / fix integer formats.
I did not check further.


-- 
David Marchand