[PATCH 24.03 0/4] add new command line argument parsing library

[PATCH 24.03 0/4] add new command line argument parsing library

[Euan Bourke]

A recent thread on the mailing list[1] discussed corelist and coremask parsing
and the idea of a new library dedicated to command line parsing was
mentioned[2]. As I had been working on a public corelist parsing API at the
time, this patchset adds the library, along with the new APIs, edits to EAL
functions to call the API instead of implementing their own, and changes to
the dlb2 driver to call the API.

The new APIs work similar to existing functions in EAL, however instead of
filling a core array like this:
[1, -1, -1, 2, 3] (a non -1 refers to an 'active core' at that index)
It fills it like this:
[0, 3, 4] (with the value at each index being an 'active core').

The new APIs will also return the number of cores contained in the passed
corelist/coremask, so in the above example, 3 would be returned.

Planned future work for the library contains more parsing functions such as a
"core string parser" which would take a string and return whether its a corelist
or coremask based on heuristics. This could then be passed into the appropriate
function.

There are also several functions in EAL that implement their own version of a
corelist/coremask parser, so the plan is to rework those in future versions of
this patch.

[1] Link to start of thread:
https://mails.dpdk.org/archives/dev/2023-November/280957.html
[2] Link to the mention of a new library:
https://mails.dpdk.org/archives/dev/2023-November/280966.html

Euan Bourke (4):
  arg_parser: new library for command line parsing
  arg_parser: add new coremask parsing API
  eal: add support for new arg parsing library
  dlb2: add new arg parsing library API support

 .mailmap                                   |   1 +
 MAINTAINERS                                |   5 +
 doc/api/doxy-api-index.md                  |   3 +-
 doc/api/doxy-api.conf.in                   |   1 +
 drivers/event/dlb2/dlb2_priv.h             |   4 +-
 drivers/event/dlb2/pf/base/dlb2_resource.c |  54 +++----
 lib/arg_parser/arg_parser.c                | 171 +++++++++++++++++++++
 lib/arg_parser/meson.build                 |   7 +
 lib/arg_parser/rte_arg_parser.h            |  99 ++++++++++++
 lib/arg_parser/version.map                 |  11 ++
 lib/eal/common/eal_common_options.c        | 114 ++------------
 lib/eal/meson.build                        |   2 +-
 lib/meson.build                            |   1 +
 13 files changed, 334 insertions(+), 139 deletions(-)
 create mode 100644 lib/arg_parser/arg_parser.c
 create mode 100644 lib/arg_parser/meson.build
 create mode 100644 lib/arg_parser/rte_arg_parser.h
 create mode 100644 lib/arg_parser/version.map

-- 
2.34.1

[PATCH 24.03 1/4] arg_parser: new library for command line parsing

[Euan Bourke]

Add a new library to make it easier for eal and other libraries to parse
command line arguments.

The first function in this library is one to parse a corelist string into an
array of individual core ids. The function will then return the total number
of cores described in the corelist

Signed-off-by: Euan Bourke <euan.bourke@intel.com>
---
 .mailmap                        |   1 +
 MAINTAINERS                     |   5 ++
 doc/api/doxy-api-index.md       |   3 +-
 doc/api/doxy-api.conf.in        |   1 +
 lib/arg_parser/arg_parser.c     | 113 ++++++++++++++++++++++++++++++++
 lib/arg_parser/meson.build      |   7 ++
 lib/arg_parser/rte_arg_parser.h |  66 +++++++++++++++++++
 lib/arg_parser/version.map      |  10 +++
 lib/meson.build                 |   1 +
 9 files changed, 206 insertions(+), 1 deletion(-)
 create mode 100644 lib/arg_parser/arg_parser.c
 create mode 100644 lib/arg_parser/meson.build
 create mode 100644 lib/arg_parser/rte_arg_parser.h
 create mode 100644 lib/arg_parser/version.map

diff --git a/.mailmap b/.mailmap
index 72b216df9c..c1a4bf85f6 100644
--- a/.mailmap
+++ b/.mailmap
@@ -379,6 +379,7 @@ Eric Zhang <eric.zhang@windriver.com>
 Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
 Erik Ziegenbalg <eziegenb@brocade.com>
 Erlu Chen <erlu.chen@intel.com>
+Euan Bourke <euan.bourke@intel.com>
 Eugenio Pérez <eperezma@redhat.com>
 Eugeny Parshutin <eugeny.parshutin@linux.intel.com>
 Evan Swanson <evan.swanson@intel.com>
diff --git a/MAINTAINERS b/MAINTAINERS
index cf2af0d3a4..ce81877ce0 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1753,6 +1753,11 @@ M: Nithin Dabilpuram <ndabilpuram@marvell.com>
 M: Pavan Nikhilesh <pbhagavatula@marvell.com>
 F: lib/node/
 
+Argument parsing
+M: Bruce Richardson <bruce.richardson@intel.com>
+M: Euan Bourke <euan.bourke@intel.com>
+F: lib/arg_parser/
+
 
 Test Applications
 -----------------
diff --git a/doc/api/doxy-api-index.md b/doc/api/doxy-api-index.md
index a6a768bd7c..f711010140 100644
--- a/doc/api/doxy-api-index.md
+++ b/doc/api/doxy-api-index.md
@@ -221,7 +221,8 @@ The public API headers are grouped by topics:
   [config file](@ref rte_cfgfile.h),
   [key/value args](@ref rte_kvargs.h),
   [string](@ref rte_string_fns.h),
-  [thread](@ref rte_thread.h)
+  [thread](@ref rte_thread.h),
+  [argument parsing](@ref rte_arg_parser.h)
 
 - **debug**:
   [jobstats](@ref rte_jobstats.h),
diff --git a/doc/api/doxy-api.conf.in b/doc/api/doxy-api.conf.in
index e94c9e4e46..05718ba6ed 100644
--- a/doc/api/doxy-api.conf.in
+++ b/doc/api/doxy-api.conf.in
@@ -28,6 +28,7 @@ INPUT                   = @TOPDIR@/doc/api/doxy-api-index.md \
                           @TOPDIR@/lib/eal/include \
                           @TOPDIR@/lib/eal/include/generic \
                           @TOPDIR@/lib/acl \
+                          @TOPDIR@/lib/arg_parser \
                           @TOPDIR@/lib/bbdev \
                           @TOPDIR@/lib/bitratestats \
                           @TOPDIR@/lib/bpf \
diff --git a/lib/arg_parser/arg_parser.c b/lib/arg_parser/arg_parser.c
new file mode 100644
index 0000000000..45acaf5631
--- /dev/null
+++ b/lib/arg_parser/arg_parser.c
@@ -0,0 +1,113 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2023 Intel Corporation
+ */
+
+#include "errno.h"
+#include "stdlib.h"
+#include "ctype.h"
+#include "string.h"
+#include "stdbool.h"
+
+#include <rte_arg_parser.h>
+#include <rte_common.h>
+
+
+struct core_bits {
+	uint8_t bits[(UINT16_MAX + 1)/CHAR_BIT];
+	uint16_t max_bit_set;
+	uint16_t min_bit_set;
+	uint32_t total_bits_set;
+};
+
+static inline bool
+get_core_bit(struct core_bits *mask, uint16_t idx)
+{
+	return !!(mask->bits[idx/8] & (1 << (idx % 8)));
+}
+
+static inline void
+set_core_bit(struct core_bits *mask, uint16_t idx)
+{
+	if (get_core_bit(mask, idx) == 0) {
+		mask->total_bits_set++;
+
+		/* If its the first bit, assign min and max that value */
+		if (mask->total_bits_set == 1) {
+			mask->min_bit_set = idx;
+			mask->max_bit_set = idx;
+		}
+	}
+
+	mask->bits[idx/8] |= 1 << (idx % 8);
+
+	if (idx > mask->max_bit_set)
+		mask->max_bit_set = idx;
+
+	if (idx < mask->min_bit_set)
+		mask->min_bit_set = idx;
+}
+
+static inline void
+corebits_to_array(struct core_bits *mask, uint16_t *cores, size_t max_cores)
+{
+	uint32_t count = 0;
+	for (uint32_t i = mask->min_bit_set; i <= mask->max_bit_set && count < max_cores; i++) {
+		if (get_core_bit(mask, i))
+			cores[count++] = i;
+	}
+}
+
+
+int
+rte_parse_corelist(const char *corelist, uint16_t *cores, uint32_t cores_len)
+{
+	int32_t min = -1;
+	char *end = NULL;
+
+	struct core_bits *mask = malloc(sizeof(struct core_bits));
+	memset(mask, 0, sizeof(struct core_bits));
+
+	min = -1;
+	do {
+		uint32_t idx;
+		int32_t max;
+
+		while (isblank(*corelist))
+			corelist++;
+		if (!isdigit(*corelist))
+			return -1;
+		errno = 0;
+		idx = strtol(corelist, &end, 10);
+		if (errno || end == NULL)
+			return -1;
+		if (idx > UINT16_MAX)
+			return -1;
+		while (isblank(*end))
+			end++;
+		if (*end == '-')
+			min = idx;
+
+		else if (*end == ',' || *end == '\0') {
+			max = idx;
+			if (min == -1)
+				min = idx;
+
+			/* Swap min and max if min is larger than max */
+			if (min > max)
+				RTE_SWAP(min, max);
+
+			for (; min <= max; min++)
+				set_core_bit(mask, min);
+
+			min = -1;
+		} else
+			return -1;
+		corelist = end + 1;
+	} while (*end != '\0');
+
+	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/meson.build b/lib/arg_parser/meson.build
new file mode 100644
index 0000000000..6ee228bd69
--- /dev/null
+++ b/lib/arg_parser/meson.build
@@ -0,0 +1,7 @@
+# SPDX-License-Identifier: BSD-3-Clause
+# Copyright(c) 2023 Intel Corporation
+
+sources = files('arg_parser.c')
+headers = files('rte_arg_parser.h')
+
+includes += global_inc
diff --git a/lib/arg_parser/rte_arg_parser.h b/lib/arg_parser/rte_arg_parser.h
new file mode 100644
index 0000000000..1b12bf451f
--- /dev/null
+++ b/lib/arg_parser/rte_arg_parser.h
@@ -0,0 +1,66 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2023 Intel Corporation
+ */
+
+#ifndef _RTE_ARG_PARSER_H_
+#define _RTE_ARG_PARSER_H_
+
+/**
+ * @file
+ *
+ * RTE Argument Parsing API
+ *
+ * The argument parsing API is a collection of functions to help parse
+ * command line arguments. The API takes a string input and will return
+ * it to the user in a more usable format.
+ * 
+ */
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#include <stdint.h>
+
+#include <rte_compat.h>
+
+
+/**
+ * Convert a string describing a list of core ids into an array of core ids.
+ *
+ * On success, the passed array is filled with the core ids present in the
+ * list up to the "cores_len", and the length of the array is returned.
+ * For example, passing a 1-3,6 "corelist" results in an array of [1, 2, 3, 6]
+ * and would return 4.
+ * 
+ * Like the snprintf function for strings, if the length of the input array is
+ * insufficient to hold the number of cores in the "corelist", 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 corelist
+ *   Input string describing a list 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 "corelist",
+ *   only "cores_len" elements will be written to the array.
+ * @return
+ *   n: the number of unique cores present in "corelist".
+ *   -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_corelist(const char *corelist, uint16_t *cores, uint32_t cores_len);
+
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* _RTE_ARG_PARSER_H_ */
diff --git a/lib/arg_parser/version.map b/lib/arg_parser/version.map
new file mode 100644
index 0000000000..f11699a306
--- /dev/null
+++ b/lib/arg_parser/version.map
@@ -0,0 +1,10 @@
+DPDK_24 {
+    local: *;
+};
+
+EXPERIMENTAL {
+    global:
+
+    # added in 24.03
+    rte_parse_corelist;
+};
diff --git a/lib/meson.build b/lib/meson.build
index 6c143ce5a6..1b068fc61f 100644
--- a/lib/meson.build
+++ b/lib/meson.build
@@ -11,6 +11,7 @@
 libraries = [
         'log',
         'kvargs', # eal depends on kvargs
+        'arg_parser',
         'telemetry', # basic info querying
         'eal', # everything depends on eal
         'ring',
-- 
2.34.1

Re: [PATCH 24.03 1/4] arg_parser: new library for command line parsing

[Bruce Richardson]

On Wed, Nov 22, 2023 at 04:45:47PM +0000, Euan Bourke wrote:
> Add a new library to make it easier for eal and other libraries to parse
> command line arguments.
> 
> The first function in this library is one to parse a corelist string into an
> array of individual core ids. The function will then return the total number
> of cores described in the corelist
> 
> Signed-off-by: Euan Bourke <euan.bourke@intel.com>

Thanks for the patchset. Some comments inline below.

/Bruce

> ---
>  .mailmap                        |   1 +
>  MAINTAINERS                     |   5 ++
>  doc/api/doxy-api-index.md       |   3 +-
>  doc/api/doxy-api.conf.in        |   1 +
>  lib/arg_parser/arg_parser.c     | 113 ++++++++++++++++++++++++++++++++
>  lib/arg_parser/meson.build      |   7 ++
>  lib/arg_parser/rte_arg_parser.h |  66 +++++++++++++++++++
>  lib/arg_parser/version.map      |  10 +++
>  lib/meson.build                 |   1 +
>  9 files changed, 206 insertions(+), 1 deletion(-)
>  create mode 100644 lib/arg_parser/arg_parser.c
>  create mode 100644 lib/arg_parser/meson.build
>  create mode 100644 lib/arg_parser/rte_arg_parser.h
>  create mode 100644 lib/arg_parser/version.map
> 
> diff --git a/.mailmap b/.mailmap
> index 72b216df9c..c1a4bf85f6 100644
> --- a/.mailmap
> +++ b/.mailmap
> @@ -379,6 +379,7 @@ Eric Zhang <eric.zhang@windriver.com>
>  Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
>  Erik Ziegenbalg <eziegenb@brocade.com>
>  Erlu Chen <erlu.chen@intel.com>
> +Euan Bourke <euan.bourke@intel.com>
>  Eugenio Pérez <eperezma@redhat.com>
>  Eugeny Parshutin <eugeny.parshutin@linux.intel.com>
>  Evan Swanson <evan.swanson@intel.com>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index cf2af0d3a4..ce81877ce0 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1753,6 +1753,11 @@ M: Nithin Dabilpuram <ndabilpuram@marvell.com>
>  M: Pavan Nikhilesh <pbhagavatula@marvell.com>
>  F: lib/node/
>  
> +Argument parsing
> +M: Bruce Richardson <bruce.richardson@intel.com>
> +M: Euan Bourke <euan.bourke@intel.com>
> +F: lib/arg_parser/
> +
>  
>  Test Applications
>  -----------------
> diff --git a/doc/api/doxy-api-index.md b/doc/api/doxy-api-index.md
> index a6a768bd7c..f711010140 100644
> --- a/doc/api/doxy-api-index.md
> +++ b/doc/api/doxy-api-index.md
> @@ -221,7 +221,8 @@ The public API headers are grouped by topics:
>    [config file](@ref rte_cfgfile.h),
>    [key/value args](@ref rte_kvargs.h),
>    [string](@ref rte_string_fns.h),
> -  [thread](@ref rte_thread.h)
> +  [thread](@ref rte_thread.h),
> +  [argument parsing](@ref rte_arg_parser.h)
>  
>  - **debug**:
>    [jobstats](@ref rte_jobstats.h),
> diff --git a/doc/api/doxy-api.conf.in b/doc/api/doxy-api.conf.in
> index e94c9e4e46..05718ba6ed 100644
> --- a/doc/api/doxy-api.conf.in
> +++ b/doc/api/doxy-api.conf.in
> @@ -28,6 +28,7 @@ INPUT                   = @TOPDIR@/doc/api/doxy-api-index.md \
>                            @TOPDIR@/lib/eal/include \
>                            @TOPDIR@/lib/eal/include/generic \
>                            @TOPDIR@/lib/acl \
> +                          @TOPDIR@/lib/arg_parser \
>                            @TOPDIR@/lib/bbdev \
>                            @TOPDIR@/lib/bitratestats \
>                            @TOPDIR@/lib/bpf \
> diff --git a/lib/arg_parser/arg_parser.c b/lib/arg_parser/arg_parser.c
> new file mode 100644
> index 0000000000..45acaf5631
> --- /dev/null
> +++ b/lib/arg_parser/arg_parser.c
> @@ -0,0 +1,113 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2023 Intel Corporation
> + */
> +
> +#include "errno.h"
> +#include "stdlib.h"
> +#include "ctype.h"
> +#include "string.h"
> +#include "stdbool.h"
> +
> +#include <rte_arg_parser.h>
> +#include <rte_common.h>
> +
> +
> +struct core_bits {
> +	uint8_t bits[(UINT16_MAX + 1)/CHAR_BIT];
> +	uint16_t max_bit_set;
> +	uint16_t min_bit_set;
> +	uint32_t total_bits_set;
> +};
> +
> +static inline bool
> +get_core_bit(struct core_bits *mask, uint16_t idx)
> +{
> +	return !!(mask->bits[idx/8] & (1 << (idx % 8)));

Very minor nit, whitespace around the "/" in idx/8.

> +}
> +
> +static inline void
> +set_core_bit(struct core_bits *mask, uint16_t idx)
> +{
> +	if (get_core_bit(mask, idx) == 0) {

The function would be simpler flipping the comparison, since we do nothing
if the bit is already set.

	if (get_core_bit(mask, idx))
	    return;

Thereafter you can unconditionally set the bit, and increment
total_bits_set, before branching for total_bits_set == 1, and the min/max
comparison in the else leg of that.

> +		mask->total_bits_set++;
> +
> +		/* If its the first bit, assign min and max that value */
> +		if (mask->total_bits_set == 1) {
> +			mask->min_bit_set = idx;
> +			mask->max_bit_set = idx;
> +		}
> +	}
> +
> +	mask->bits[idx/8] |= 1 << (idx % 8);
> +
> +	if (idx > mask->max_bit_set)
> +		mask->max_bit_set = idx;
> +
> +	if (idx < mask->min_bit_set)
> +		mask->min_bit_set = idx;
> +}
> +
> +static inline void
> +corebits_to_array(struct core_bits *mask, uint16_t *cores, size_t max_cores)
> +{
> +	uint32_t count = 0;
> +	for (uint32_t i = mask->min_bit_set; i <= mask->max_bit_set && count < max_cores; i++) {
> +		if (get_core_bit(mask, i))
> +			cores[count++] = i;
> +	}
> +}
> +
> +
> +int
> +rte_parse_corelist(const char *corelist, uint16_t *cores, uint32_t cores_len)
> +{
> +	int32_t min = -1;
> +	char *end = NULL;
> +
> +	struct core_bits *mask = malloc(sizeof(struct core_bits));
> +	memset(mask, 0, sizeof(struct core_bits));
> +
> +	min = -1;
> +	do {
> +		uint32_t idx;
> +		int32_t max;
> +
> +		while (isblank(*corelist))
> +			corelist++;
> +		if (!isdigit(*corelist))
> +			return -1;

a blank line here wouldn't hurt to break up the block.

> +		errno = 0;
> +		idx = strtol(corelist, &end, 10);
> +		if (errno || end == NULL)
> +			return -1;
> +		if (idx > UINT16_MAX)
> +			return -1;

Can shorten code by merging these two conditions since both just return -1.

> +		while (isblank(*end))
> +			end++;
> +		if (*end == '-')
> +			min = idx;
> +
> +		else if (*end == ',' || *end == '\0') {
> +			max = idx;
> +			if (min == -1)
> +				min = idx;
> +
> +			/* Swap min and max if min is larger than max */
> +			if (min > max)
> +				RTE_SWAP(min, max);
> +
> +			for (; min <= max; min++)
> +				set_core_bit(mask, min);
> +
> +			min = -1;
> +		} else
> +			return -1;
> +		corelist = end + 1;
> +	} while (*end != '\0');
> +
> +	corebits_to_array(mask, cores, cores_len);
> +	uint32_t total_count = mask->total_bits_set;

corebits_to_array() should return total_bits_set, save accessing the
structure directly.

> +	free(mask);
> +
> +	return total_count;
> +}
> diff --git a/lib/arg_parser/meson.build b/lib/arg_parser/meson.build
> new file mode 100644
> index 0000000000..6ee228bd69
> --- /dev/null
> +++ b/lib/arg_parser/meson.build
> @@ -0,0 +1,7 @@
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright(c) 2023 Intel Corporation
> +
> +sources = files('arg_parser.c')
> +headers = files('rte_arg_parser.h')
> +
> +includes += global_inc
> diff --git a/lib/arg_parser/rte_arg_parser.h b/lib/arg_parser/rte_arg_parser.h
> new file mode 100644
> index 0000000000..1b12bf451f
> --- /dev/null
> +++ b/lib/arg_parser/rte_arg_parser.h
> @@ -0,0 +1,66 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2023 Intel Corporation
> + */
> +
> +#ifndef _RTE_ARG_PARSER_H_
> +#define _RTE_ARG_PARSER_H_
> +
> +/**
> + * @file
> + *
> + * RTE Argument Parsing API
> + *
> + * The argument parsing API is a collection of functions to help parse
> + * command line arguments. The API takes a string input and will return
> + * it to the user in a more usable format.
> + * 
> + */
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +#include <stdint.h>
> +
> +#include <rte_compat.h>
> +
> +
> +/**
> + * Convert a string describing a list of core ids into an array of core ids.
> + *
> + * On success, the passed array is filled with the core ids present in the
> + * list up to the "cores_len", and the length of the array is returned.
> + * For example, passing a 1-3,6 "corelist" results in an array of [1, 2, 3, 6]
> + * and would return 4.
> + * 
> + * Like the snprintf function for strings, if the length of the input array is
> + * insufficient to hold the number of cores in the "corelist", 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 corelist
> + *   Input string describing a list 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 "corelist",
> + *   only "cores_len" elements will be written to the array.
> + * @return
> + *   n: the number of unique cores present in "corelist".
> + *   -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_corelist(const char *corelist, uint16_t *cores, uint32_t cores_len);
> +
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +#endif /* _RTE_ARG_PARSER_H_ */
> diff --git a/lib/arg_parser/version.map b/lib/arg_parser/version.map
> new file mode 100644
> index 0000000000..f11699a306
> --- /dev/null
> +++ b/lib/arg_parser/version.map
> @@ -0,0 +1,10 @@
> +DPDK_24 {
> +    local: *;
> +};
> +
> +EXPERIMENTAL {
> +    global:
> +
> +    # added in 24.03
> +    rte_parse_corelist;
> +};
> diff --git a/lib/meson.build b/lib/meson.build
> index 6c143ce5a6..1b068fc61f 100644
> --- a/lib/meson.build
> +++ b/lib/meson.build
> @@ -11,6 +11,7 @@
>  libraries = [
>          'log',
>          'kvargs', # eal depends on kvargs
> +        'arg_parser',
>          'telemetry', # basic info querying
>          'eal', # everything depends on eal
>          'ring',

The new lib needs to be added to the list for windows builds too.

> -- 
> 2.34.1
> 

[PATCH 24.03 2/4] arg_parser: add new coremask parsing API

[Euan Bourke]

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.

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

Re: [PATCH 24.03 2/4] arg_parser: add new coremask parsing API

[Bruce Richardson]

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
> 

[PATCH 24.03 3/4] eal: add support for new arg parsing library

[Euan Bourke]

Update to eal functions relating to corelist and coremask parsing to support the
new arg parsing library. Functions now call the API instead of implementing
their own version.

Signed-off-by: Euan Bourke <euan.bourke@intel.com>
---
 lib/eal/common/eal_common_options.c | 114 ++++------------------------
 lib/eal/meson.build                 |   2 +-
 2 files changed, 14 insertions(+), 102 deletions(-)

diff --git a/lib/eal/common/eal_common_options.c b/lib/eal/common/eal_common_options.c
index a6d21f1cba..7c89a1e18b 100644
--- a/lib/eal/common/eal_common_options.c
+++ b/lib/eal/common/eal_common_options.c
@@ -30,6 +30,7 @@
 #include <rte_tailq.h>
 #include <rte_version.h>
 #include <rte_devargs.h>
+#include <rte_arg_parser.h>
 #include <rte_memcpy.h>
 #ifndef RTE_EXEC_ENV_WINDOWS
 #include <rte_telemetry.h>
@@ -706,7 +707,7 @@ update_lcore_config(int *cores)
 }
 
 static int
-check_core_list(int *lcores, unsigned int count)
+check_core_list(uint16_t *lcores, unsigned int count)
 {
 	char lcorestr[RTE_MAX_LCORE * 10];
 	bool overflow = false;
@@ -746,60 +747,18 @@ check_core_list(int *lcores, unsigned int count)
 int
 rte_eal_parse_coremask(const char *coremask, int *cores)
 {
-	const char *coremask_orig = coremask;
-	int lcores[RTE_MAX_LCORE];
-	unsigned int count = 0;
-	int i, j, idx;
-	int val;
-	char c;
+	int count;
+	uint16_t lcores[RTE_MAX_LCORE];
+	int idx;
 
 	for (idx = 0; idx < RTE_MAX_LCORE; idx++)
 		cores[idx] = -1;
-	idx = 0;
 
-	/* 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')))
-		coremask += 2;
-	i = strlen(coremask);
-	while ((i > 0) && isblank(coremask[i - 1]))
-		i--;
-	if (i == 0) {
-		RTE_LOG(ERR, EAL, "No lcores in coremask: [%s]\n",
-			coremask_orig);
-		return -1;
-	}
+	/* Call public coremask parsing API */
+	count = rte_parse_coremask(coremask, lcores, RTE_MAX_LCORE);
 
-	for (i = i - 1; i >= 0; i--) {
-		c = coremask[i];
-		if (isxdigit(c) == 0) {
-			/* invalid characters */
-			RTE_LOG(ERR, EAL, "invalid characters in coremask: [%s]\n",
-				coremask_orig);
-			return -1;
-		}
-		val = xdigit2val(c);
-		for (j = 0; j < BITS_PER_HEX; j++, idx++)
-		{
-			if ((1 << j) & val) {
-				if (count >= RTE_MAX_LCORE) {
-					RTE_LOG(ERR, EAL, "Too many lcores provided. Cannot exceed RTE_MAX_LCORE (%d)\n",
-						RTE_MAX_LCORE);
-					return -1;
-				}
-				lcores[count++] = idx;
-			}
-		}
-	}
-	if (count == 0) {
-		RTE_LOG(ERR, EAL, "No lcores in coremask: [%s]\n",
-			coremask_orig);
+	if (count <= 0 || count > RTE_MAX_LCORE)
 		return -1;
-	}
 
 	if (check_core_list(lcores, count))
 		return -1;
@@ -898,64 +857,17 @@ eal_parse_service_corelist(const char *corelist)
 static int
 eal_parse_corelist(const char *corelist, int *cores)
 {
-	unsigned int count = 0, i;
-	int lcores[RTE_MAX_LCORE];
-	char *end = NULL;
-	int min, max;
+	int count;
+	uint16_t lcores[RTE_MAX_LCORE];
 	int idx;
 
 	for (idx = 0; idx < RTE_MAX_LCORE; idx++)
 		cores[idx] = -1;
 
-	/* Remove all blank characters ahead */
-	while (isblank(*corelist))
-		corelist++;
+	/* Call public corelist parsing API */
+	count = rte_parse_corelist(corelist, lcores, RTE_MAX_LCORE);
 
-	/* Get list of cores */
-	min = -1;
-	do {
-		while (isblank(*corelist))
-			corelist++;
-		if (*corelist == '\0')
-			return -1;
-		errno = 0;
-		idx = strtol(corelist, &end, 10);
-		if (errno || end == NULL)
-			return -1;
-		if (idx < 0)
-			return -1;
-		while (isblank(*end))
-			end++;
-		if (*end == '-') {
-			min = idx;
-		} else if ((*end == ',') || (*end == '\0')) {
-			max = idx;
-			if (min == -1)
-				min = idx;
-			for (idx = min; idx <= max; idx++) {
-				bool dup = false;
-
-				/* Check if this idx is already present */
-				for (i = 0; i < count; i++) {
-					if (lcores[i] == idx)
-						dup = true;
-				}
-				if (dup)
-					continue;
-				if (count >= RTE_MAX_LCORE) {
-					RTE_LOG(ERR, EAL, "Too many lcores provided. Cannot exceed RTE_MAX_LCORE (%d)\n",
-						RTE_MAX_LCORE);
-					return -1;
-				}
-				lcores[count++] = idx;
-			}
-			min = -1;
-		} else
-			return -1;
-		corelist = end + 1;
-	} while (*end != '\0');
-
-	if (count == 0)
+	if (count <= 0 || count > RTE_MAX_LCORE)
 		return -1;
 
 	if (check_core_list(lcores, count))
diff --git a/lib/eal/meson.build b/lib/eal/meson.build
index e1d6c4cf17..52a22e2e66 100644
--- a/lib/eal/meson.build
+++ b/lib/eal/meson.build
@@ -14,7 +14,7 @@ subdir(exec_env)
 
 subdir(arch_subdir)
 
-deps += ['log', 'kvargs']
+deps += ['log', 'kvargs', 'arg_parser']
 if not is_windows
     deps += ['telemetry']
 endif
-- 
2.34.1

[PATCH 24.03 4/4] dlb2: add new arg parsing library API support

[Euan Bourke]

Switched the dlb2 driver to call the new arg parsing library instead of eal for
coremask parsing, and updated the resource probe funcion to support the changed
formatting of the API.

Signed-off-by: Euan Bourke <euan.bourke@intel.com>
---
 drivers/event/dlb2/dlb2_priv.h             |  4 +-
 drivers/event/dlb2/pf/base/dlb2_resource.c | 54 +++++++++-------------
 2 files changed, 22 insertions(+), 36 deletions(-)

diff --git a/drivers/event/dlb2/dlb2_priv.h b/drivers/event/dlb2/dlb2_priv.h
index 31a3beeb6c..c14d83da5b 100644
--- a/drivers/event/dlb2/dlb2_priv.h
+++ b/drivers/event/dlb2/dlb2_priv.h
@@ -10,6 +10,7 @@
 
 #include <rte_eventdev.h>
 #include <rte_config.h>
+#include <rte_arg_parser.h>
 #include "dlb2_user.h"
 #include "dlb2_log.h"
 #include "rte_pmd_dlb2.h"
@@ -729,9 +730,6 @@ void dlb2_event_build_hcws(struct dlb2_port *qm_port,
 			   uint8_t *sched_type,
 			   uint8_t *queue_id);
 
-/* Extern functions */
-extern int rte_eal_parse_coremask(const char *coremask, int *cores);
-
 /* Extern globals */
 extern struct process_local_port_data dlb2_port[][DLB2_NUM_PORT_TYPES];
 
diff --git a/drivers/event/dlb2/pf/base/dlb2_resource.c b/drivers/event/dlb2/pf/base/dlb2_resource.c
index 7ce3e3531c..422d075ff8 100644
--- a/drivers/event/dlb2/pf/base/dlb2_resource.c
+++ b/drivers/event/dlb2/pf/base/dlb2_resource.c
@@ -922,49 +922,37 @@ dlb2_resource_probe(struct dlb2_hw *hw, const void *probe_args)
 {
 	const struct dlb2_devargs *args = (const struct dlb2_devargs *)probe_args;
 	const char *mask = args ? args->producer_coremask : NULL;
-	int cpu = 0, cnt = 0, cores[RTE_MAX_LCORE], i;
+	int cpu = 0, i;
+	uint16_t cores[RTE_MAX_LCORE];
 
 	if (args) {
 		mask = (const char *)args->producer_coremask;
 	}
 
-	if (mask && rte_eal_parse_coremask(mask, cores)) {
+	int ret = rte_parse_coremask(mask, cores, RTE_DIM(cores));
+
+	if (mask && ret == -1) {
 		DLB2_LOG_ERR(": Invalid producer coremask=%s", mask);
 		return -1;
 	}
 
-	hw->num_prod_cores = 0;
-	for (i = 0; i < RTE_MAX_LCORE; i++) {
-		bool is_pcore = (mask && cores[i] != -1);
-
-		if (rte_lcore_is_enabled(i)) {
-			if (is_pcore) {
-				/*
-				 * Populate the producer cores from parsed
-				 * coremask
-				 */
-				hw->prod_core_list[cores[i]] = i;
-				hw->num_prod_cores++;
-
-			} else if ((++cnt == DLB2_EAL_PROBE_CORE ||
-			   rte_lcore_count() < DLB2_EAL_PROBE_CORE)) {
-				/*
-				 * If no producer coremask is provided, use the
-				 * second EAL core to probe
-				 */
-				cpu = i;
-				break;
-			}
-		} else if (is_pcore) {
-			DLB2_LOG_ERR("Producer coremask(%s) must be a subset of EAL coremask",
-				     mask);
-			return -1;
-		}
+	hw->num_prod_cores = ret;
+	/* Check for no producer cores and then get the second EAL core */
+	if (hw->num_prod_cores > 0)
+		cpu = cores[0];
+	else if (rte_lcore_count() < DLB2_EAL_PROBE_CORE)
+		cpu = rte_get_main_lcore();
+	else
+		cpu = rte_get_next_lcore(-1, 1, 0);
 
-	}
-	/* Use the first core in producer coremask to probe */
-	if (hw->num_prod_cores)
-		cpu = hw->prod_core_list[0];
+	/* check our producer list is valid and error out if not */
+	for (i = 0; i < hw->num_prod_cores; i++) {
+		if (!rte_lcore_is_enabled(cores[i])) {
+			DLB2_LOG_ERR("Producer coremask(%s) must be a subset of EAL coremask", mask);						 			 				 
+			return -1;
+	} 
+	hw->prod_core_list[i] = cores[i];
+}
 
 	dlb2_get_pp_allocation(hw, cpu, DLB2_LDB_PORT);
 	dlb2_get_pp_allocation(hw, cpu, DLB2_DIR_PORT);
-- 
2.34.1

[PATCH 24.03 v2 0/5] add new command line argument parsing library

[Euan Bourke]

A recent thread on the mailing list[1] discussed corelist and coremask
parsing and the idea of a new library dedicated to command line parsing
was mentioned[2]. This patchset adds the library, along with the new
APIs, edits to EAL functions to call the API instead of implementing
their own, and changes to the dlb2 driver to call the API.

The new APIs work similar to existing functions in EAL, however instead
of filling a core array like this:
[1, -1, -1, 2, 3] (a non -1 refers to an 'active core' at that index)
It fills it like this:
[0, 3, 4] (with the value at each index being an 'active core').

The new APIs will also return the number of cores contained in the
passed corelist/coremask, so in the above example, 3 would be returned.

Planned future work for the library contains more parsing functions such
as a "core string parser" which would take a string and return whether
its a corelist or coremask based on heuristics. This could then be
passed into the appropriate function.

There are also several example applications that implement their own
version of a corelist/coremask parser, so the plan is to rework those in
future versions of this patch.

[1] https://mails.dpdk.org/archives/dev/2023-November/280957.html
[2] https://mails.dpdk.org/archives/dev/2023-November/280966.html


v2:
* changes to EAL service core related parsers to call API.
* various optimisations in core_bit related functions in arg_parser.c.
* add lib to list for windows build.
* minor changes for issues flagged during review.

Euan Bourke (5):
  arg_parser: new library for command line parsing
  arg_parser: add new coremask parsing API
  eal: add support for new arg parsing library
  eal: update to service core related parsers
  event/dlb2: add new arg parsing library API support

 .mailmap                                   |   1 +
 MAINTAINERS                                |   4 +
 doc/api/doxy-api-index.md                  |   3 +-
 doc/api/doxy-api.conf.in                   |   1 +
 drivers/event/dlb2/dlb2_priv.h             |   4 +-
 drivers/event/dlb2/pf/base/dlb2_resource.c |  51 ++--
 lib/arg_parser/arg_parser.c                | 170 ++++++++++++
 lib/arg_parser/meson.build                 |   7 +
 lib/arg_parser/rte_arg_parser.h            |  98 +++++++
 lib/arg_parser/version.map                 |  11 +
 lib/eal/common/eal_common_options.c        | 291 +++++----------------
 lib/eal/meson.build                        |   2 +-
 lib/meson.build                            |   2 +
 13 files changed, 378 insertions(+), 267 deletions(-)
 create mode 100644 lib/arg_parser/arg_parser.c
 create mode 100644 lib/arg_parser/meson.build
 create mode 100644 lib/arg_parser/rte_arg_parser.h
 create mode 100644 lib/arg_parser/version.map

-- 
2.34.1

[PATCH 24.03 v2 1/5] arg_parser: new library for command line parsing

[Euan Bourke]

Add a new library to make it easier for eal and other libraries to parse
command line arguments.

The first function in this library is one to parse a corelist string
into an array of individual core ids. The function will then return the
total number of cores described in the corelist.

Signed-off-by: Euan Bourke <euan.bourke@intel.com>
---
 .mailmap                        |   1 +
 MAINTAINERS                     |   4 ++
 doc/api/doxy-api-index.md       |   3 +-
 doc/api/doxy-api.conf.in        |   1 +
 lib/arg_parser/arg_parser.c     | 113 ++++++++++++++++++++++++++++++++
 lib/arg_parser/meson.build      |   7 ++
 lib/arg_parser/rte_arg_parser.h |  65 ++++++++++++++++++
 lib/arg_parser/version.map      |  10 +++
 lib/meson.build                 |   2 +
 9 files changed, 205 insertions(+), 1 deletion(-)
 create mode 100644 lib/arg_parser/arg_parser.c
 create mode 100644 lib/arg_parser/meson.build
 create mode 100644 lib/arg_parser/rte_arg_parser.h
 create mode 100644 lib/arg_parser/version.map

diff --git a/.mailmap b/.mailmap
index ab0742a382..528bc68a30 100644
--- a/.mailmap
+++ b/.mailmap
@@ -379,6 +379,7 @@ Eric Zhang <eric.zhang@windriver.com>
 Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
 Erik Ziegenbalg <eziegenb@brocade.com>
 Erlu Chen <erlu.chen@intel.com>
+Euan Bourke <euan.bourke@intel.com>
 Eugenio Pérez <eperezma@redhat.com>
 Eugeny Parshutin <eugeny.parshutin@linux.intel.com>
 Evan Swanson <evan.swanson@intel.com>
diff --git a/MAINTAINERS b/MAINTAINERS
index 0d1c8126e3..68ef5ba14b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1756,6 +1756,10 @@ M: Nithin Dabilpuram <ndabilpuram@marvell.com>
 M: Pavan Nikhilesh <pbhagavatula@marvell.com>
 F: lib/node/
 
+Argument parsing
+M: Bruce Richardson <bruce.richardson@intel.com>
+F: lib/arg_parser/
+
 
 Test Applications
 -----------------
diff --git a/doc/api/doxy-api-index.md b/doc/api/doxy-api-index.md
index a6a768bd7c..f711010140 100644
--- a/doc/api/doxy-api-index.md
+++ b/doc/api/doxy-api-index.md
@@ -221,7 +221,8 @@ The public API headers are grouped by topics:
   [config file](@ref rte_cfgfile.h),
   [key/value args](@ref rte_kvargs.h),
   [string](@ref rte_string_fns.h),
-  [thread](@ref rte_thread.h)
+  [thread](@ref rte_thread.h),
+  [argument parsing](@ref rte_arg_parser.h)
 
 - **debug**:
   [jobstats](@ref rte_jobstats.h),
diff --git a/doc/api/doxy-api.conf.in b/doc/api/doxy-api.conf.in
index e94c9e4e46..05718ba6ed 100644
--- a/doc/api/doxy-api.conf.in
+++ b/doc/api/doxy-api.conf.in
@@ -28,6 +28,7 @@ INPUT                   = @TOPDIR@/doc/api/doxy-api-index.md \
                           @TOPDIR@/lib/eal/include \
                           @TOPDIR@/lib/eal/include/generic \
                           @TOPDIR@/lib/acl \
+                          @TOPDIR@/lib/arg_parser \
                           @TOPDIR@/lib/bbdev \
                           @TOPDIR@/lib/bitratestats \
                           @TOPDIR@/lib/bpf \
diff --git a/lib/arg_parser/arg_parser.c b/lib/arg_parser/arg_parser.c
new file mode 100644
index 0000000000..4aa876b4ed
--- /dev/null
+++ b/lib/arg_parser/arg_parser.c
@@ -0,0 +1,113 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2023 Intel Corporation
+ */
+
+#include "errno.h"
+#include "stdlib.h"
+#include "ctype.h"
+#include "string.h"
+#include "stdbool.h"
+
+#include <rte_arg_parser.h>
+#include <rte_common.h>
+
+
+struct core_bits {
+	uint8_t bits[(UINT16_MAX + 1)/CHAR_BIT];
+	uint16_t max_bit_set;
+	uint16_t min_bit_set;
+	uint32_t total_bits_set;
+};
+
+static inline bool
+get_core_bit(struct core_bits *mask, uint16_t idx)
+{
+	return !!(mask->bits[idx / 8] & (1 << (idx % 8)));
+}
+
+static inline void
+set_core_bit(struct core_bits *mask, uint16_t idx)
+{
+	if (get_core_bit(mask, idx))
+		return;
+
+	mask->bits[idx/8] |= 1 << (idx % 8);
+	/* Update min and max bit if its first time setting a bit */
+	if (++(mask->total_bits_set) == 1) {
+		mask->min_bit_set = idx;
+		mask->max_bit_set = idx;
+		return;
+	}
+
+	if (idx > mask->max_bit_set)
+		mask->max_bit_set = idx;
+
+	if (idx < mask->min_bit_set)
+		mask->min_bit_set = idx;
+}
+
+static inline uint32_t
+corebits_to_array(struct core_bits *mask, uint16_t *cores, size_t max_cores)
+{
+	uint32_t count = 0;
+	for (uint32_t i = mask->min_bit_set; i <= mask->max_bit_set && count < max_cores; i++) {
+		if (get_core_bit(mask, i))
+			cores[count++] = i;
+	}
+	return mask->total_bits_set;
+}
+
+
+int
+rte_parse_corelist(const char *corelist, uint16_t *cores, uint32_t cores_len)
+{
+	int32_t min = -1;
+	char *end = NULL;
+
+	struct core_bits *mask = malloc(sizeof(struct core_bits));
+	if (mask == NULL)
+		return -1;
+	memset(mask, 0, sizeof(struct core_bits));
+
+	min = -1;
+	do {
+		uint32_t idx;
+		int32_t max;
+
+		while (isblank(*corelist))
+			corelist++;
+		if (!isdigit(*corelist))
+			return -1;
+
+		errno = 0;
+		idx = strtol(corelist, &end, 10);
+		if (errno || end == NULL || idx > UINT16_MAX)
+			return -1;
+		while (isblank(*end))
+			end++;
+		if (*end == '-')
+			min = idx;
+
+		else if (*end == ',' || *end == '\0') {
+			max = idx;
+			if (min == -1)
+				min = idx;
+
+			/* Swap min and max if min is larger than max */
+			if (min > max)
+				RTE_SWAP(min, max);
+
+			for (; min <= max; min++)
+				set_core_bit(mask, min);
+
+			min = -1;
+		} else
+			return -1;
+		corelist = end + 1;
+	} while (*end != '\0');
+
+	uint32_t total_count = corebits_to_array(mask, cores, cores_len);
+	free(mask);
+
+	return total_count;
+}
diff --git a/lib/arg_parser/meson.build b/lib/arg_parser/meson.build
new file mode 100644
index 0000000000..6ee228bd69
--- /dev/null
+++ b/lib/arg_parser/meson.build
@@ -0,0 +1,7 @@
+# SPDX-License-Identifier: BSD-3-Clause
+# Copyright(c) 2023 Intel Corporation
+
+sources = files('arg_parser.c')
+headers = files('rte_arg_parser.h')
+
+includes += global_inc
diff --git a/lib/arg_parser/rte_arg_parser.h b/lib/arg_parser/rte_arg_parser.h
new file mode 100644
index 0000000000..cf9291d13a
--- /dev/null
+++ b/lib/arg_parser/rte_arg_parser.h
@@ -0,0 +1,65 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2023 Intel Corporation
+ */
+
+#ifndef _RTE_ARG_PARSER_H_
+#define _RTE_ARG_PARSER_H_
+
+/**
+ * @file
+ *
+ * RTE Argument Parsing API
+ *
+ * The argument parsing API is a collection of functions to help parse
+ * command line arguments. The API takes a string input and will return
+ * it to the user in a more usable format.
+ */
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#include <stdint.h>
+
+#include <rte_compat.h>
+
+
+/**
+ * Convert a string describing a list of core ids into an array of core ids.
+ *
+ * On success, the passed array is filled with the core ids present in the
+ * list up to the "cores_len", and the length of the array is returned.
+ * For example, passing a 1-3,6 "corelist" results in an array of [1, 2, 3, 6]
+ * and would return 4.
+ *
+ * Like the snprintf function for strings, if the length of the input array is
+ * insufficient to hold the number of cores in the "corelist", 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 corelist
+ *   Input string describing a list 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 "corelist",
+ *   only "cores_len" elements will be written to the array.
+ * @return
+ *   n: the number of unique cores present in "corelist".
+ *   -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_corelist(const char *corelist, uint16_t *cores, uint32_t cores_len);
+
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* _RTE_ARG_PARSER_H_ */
diff --git a/lib/arg_parser/version.map b/lib/arg_parser/version.map
new file mode 100644
index 0000000000..588e14c7ad
--- /dev/null
+++ b/lib/arg_parser/version.map
@@ -0,0 +1,10 @@
+DPDK_24 {
+	local: *;
+};
+
+EXPERIMENTAL {
+	global:
+
+	# added in 24.03
+	rte_parse_corelist;
+};
diff --git a/lib/meson.build b/lib/meson.build
index 6c143ce5a6..db9e769033 100644
--- a/lib/meson.build
+++ b/lib/meson.build
@@ -11,6 +11,7 @@
 libraries = [
         'log',
         'kvargs', # eal depends on kvargs
+        'arg_parser',
         'telemetry', # basic info querying
         'eal', # everything depends on eal
         'ring',
@@ -72,6 +73,7 @@ if is_ms_compiler
             'log',
             'kvargs',
             'telemetry',
+            'arg_parser',
     ]
 endif
 
-- 
2.34.1

Re: [PATCH 24.03 v2 1/5] arg_parser: new library for command line parsing

[Stephen Hemminger]

On Tue, 28 Nov 2023 14:07:41 +0000
Euan Bourke <euan.bourke@intel.com> wrote:

> +struct core_bits {
> +	uint8_t bits[(UINT16_MAX + 1)/CHAR_BIT];
> +	uint16_t max_bit_set;
> +	uint16_t min_bit_set;
> +	uint32_t total_bits_set;
> +};

Looks like a good candidate for flex array

Re: [PATCH 24.03 v2 1/5] arg_parser: new library for command line parsing

[Stephen Hemminger]

On Tue, 28 Nov 2023 14:07:41 +0000
Euan Bourke <euan.bourke@intel.com> wrote:

> +static inline bool
> +get_core_bit(struct core_bits *mask, uint16_t idx)
> +{
> +	return !!(mask->bits[idx / 8] & (1 << (idx % 8)));
> +}
> +

You used CHAR_BIT above, why not here?

Re: [PATCH 24.03 v2 1/5] arg_parser: new library for command line parsing

[Stephen Hemminger]

On Tue, 28 Nov 2023 14:07:41 +0000
Euan Bourke <euan.bourke@intel.com> wrote:

> +int
> +rte_parse_corelist(const char *corelist, uint16_t *cores, uint32_t cores_len)
> +{
> +	int32_t min = -1;
> +	char *end = NULL;
> +
> +	struct core_bits *mask = malloc(sizeof(struct core_bits));
> +	if (mask == NULL)
> +		return -1;
> +	memset(mask, 0, sizeof(struct core_bits));
> +

The structure is not variable size, and small, why bother using malloc().
Just use on stack structure.

Re: [PATCH 24.03 v2 1/5] arg_parser: new library for command line parsing

[Bruce Richardson]

On Wed, Nov 29, 2023 at 02:14:13PM -0800, Stephen Hemminger wrote:
> On Tue, 28 Nov 2023 14:07:41 +0000
> Euan Bourke <euan.bourke@intel.com> wrote:
> 
> > +int
> > +rte_parse_corelist(const char *corelist, uint16_t *cores, uint32_t cores_len)
> > +{
> > +	int32_t min = -1;
> > +	char *end = NULL;
> > +
> > +	struct core_bits *mask = malloc(sizeof(struct core_bits));
> > +	if (mask == NULL)
> > +		return -1;
> > +	memset(mask, 0, sizeof(struct core_bits));
> > +
> 
> The structure is not variable size, and small, why bother using malloc().
> Just use on stack structure.

Well, it's just over 4k in size, and we hit problems in the past with
alpine linux where the stack size wasn't as big as expected, leading to
issues with telemetry string manipulation. In that case I think it was due
to nested calls as well as large stack structs, though. For this core
parsing, having this struct on stack is unlikely to cause issues, but for a
non-datapath API I suggests to Euan to use malloc as a lower-risk
alternative, since performance of an arg parsing API is unlikely to be a
major concern.

However, if you feel that it should be on the stack and won't cause any
issues, it does make the code shorter and easier to work with!

/Bruce

[PATCH 24.03 v2 2/5] arg_parser: add new coremask parsing API

[Euan Bourke]

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.

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>
---
 lib/arg_parser/arg_parser.c     | 57 +++++++++++++++++++++++++++++++++
 lib/arg_parser/rte_arg_parser.h | 33 +++++++++++++++++++
 lib/arg_parser/version.map      |  1 +
 3 files changed, 91 insertions(+)

diff --git a/lib/arg_parser/arg_parser.c b/lib/arg_parser/arg_parser.c
index 4aa876b4ed..6405eebd03 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)
+
 
 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)
 	return mask->total_bits_set;
 }
 
+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,48 @@ 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));
+	if (mask == NULL)
+		return -1;
+	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')))
+		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;
+
+	for (i = i - 1; i >= 0; i--) {
+		int val;
+		char c = coremask[i];
+
+		if (isxdigit(c) == 0)
+			return -1;
+
+		val = xdigit2val(c);
+
+		for (uint8_t j = 0; j < BITS_PER_HEX; j++, idx++) {
+			if ((1 << j) & val)
+				set_core_bit(mask, idx);
+		}
+	}
+
+	uint32_t total_count = corebits_to_array(mask, cores, cores_len);
+	free(mask);
+
+	return total_count;
+}
diff --git a/lib/arg_parser/rte_arg_parser.h b/lib/arg_parser/rte_arg_parser.h
index cf9291d13a..956239d085 100644
--- a/lib/arg_parser/rte_arg_parser.h
+++ b/lib/arg_parser/rte_arg_parser.h
@@ -57,6 +57,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 588e14c7ad..ac070db785 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

[PATCH 24.03 v2 3/5] eal: add support for new arg parsing library

[Euan Bourke]

Update to EAL functions relating to corelist and coremask parsing to
support the new arg parsing library. Functions now call the API instead
of implementing their own version.

Signed-off-by: Euan Bourke <euan.bourke@intel.com>
---
 lib/eal/common/eal_common_options.c | 114 ++++------------------------
 lib/eal/meson.build                 |   2 +-
 2 files changed, 14 insertions(+), 102 deletions(-)

diff --git a/lib/eal/common/eal_common_options.c b/lib/eal/common/eal_common_options.c
index a6d21f1cba..7c89a1e18b 100644
--- a/lib/eal/common/eal_common_options.c
+++ b/lib/eal/common/eal_common_options.c
@@ -30,6 +30,7 @@
 #include <rte_tailq.h>
 #include <rte_version.h>
 #include <rte_devargs.h>
+#include <rte_arg_parser.h>
 #include <rte_memcpy.h>
 #ifndef RTE_EXEC_ENV_WINDOWS
 #include <rte_telemetry.h>
@@ -706,7 +707,7 @@ update_lcore_config(int *cores)
 }
 
 static int
-check_core_list(int *lcores, unsigned int count)
+check_core_list(uint16_t *lcores, unsigned int count)
 {
 	char lcorestr[RTE_MAX_LCORE * 10];
 	bool overflow = false;
@@ -746,60 +747,18 @@ check_core_list(int *lcores, unsigned int count)
 int
 rte_eal_parse_coremask(const char *coremask, int *cores)
 {
-	const char *coremask_orig = coremask;
-	int lcores[RTE_MAX_LCORE];
-	unsigned int count = 0;
-	int i, j, idx;
-	int val;
-	char c;
+	int count;
+	uint16_t lcores[RTE_MAX_LCORE];
+	int idx;
 
 	for (idx = 0; idx < RTE_MAX_LCORE; idx++)
 		cores[idx] = -1;
-	idx = 0;
 
-	/* 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')))
-		coremask += 2;
-	i = strlen(coremask);
-	while ((i > 0) && isblank(coremask[i - 1]))
-		i--;
-	if (i == 0) {
-		RTE_LOG(ERR, EAL, "No lcores in coremask: [%s]\n",
-			coremask_orig);
-		return -1;
-	}
+	/* Call public coremask parsing API */
+	count = rte_parse_coremask(coremask, lcores, RTE_MAX_LCORE);
 
-	for (i = i - 1; i >= 0; i--) {
-		c = coremask[i];
-		if (isxdigit(c) == 0) {
-			/* invalid characters */
-			RTE_LOG(ERR, EAL, "invalid characters in coremask: [%s]\n",
-				coremask_orig);
-			return -1;
-		}
-		val = xdigit2val(c);
-		for (j = 0; j < BITS_PER_HEX; j++, idx++)
-		{
-			if ((1 << j) & val) {
-				if (count >= RTE_MAX_LCORE) {
-					RTE_LOG(ERR, EAL, "Too many lcores provided. Cannot exceed RTE_MAX_LCORE (%d)\n",
-						RTE_MAX_LCORE);
-					return -1;
-				}
-				lcores[count++] = idx;
-			}
-		}
-	}
-	if (count == 0) {
-		RTE_LOG(ERR, EAL, "No lcores in coremask: [%s]\n",
-			coremask_orig);
+	if (count <= 0 || count > RTE_MAX_LCORE)
 		return -1;
-	}
 
 	if (check_core_list(lcores, count))
 		return -1;
@@ -898,64 +857,17 @@ eal_parse_service_corelist(const char *corelist)
 static int
 eal_parse_corelist(const char *corelist, int *cores)
 {
-	unsigned int count = 0, i;
-	int lcores[RTE_MAX_LCORE];
-	char *end = NULL;
-	int min, max;
+	int count;
+	uint16_t lcores[RTE_MAX_LCORE];
 	int idx;
 
 	for (idx = 0; idx < RTE_MAX_LCORE; idx++)
 		cores[idx] = -1;
 
-	/* Remove all blank characters ahead */
-	while (isblank(*corelist))
-		corelist++;
+	/* Call public corelist parsing API */
+	count = rte_parse_corelist(corelist, lcores, RTE_MAX_LCORE);
 
-	/* Get list of cores */
-	min = -1;
-	do {
-		while (isblank(*corelist))
-			corelist++;
-		if (*corelist == '\0')
-			return -1;
-		errno = 0;
-		idx = strtol(corelist, &end, 10);
-		if (errno || end == NULL)
-			return -1;
-		if (idx < 0)
-			return -1;
-		while (isblank(*end))
-			end++;
-		if (*end == '-') {
-			min = idx;
-		} else if ((*end == ',') || (*end == '\0')) {
-			max = idx;
-			if (min == -1)
-				min = idx;
-			for (idx = min; idx <= max; idx++) {
-				bool dup = false;
-
-				/* Check if this idx is already present */
-				for (i = 0; i < count; i++) {
-					if (lcores[i] == idx)
-						dup = true;
-				}
-				if (dup)
-					continue;
-				if (count >= RTE_MAX_LCORE) {
-					RTE_LOG(ERR, EAL, "Too many lcores provided. Cannot exceed RTE_MAX_LCORE (%d)\n",
-						RTE_MAX_LCORE);
-					return -1;
-				}
-				lcores[count++] = idx;
-			}
-			min = -1;
-		} else
-			return -1;
-		corelist = end + 1;
-	} while (*end != '\0');
-
-	if (count == 0)
+	if (count <= 0 || count > RTE_MAX_LCORE)
 		return -1;
 
 	if (check_core_list(lcores, count))
diff --git a/lib/eal/meson.build b/lib/eal/meson.build
index e1d6c4cf17..52a22e2e66 100644
--- a/lib/eal/meson.build
+++ b/lib/eal/meson.build
@@ -14,7 +14,7 @@ subdir(exec_env)
 
 subdir(arch_subdir)
 
-deps += ['log', 'kvargs']
+deps += ['log', 'kvargs', 'arg_parser']
 if not is_windows
     deps += ['telemetry']
 endif
-- 
2.34.1

[PATCH 24.03 v2 4/5] eal: update to service core related parsers

[Euan Bourke]

Updates to the parse service cores functions in EAL to call the arg
parser API instead of implementing its own versions.

Signed-off-by: Euan Bourke <euan.bourke@intel.com>
---
 lib/eal/common/eal_common_options.c | 177 ++++++++--------------------
 1 file changed, 47 insertions(+), 130 deletions(-)

diff --git a/lib/eal/common/eal_common_options.c b/lib/eal/common/eal_common_options.c
index 7c89a1e18b..bf70d22975 100644
--- a/lib/eal/common/eal_common_options.c
+++ b/lib/eal/common/eal_common_options.c
@@ -573,97 +573,52 @@ eal_plugins_init(void)
  * the global configuration (core role and core count) with the parsed
  * value.
  */
-static int xdigit2val(unsigned char c)
-{
-	int val;
-
-	if (isdigit(c))
-		val = c - '0';
-	else if (isupper(c))
-		val = c - 'A' + 10;
-	else
-		val = c - 'a' + 10;
-	return val;
-}
-
 static int
 eal_parse_service_coremask(const char *coremask)
 {
 	struct rte_config *cfg = rte_eal_get_configuration();
-	int i, j, idx = 0;
-	unsigned int count = 0;
-	char c;
-	int val;
+	uint16_t cores[RTE_MAX_LCORE];
+	int64_t count;
 	uint32_t taken_lcore_count = 0;
 
-	if (coremask == NULL)
-		return -1;
-	/* 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')))
-		coremask += 2;
-	i = strlen(coremask);
-	while ((i > 0) && isblank(coremask[i - 1]))
-		i--;
-
-	if (i == 0)
-		return -1;
-
-	for (i = i - 1; i >= 0 && idx < RTE_MAX_LCORE; i--) {
-		c = coremask[i];
-		if (isxdigit(c) == 0) {
-			/* invalid characters */
-			return -1;
-		}
-		val = xdigit2val(c);
-		for (j = 0; j < BITS_PER_HEX && idx < RTE_MAX_LCORE;
-				j++, idx++) {
-			if ((1 << j) & val) {
-				/* handle main lcore already parsed */
-				uint32_t lcore = idx;
-				if (main_lcore_parsed &&
-						cfg->main_lcore == lcore) {
-					RTE_LOG(ERR, EAL,
-						"lcore %u is main lcore, cannot use as service core\n",
-						idx);
-					return -1;
-				}
+	for (uint16_t idx = 0; idx < RTE_DIM(cores); idx++)
+		cores[idx] = -1;
 
-				if (eal_cpu_detected(idx) == 0) {
-					RTE_LOG(ERR, EAL,
-						"lcore %u unavailable\n", idx);
-					return -1;
-				}
+	count = rte_parse_coremask(coremask, cores, RTE_DIM(cores));
 
-				if (cfg->lcore_role[idx] == ROLE_RTE)
-					taken_lcore_count++;
+	if (count == 0 || count == -1)
+		return -1;
 
-				lcore_config[idx].core_role = ROLE_SERVICE;
-				count++;
-			}
+	for (int i = 0; i < count; i++) {
+		uint32_t lcore = cores[i];
+		if (main_lcore_parsed &&
+				cfg->main_lcore == lcore) {
+			RTE_LOG(ERR, EAL,
+				"lcore %u is main lcore, cannot use as service core\n",
+				cores[i]);
+			return -1;
 		}
-	}
 
-	for (; i >= 0; i--)
-		if (coremask[i] != '0')
+		if (eal_cpu_detected(cores[i]) == 0) {
+			RTE_LOG(ERR, EAL,
+				"lcore %u unavailable\n", cores[i]);
 			return -1;
+		}
 
-	for (; idx < RTE_MAX_LCORE; idx++)
-		lcore_config[idx].core_index = -1;
-
-	if (count == 0)
-		return -1;
+		if (cfg->lcore_role[cores[i]] == ROLE_RTE)
+			taken_lcore_count++;
 
+		lcore_config[cores[i]].core_role = ROLE_SERVICE;
+	}
 	if (core_parsed && taken_lcore_count != count) {
 		RTE_LOG(WARNING, EAL,
-			"Not all service cores are in the coremask. "
+			"Not all service cores were in the coremask. "
 			"Please ensure -c or -l includes service cores\n");
 	}
 
+	for (uint16_t j = count*4; j < RTE_MAX_LCORE; j++)
+		lcore_config[j].core_index = -1;
+
 	cfg->service_lcore_count = count;
 	return 0;
 }
@@ -780,70 +735,32 @@ static int
 eal_parse_service_corelist(const char *corelist)
 {
 	struct rte_config *cfg = rte_eal_get_configuration();
-	int i;
-	unsigned count = 0;
-	char *end = NULL;
-	uint32_t min, max, idx;
+	int64_t i, count;
+	uint16_t cores[RTE_MAX_LCORE];
 	uint32_t taken_lcore_count = 0;
 
-	if (corelist == NULL)
-		return -1;
+	for (uint16_t idx = 0; idx < RTE_DIM(cores); idx++)
+		cores[idx] = -1;
 
-	/* Remove all blank characters ahead and after */
-	while (isblank(*corelist))
-		corelist++;
-	i = strlen(corelist);
-	while ((i > 0) && isblank(corelist[i - 1]))
-		i--;
+	count = rte_parse_corelist(corelist, cores, RTE_DIM(cores));
 
-	/* Get list of cores */
-	min = RTE_MAX_LCORE;
-	do {
-		while (isblank(*corelist))
-			corelist++;
-		if (*corelist == '\0')
-			return -1;
-		errno = 0;
-		idx = strtoul(corelist, &end, 10);
-		if (errno || end == NULL)
-			return -1;
-		if (idx >= RTE_MAX_LCORE)
-			return -1;
-		while (isblank(*end))
-			end++;
-		if (*end == '-') {
-			min = idx;
-		} else if ((*end == ',') || (*end == '\0')) {
-			max = idx;
-			if (min == RTE_MAX_LCORE)
-				min = idx;
-			for (idx = min; idx <= max; idx++) {
-				if (cfg->lcore_role[idx] != ROLE_SERVICE) {
-					/* handle main lcore already parsed */
-					uint32_t lcore = idx;
-					if (cfg->main_lcore == lcore &&
-							main_lcore_parsed) {
-						RTE_LOG(ERR, EAL,
-							"Error: lcore %u is main lcore, cannot use as service core\n",
-							idx);
-						return -1;
-					}
-					if (cfg->lcore_role[idx] == ROLE_RTE)
-						taken_lcore_count++;
-
-					lcore_config[idx].core_role =
-							ROLE_SERVICE;
-					count++;
-				}
-			}
-			min = RTE_MAX_LCORE;
-		} else
+	if (count == 0 || count == -1)
+		return -1;
+
+	for (i = 0; i < count; i++) {
+		uint32_t lcore = cores[i];
+		if (cfg->main_lcore == lcore &&
+				main_lcore_parsed) {
+			RTE_LOG(ERR, EAL,
+				"Error: lcore %u is main lcore, cannot use as service core\n",
+				cores[i]);
 			return -1;
-		corelist = end + 1;
-	} while (*end != '\0');
+		}
+		if (cfg->lcore_role[cores[i]] == ROLE_RTE)
+			taken_lcore_count++;
 
-	if (count == 0)
-		return -1;
+		lcore_config[cores[i]].core_role = ROLE_SERVICE;
+	}
 
 	if (core_parsed && taken_lcore_count != count) {
 		RTE_LOG(WARNING, EAL,
-- 
2.34.1

[PATCH 24.03 v2 5/5] event/dlb2: add new arg parsing library API support

[Euan Bourke]

Switched the dlb2 driver to call the new arg parsing library instead of
eal for coremask parsing, and updated the resource probe function to
support the changed formatting of the API.

Signed-off-by: Euan Bourke <euan.bourke@intel.com>
---
 drivers/event/dlb2/dlb2_priv.h             |  4 +-
 drivers/event/dlb2/pf/base/dlb2_resource.c | 51 +++++++++-------------
 2 files changed, 21 insertions(+), 34 deletions(-)

diff --git a/drivers/event/dlb2/dlb2_priv.h b/drivers/event/dlb2/dlb2_priv.h
index 31a3beeb6c..c14d83da5b 100644
--- a/drivers/event/dlb2/dlb2_priv.h
+++ b/drivers/event/dlb2/dlb2_priv.h
@@ -10,6 +10,7 @@
 
 #include <rte_eventdev.h>
 #include <rte_config.h>
+#include <rte_arg_parser.h>
 #include "dlb2_user.h"
 #include "dlb2_log.h"
 #include "rte_pmd_dlb2.h"
@@ -729,9 +730,6 @@ void dlb2_event_build_hcws(struct dlb2_port *qm_port,
 			   uint8_t *sched_type,
 			   uint8_t *queue_id);
 
-/* Extern functions */
-extern int rte_eal_parse_coremask(const char *coremask, int *cores);
-
 /* Extern globals */
 extern struct process_local_port_data dlb2_port[][DLB2_NUM_PORT_TYPES];
 
diff --git a/drivers/event/dlb2/pf/base/dlb2_resource.c b/drivers/event/dlb2/pf/base/dlb2_resource.c
index 7ce3e3531c..fda094c3f7 100644
--- a/drivers/event/dlb2/pf/base/dlb2_resource.c
+++ b/drivers/event/dlb2/pf/base/dlb2_resource.c
@@ -922,49 +922,38 @@ dlb2_resource_probe(struct dlb2_hw *hw, const void *probe_args)
 {
 	const struct dlb2_devargs *args = (const struct dlb2_devargs *)probe_args;
 	const char *mask = args ? args->producer_coremask : NULL;
-	int cpu = 0, cnt = 0, cores[RTE_MAX_LCORE], i;
+	int cpu = 0, i;
+	uint16_t cores[RTE_MAX_LCORE];
 
 	if (args) {
 		mask = (const char *)args->producer_coremask;
 	}
 
-	if (mask && rte_eal_parse_coremask(mask, cores)) {
+	int ret = rte_parse_coremask(mask, cores, RTE_DIM(cores));
+
+	if (mask && ret == -1) {
 		DLB2_LOG_ERR(": Invalid producer coremask=%s", mask);
 		return -1;
 	}
 
-	hw->num_prod_cores = 0;
-	for (i = 0; i < RTE_MAX_LCORE; i++) {
-		bool is_pcore = (mask && cores[i] != -1);
-
-		if (rte_lcore_is_enabled(i)) {
-			if (is_pcore) {
-				/*
-				 * Populate the producer cores from parsed
-				 * coremask
-				 */
-				hw->prod_core_list[cores[i]] = i;
-				hw->num_prod_cores++;
-
-			} else if ((++cnt == DLB2_EAL_PROBE_CORE ||
-			   rte_lcore_count() < DLB2_EAL_PROBE_CORE)) {
-				/*
-				 * If no producer coremask is provided, use the
-				 * second EAL core to probe
-				 */
-				cpu = i;
-				break;
-			}
-		} else if (is_pcore) {
+	hw->num_prod_cores = ret;
+	/* Check for no producer cores and then get the second EAL core */
+	if (hw->num_prod_cores > 0)
+		cpu = cores[0];
+	else if (rte_lcore_count() < DLB2_EAL_PROBE_CORE)
+		cpu = rte_get_main_lcore();
+	else
+		cpu = rte_get_next_lcore(-1, 1, 0);
+
+	/* check our producer list is valid and error out if not */
+	for (i = 0; i < hw->num_prod_cores; i++) {
+		if (!rte_lcore_is_enabled(cores[i])) {
 			DLB2_LOG_ERR("Producer coremask(%s) must be a subset of EAL coremask",
-				     mask);
+				mask);
 			return -1;
-		}
-
 	}
-	/* Use the first core in producer coremask to probe */
-	if (hw->num_prod_cores)
-		cpu = hw->prod_core_list[0];
+	hw->prod_core_list[i] = cores[i];
+}
 
 	dlb2_get_pp_allocation(hw, cpu, DLB2_LDB_PORT);
 	dlb2_get_pp_allocation(hw, cpu, DLB2_DIR_PORT);
-- 
2.34.1