On Wed, Nov 22, 2023 at 04:45:48PM +0000, Euan Bourke wrote:
> Add new coremask parsing API. This API behaves similarly to the corelist
> parsing
> API, parsing the coremask string, filling its values into the cores array.
>
General tip - commit log messages are generally wrapped at 72 characters.
> The API also returns a 'count' which corresponds to the total number of cores
> in the coremask string.
>
> Signed-off-by: Euan Bourke <euan.bou...@intel.com>
Again, some review comments inline below.
/Bruce
> ---
> lib/arg_parser/arg_parser.c | 58 +++++++++++++++++++++++++++++++++
> lib/arg_parser/rte_arg_parser.h | 33 +++++++++++++++++++
> lib/arg_parser/version.map | 1 +
> 3 files changed, 92 insertions(+)
>
> diff --git a/lib/arg_parser/arg_parser.c b/lib/arg_parser/arg_parser.c
> index 45acaf5631..58be94b67d 100644
> --- a/lib/arg_parser/arg_parser.c
> +++ b/lib/arg_parser/arg_parser.c
> @@ -11,6 +11,9 @@
> #include <rte_arg_parser.h>
> #include <rte_common.h>
>
> +#define BITS_PER_HEX 4
> +#define MAX_COREMASK_SIZE ((UINT16_MAX+1)/BITS_PER_HEX)
> +
Whitespace around "/" operator here, and below in bits definition (which I
missed on review of first patch).
>
> struct core_bits {
> uint8_t bits[(UINT16_MAX + 1)/CHAR_BIT];
> @@ -57,6 +60,15 @@ corebits_to_array(struct core_bits *mask, uint16_t *cores,
> size_t max_cores)
> }
> }
>
> +static int xdigit2val(unsigned char c)
> +{
> + if (isdigit(c))
> + return c - '0';
> + else if (isupper(c))
> + return c - 'A' + 10;
> + else
> + return c - 'a' + 10;
> +}
>
> int
> rte_parse_corelist(const char *corelist, uint16_t *cores, uint32_t cores_len)
> @@ -111,3 +123,49 @@ rte_parse_corelist(const char *corelist, uint16_t
> *cores, uint32_t cores_len)
>
> return total_count;
> }
> +
> +int
> +rte_parse_coremask(const char *coremask, uint16_t *cores, uint32_t cores_len)
> +{
> + struct core_bits *mask = malloc(sizeof(struct core_bits));
Check return value from malloc. Need to do so in patch 1 also.
> + memset(mask, 0, sizeof(struct core_bits));
> +
> + /* Remove all blank characters ahead and after .
> + * Remove 0x/0X if exists.
> + */
> + while (isblank(*coremask))
> + coremask++;
> + if (coremask[0] == '0' && ((coremask[1] == 'x')
> + || (coremask[1] == 'X')))
Nit: this can all fit on one line, as it's <100 chars long.
> + coremask += 2;
> +
> + int32_t i = strlen(coremask);
> + while ((i > 0) && isblank(coremask[i - 1]))
> + i--;
> + if (i == 0 || i > MAX_COREMASK_SIZE)
> + return -1;
> +
> + uint32_t idx = 0;
> + uint8_t j;
> + int val;
> +
You can define "val" inside the for loop as it's not needed outside it.
Since we use C11 standard, you can also avoid declaring j here too, and
just do "for (uint8_t j = 0; ....)".
> + for (i = i - 1; i >= 0; i--) {
> + char c = coremask[i];
> +
> + if (isxdigit(c) == 0)
> + return -1;
> +
> + val = xdigit2val(c);
> +
> + for (j = 0; j < BITS_PER_HEX; j++, idx++) {
> + if ((1 << j) & val)
> + set_core_bit(mask, idx);
> + }
> + }
> +
> + corebits_to_array(mask, cores, cores_len);
> + uint32_t total_count = mask->total_bits_set;
> + free(mask);
> +
> + return total_count;
> +}
> diff --git a/lib/arg_parser/rte_arg_parser.h b/lib/arg_parser/rte_arg_parser.h
> index 1b12bf451f..b149b37755 100644
> --- a/lib/arg_parser/rte_arg_parser.h
> +++ b/lib/arg_parser/rte_arg_parser.h
> @@ -58,6 +58,39 @@ __rte_experimental
> int
> rte_parse_corelist(const char *corelist, uint16_t *cores, uint32_t
> cores_len);
>
> +/**
> + * Convert a string describing a bitmask of core ids into an array of core
> ids.
> + *
> + * On success, the passed array is filled with the core ids present in the
> + * bitmask up to the "cores_len", and the number of elements added into the
> array is returned.
> + * For example, passing a 0xA "coremask" results in an array of [1, 3]
> + * and would return 2.
> + *
> + * Like the snprintf function for strings, if the length of the input array
> is
> + * insufficient to hold the number of cores in the "coresmask", the input
> array is
> + * filled to capacity and the return value is the number of elements which
> would
> + * be returned if the array had been big enough.
> + * Function can also be called with a NULL array and 0 "cores_len" to find
> out
> + * the "cores_len" required.
> + *
> + * @param coremask
> + * A string containing a bitmask of core ids.
> + * @param cores
> + * An array where to store the core ids.
> + * Array can be NULL if "cores_len" is 0.
> + * @param cores_len
> + * The length of the "cores" array.
> + * If the size is smaller than that needed to hold all cores from
> "coremask",
> + * only "cores_len" elements will be written to the array.
> + * @return
> + * n: the number of unique cores present in "coremask".
> + * -1 if the string was invalid.
> + * NOTE: if n > "cores_len", then only "cores_len" elements in the "cores"
> array are valid.
> +*/
> +__rte_experimental
> +int
> +rte_parse_coremask(const char* coremask, uint16_t *cores, uint32_t
> cores_len);
> +
>
> #ifdef __cplusplus
> }
> diff --git a/lib/arg_parser/version.map b/lib/arg_parser/version.map
> index f11699a306..f334f1aaed 100644
> --- a/lib/arg_parser/version.map
> +++ b/lib/arg_parser/version.map
> @@ -7,4 +7,5 @@ EXPERIMENTAL {
>
> # added in 24.03
> rte_parse_corelist;
> + rte_parse_coremask;
> };
> --
> 2.34.1
>
