From: Bruce Richardson <bruce.richardson@intel.com>
To: Euan Bourke <euan.bourke@intel.com>
Cc: <dev@dpdk.org>
Subject: Re: [PATCH 24.03 2/4] arg_parser: add new coremask parsing API
Date: Thu, 23 Nov 2023 15:55:53 +0000 [thread overview]
Message-ID: <ZV92CfXmQK9o02lt@bricha3-MOBL.ger.corp.intel.com> (raw)
In-Reply-To: <20231122164550.3873633-3-euan.bourke@intel.com>
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.bourke@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
>
next prev parent reply other threads:[~2023-11-23 15:56 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-11-22 16:45 [PATCH 24.03 0/4] add new command line argument parsing library Euan Bourke
2023-11-22 16:45 ` [PATCH 24.03 1/4] arg_parser: new library for command line parsing Euan Bourke
2023-11-23 15:50 ` Bruce Richardson
2023-11-22 16:45 ` [PATCH 24.03 2/4] arg_parser: add new coremask parsing API Euan Bourke
2023-11-23 15:55 ` Bruce Richardson [this message]
2023-11-22 16:45 ` [PATCH 24.03 3/4] eal: add support for new arg parsing library Euan Bourke
2023-11-22 16:45 ` [PATCH 24.03 4/4] dlb2: add new arg parsing library API support Euan Bourke
2023-11-28 14:07 ` [PATCH 24.03 v2 0/5] add new command line argument parsing library Euan Bourke
2023-11-28 14:07 ` [PATCH 24.03 v2 1/5] arg_parser: new library for command line parsing Euan Bourke
2023-11-29 22:12 ` Stephen Hemminger
2023-11-29 22:12 ` Stephen Hemminger
2023-11-29 22:14 ` Stephen Hemminger
2023-11-30 8:59 ` Bruce Richardson
2023-11-28 14:07 ` [PATCH 24.03 v2 2/5] arg_parser: add new coremask parsing API Euan Bourke
2023-11-28 14:07 ` [PATCH 24.03 v2 3/5] eal: add support for new arg parsing library Euan Bourke
2023-11-28 14:07 ` [PATCH 24.03 v2 4/5] eal: update to service core related parsers Euan Bourke
2023-11-28 14:07 ` [PATCH 24.03 v2 5/5] event/dlb2: add new arg parsing library API support Euan Bourke
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZV92CfXmQK9o02lt@bricha3-MOBL.ger.corp.intel.com \
--to=bruce.richardson@intel.com \
--cc=dev@dpdk.org \
--cc=euan.bourke@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).