Device querying

A new interactive command is offered:

show device <device description>

This commands lists all rte_device element matching the device description. e.g.:

show device bus=pci
show device bus=vdev
show device bus=vdev,class=eth
show device bus=vdev,class=eth,name=net_ring0

These devices may not be otherwise useful, some buses will spawn devices to keep track of their assets without having a driver to use them.

Signed-off-by Gaetan Rivet <[email protected]>

---
 app/test-pmd/cmdline.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

app/test-pmd/cmdline.c +52 -0

index 40b31ad7e..8ac5fb3ca 100644
@@ -6701,6 +6701,57 @@ cmdline_parse_inst_t cmd_showportall = {
 	},
 };
 
+/* *** SHOW DEVICE INFO *** */
+struct cmd_showdevice_result {
+	cmdline_fixed_string_t show;
+	cmdline_fixed_string_t device;
+	cmdline_fixed_string_t filter;
+};
+
+static void
+cmd_showdevice_dump_device(const struct rte_device *dev)
+{
+	const struct rte_driver *drv = dev->driver;
+
+	printf("0x%p: %s:%s\n", (const void *)dev, dev->name,
+		drv ? drv->name : "<nil>");
+}
+
+static void cmd_showdevice_parsed(void *parsed_result,
+				__attribute__((unused)) struct cmdline *cl,
+				__attribute__((unused)) void *data)
+{
+	struct cmd_showdevice_result *res = parsed_result;
+	struct rte_dev_iterator it;
+	const struct rte_device *dev;
+
+	RTE_DEV_FOREACH(dev, res->filter, &it)
+		cmd_showdevice_dump_device(dev);
+}
+
+cmdline_parse_token_string_t cmd_showdevice_show =
+	TOKEN_STRING_INITIALIZER(struct cmd_showdevice_result,
+				show, "show");
+cmdline_parse_token_string_t cmd_showdevice_device =
+	TOKEN_STRING_INITIALIZER(struct cmd_showdevice_result,
+				device, "device");
+cmdline_parse_token_string_t cmd_showdevice_filter =
+	TOKEN_STRING_INITIALIZER(struct cmd_showdevice_result,
+			filter, NULL);
+
+cmdline_parse_inst_t cmd_showdevice = {
+	.f = cmd_showdevice_parsed,
+	.data = NULL,
+	.help_str = "show device "
+		"<device string>",
+	.tokens = {
+		(void *)&cmd_showdevice_show,
+		(void *)&cmd_showdevice_device,
+		(void *)&cmd_showdevice_filter,
+		NULL,
+	},
+};
+
 /* *** SHOW PORT INFO *** */
 struct cmd_showport_result {
 	cmdline_fixed_string_t show;

@@ -16038,6 +16089,7 @@ cmdline_parse_ctx_t main_ctx[] = {
 	(cmdline_parse_inst_t *)&cmd_help_long,
 	(cmdline_parse_inst_t *)&cmd_quit,
 	(cmdline_parse_inst_t *)&cmd_load_from_file,
+	(cmdline_parse_inst_t *)&cmd_showdevice,
 	(cmdline_parse_inst_t *)&cmd_showport,
 	(cmdline_parse_inst_t *)&cmd_showqueue,
 	(cmdline_parse_inst_t *)&cmd_showportall,

Signature · 1 line

2.11.0

This patchset introduces a new EAL API for querying devices, filtered by arbitrary properties.

The following elements are introduced to this end:

* A new object, "rte_class", is used to describe the device class abstraction layer (eth, crypto, ...).

* Both rte_bus and rte_class now offer a way to
list their devices and filter the result
using locally defined properties.

* The rte_dev API now has an rte_dev_iterator, which is the way for the user to define the device filter and iterate upon the resulting set.

As an example, the "eth" device class is implemented.

Additionally, the device filters for

+ rte_bus_pci
+ rte_bus_vdev
+ rte_class_eth

are implemented and can be used with some
properties each, to show how to extend those.

Some example of filters:

"bus=pci/class=eth"
"bus=pci"
"class=eth"
"class=eth,name=net_ring0"
"bus=pci,id=00:00.0"
"bus=vdev,driver=net_ring"

Gaetan Rivet (18):

eal: introduce dtor macros

eal: introduce device class abstraction

eal/class: register destructor

eal: add lightweight kvarg parsing utility
eal/dev: add device iterator interface

eal/dev: implement device iteration initialization

eal/class: add device iteration
eal/bus: add device iteration
eal/dev: implement device iteration

ethdev: register ether layer as a class
ethdev: add device matching field name bus/pci: fix find device implementation bus/pci: implement device iteration and comparison bus/pci: add device matching field id bus/vdev: fix find device implementation bus/vdev: implement device iteration bus/vdev: add device matching field driver app/testpmd: add show device command

 app/test-pmd/cmdline.c                     |  52 +++++++++
 drivers/bus/pci/Makefile                   |   2 +-
 drivers/bus/pci/pci_common.c               |  97 ++++++++++++++--
 drivers/bus/pci/rte_bus_pci.h              |   3 +
 drivers/bus/vdev/Makefile                  |   2 +-
 drivers/bus/vdev/rte_bus_vdev.h            |   3 +
 drivers/bus/vdev/vdev.c                    |  86 +++++++++++++-
 lib/Makefile                               |   2 +-
 lib/librte_eal/bsdapp/eal/Makefile         |   1 +
 lib/librte_eal/common/Makefile             |   2 +-
 lib/librte_eal/common/eal_common_class.c   |  62 ++++++++++
 lib/librte_eal/common/eal_common_dev.c     | 177 +++++++++++++++++++++++++++++
 lib/librte_eal/common/eal_private.h        |  34 ++++++
 lib/librte_eal/common/include/rte_bus.h    |   1 +
 lib/librte_eal/common/include/rte_class.h  | 127 +++++++++++++++++++++
 lib/librte_eal/common/include/rte_common.h |  23 ++++
 lib/librte_eal/common/include/rte_dev.h    |  84 ++++++++++++++

 lib/librte_eal/linuxapp/eal/Makefile       |   1 +
 lib/librte_eal/rte_eal_version.map         |   4 +
 lib/librte_ether/Makefile                  |   3 +-
 lib/librte_ether/rte_class_eth.c           | 100 ++++++++++++++++
 21 files changed, 847 insertions(+), 19 deletions(-)
 create mode 100644 lib/librte_eal/common/eal_common_class.c
 create mode 100644 lib/librte_eal/common/include/rte_class.h
 create mode 100644 lib/librte_ether/rte_class_eth.c

Signature · 1 line

2.11.0

Signed-off-by Gaetan Rivet <[email protected]>

---
 lib/librte_eal/common/include/rte_common.h | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

lib/librte_eal/common/include/rte_common.h +13 -0

index c7803e41c..500fc3adb 100644
@@ -105,6 +105,29 @@ static void __attribute__((constructor, used)) func(void)
 static void __attribute__((constructor(prio), used)) func(void)
 
 /**
+ * Run after main() with high priority.
+ *
+ * The destructor will be run *before* prioritized destructors.
+ *
+ * @param func
+ *   Destructor function name.
+ */
+#define RTE_FINI(func) \
+static void __attribute__((destructor, used)) func(void)
+
+/**
+ * Run after main() with low priority.
+ *

lib/librte_eal/common/include/rte_common.h +0 -0

  * Force a function to be inlined
  */
 #define __rte_always_inline inline __attribute__((always_inline))

Signature · 1 line

2.11.0

Signed-off-by Gaetan Rivet <[email protected]>

---
 lib/librte_eal/common/include/rte_common.h | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

lib/librte_eal/common/include/rte_common.h +13 -0

index c7803e41c..500fc3adb 100644
@@ -105,6 +105,29 @@ static void __attribute__((constructor, used)) func(void)
 static void __attribute__((constructor(prio), used)) func(void)
 
 /**
+ * Run after main() with high priority.
+ *
+ * The destructor will be run *before* prioritized destructors.
+ *
+ * @param func
+ *   Destructor function name.
+ */
+#define RTE_FINI(func) \
+static void __attribute__((destructor, used)) func(void)
+
+/**
+ * Run after main() with low priority.
+ *

lib/librte_eal/common/include/rte_common.h +0 -0

  * Force a function to be inlined
  */
 #define __rte_always_inline inline __attribute__((always_inline))

Signature · 1 line

2.11.0

Quoted 26 lines · Gaetan Rivet

Signed-off-by Gaetan Rivet <[email protected]>

---
 lib/librte_eal/common/include/rte_common.h | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

lib/librte_eal/common/include/rte_common.h +13 -0

index c7803e41c..500fc3adb 100644
@@ -105,6 +105,29 @@ static void __attribute__((constructor, used)) func(void)
 static void __attribute__((constructor(prio), used)) func(void)
 
 /**
+ * Run after main() with high priority.
+ *
+ * The destructor will be run *before* prioritized destructors.
+ *
+ * @param func
+ *   Destructor function name.
+ */
+#define RTE_FINI(func) \
+static void __attribute__((destructor, used)) func(void)
+
+/**
+ * Run after main() with low priority.
+ *

lib/librte_eal/common/include/rte_common.h static void __attribute__((constructor, used)) func(void) · line 130

Quoted 10 lines

lib/librte_eal/common/include/rte_common.h +10 -0

+ * @param func
+ *   Destructor function name.
+ * @param prio
+ *   Priority number must be above 100.
+ *   Lowest number is the last to run.
+ */
+#define RTE_FINI_PRIO(func, prio) \
+static void __attribute__((destructor(prio), used)) func(void)
+
+/**

Additionally, it might be nice to further abstract the destructor priority to fixed levels so that people aren't always trying to guess what the right magic number should be. I.e. create several destructor macros of the form:
RTE_FINI_<NAME>

Where name is a meaningfull term like FINAL, PMD, EARLY, or some such set that implies a priority value encoded within the macro definition. That would also eliminate the need to create a BUILD BUG macro if the priority was specified to be too low

Neil

Build a central list to quickly see each used priorities for constructors, allowing to verify that they are both above 100 and in the proper order.

Cc Neil Horman <[email protected]>

Signed-off-by Gaetan Rivet <[email protected]>

---
 lib/librte_eal/common/eal_common_log.c     | 2 +-
 lib/librte_eal/common/include/rte_bus.h    | 2 +-
 lib/librte_eal/common/include/rte_common.h | 8 +++++++-
 3 files changed, 9 insertions(+), 3 deletions(-)

lib/librte_eal/common/eal_common_log.c +1 -1

index 37b2e20e5..b384c72d3 100644
@@ -224,7 +224,7 @@ static const struct logtype logtype_strings[] = {
 };
 
 /* Logging should be first initializer (before drivers and bus) */
-RTE_INIT_PRIO(rte_log_init, 101);
+RTE_INIT_PRIO(rte_log_init, LOG);
 static void
 rte_log_init(void)
 {

lib/librte_eal/common/include/rte_bus.h +1 -1

index 46911afa7..747baf140 100644
@@ -326,7 +326,7 @@ enum rte_iova_mode rte_bus_get_iommu_class(void);
  * The constructor has higher priority than PMD constructors.
  */
 #define RTE_REGISTER_BUS(nm, bus) \
-RTE_INIT_PRIO(businitfn_ ##nm, 110); \
+RTE_INIT_PRIO(businitfn_ ##nm, BUS); \
 static void businitfn_ ##nm(void) \
 {\
 	(bus).name = RTE_STR(nm);\

lib/librte_eal/common/include/rte_common.h +7 -1

index 500fc3adb..5ff0de08f 100644
@@ -81,6 +81,12 @@ typedef uint16_t unaligned_uint16_t;
  */
 #define RTE_SET_USED(x) (void)(x)
 
+#define RTE_PRIORITY_LOG 101
+#define RTE_PRIORITY_BUS 110
+
+#define RTE_PRIO(prio) \
+	RTE_PRIORITY_ ## prio
+
 /**
  * Run function before main() with low priority.
  *

@@ -102,7 +108,7 @@ static void __attribute__((constructor, used)) func(void)
  *   Lowest number is the first to run.
  */
 #define RTE_INIT_PRIO(func, prio) \
-static void __attribute__((constructor(prio), used)) func(void)
+static void __attribute__((constructor(RTE_PRIO(prio)), used)) func(void)
 
 /**
  * Run after main() with high priority.

Signature · 1 line

2.11.0

Quoted 47 lines · Neil Horman

Quoted 26 lines · Gaetan Rivet

Signed-off-by Gaetan Rivet <[email protected]>

---
 lib/librte_eal/common/include/rte_common.h | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

lib/librte_eal/common/include/rte_common.h +13 -0

index c7803e41c..500fc3adb 100644
@@ -105,6 +105,29 @@ static void __attribute__((constructor, used)) func(void)
 static void __attribute__((constructor(prio), used)) func(void)
 
 /**
+ * Run after main() with high priority.
+ *
+ * The destructor will be run *before* prioritized destructors.
+ *
+ * @param func
+ *   Destructor function name.
+ */
+#define RTE_FINI(func) \
+static void __attribute__((destructor, used)) func(void)
+
+/**
+ * Run after main() with low priority.
+ *

lib/librte_eal/common/include/rte_common.h static void __attribute__((constructor, used)) func(void) · line 130

Quoted 10 lines

lib/librte_eal/common/include/rte_common.h +10 -0

+ * @param func
+ *   Destructor function name.
+ * @param prio
+ *   Priority number must be above 100.
+ *   Lowest number is the last to run.
+ */
+#define RTE_FINI_PRIO(func, prio) \
+static void __attribute__((destructor(prio), used)) func(void)
+
+/**

Additionally, it might be nice to further abstract the destructor priority to fixed levels so that people aren't always trying to guess what the right magic number should be. I.e. create several destructor macros of the form:
RTE_FINI_<NAME>

Where name is a meaningfull term like FINAL, PMD, EARLY, or some such set that implies a priority value encoded within the macro definition. That would also eliminate the need to create a BUILD BUG macro if the priority was specified to be too low

Quoted 2 lines

Neil

Good idea, and I agree that we need helpers on this. Not sure about _FINAL and _EARLY however.

I will propose a patch as a response to this mail, let me know if that's what you had in mind.

Signature · 3 lines

-- 
Gaëtan Rivet
6WIND

This library offers a quick way to parse parameters passed with a key=value syntax.

A single function is needed and finds the relevant element within the text. No dynamic allocation is performed. It is possible to chain the parsing of each pairs for quickly scanning a list.

This utility is private to the EAL and should allow avoiding having to move around the more complete librte_kvargs.

Signed-off-by Gaetan Rivet <[email protected]>

Quoted 7 lines · Gaetan Rivet

This library offers a quick way to parse parameters passed with a key=value syntax.

Quoted 6 lines

A single function is needed and finds the relevant element within the text. No dynamic allocation is performed. It is possible to chain the parsing of each pairs for quickly scanning a list.

This utility is private to the EAL and should allow avoiding having to move around the more complete librte_kvargs.

What is the big advantage with this code and the librte_kvargs code. Is it just no allocation, rte_kvargs needs to be build before parts of EAL or what?

My concern is we have now two flavors one in EAL and one in librte_kvargs, would it not be more reasonable to improve rte_kvargs to remove your objections? I am all for fast, better, stronger code :-)

Quoted 11 lines · Gaetan Rivet

This library offers a quick way to parse parameters passed with a key=value syntax.

A single function is needed and finds the relevant element within the text. No dynamic allocation is performed. It is possible to chain the parsing of each pairs for quickly scanning a list.

Quoted 2 lines

This utility is private to the EAL and should allow avoiding having to move around the more complete librte_kvargs.

What is the big advantage with this code and the librte_kvargs code. Is it just no allocation, rte_kvargs needs to be build before parts of EAL or what?

My concern is we have now two flavors one in EAL and one in librte_kvargs, would it not be more reasonable to improve rte_kvargs to remove your objections? I am all for fast, better, stronger code :-)

Quoted 2 lines

Signed-off-by Gaetan Rivet <[email protected]>

Regards,
Keith

Quoted 16 lines · Keith

Quoted 7 lines · Gaetan Rivet

This library offers a quick way to parse parameters passed with a key=value syntax.

Quoted 6 lines

A single function is needed and finds the relevant element within the text. No dynamic allocation is performed. It is possible to chain the parsing of each pairs for quickly scanning a list.

This utility is private to the EAL and should allow avoiding having to move around the more complete librte_kvargs.

What is the big advantage with this code and the librte_kvargs code. Is it just no allocation, rte_kvargs needs to be build before parts of EAL or what?

Quoted 2 lines

My concern is we have now two flavors one in EAL and one in librte_kvargs, would it not be more reasonable to improve rte_kvargs to remove your objections? I am all for fast, better, stronger code :-)

+1, this really doesn't make much sense to me. Two parsing routines seems like its just asking for us to have to fix parsing bugs in two places. If allocation is a concern, I don't see why you can't just change the malloc in rte_kvargs_parse to an automatic allocation on the stack, or a preallocation set of kvargs that can be shared from init time.

librte_kvargs isn't necessecarily
the best parsing library ever, but its not bad, and it just seems wrong to go re-inventing the wheel.

Quoted 19 lines · Neil Horman

Quoted 18 lines · Keith

Quoted 7 lines · Gaetan Rivet

This library offers a quick way to parse parameters passed with a key=value syntax.

Quoted 6 lines

A single function is needed and finds the relevant element within the text. No dynamic allocation is performed. It is possible to chain the parsing of each pairs for quickly scanning a list.

This utility is private to the EAL and should allow avoiding having to move around the more complete librte_kvargs.

What is the big advantage with this code and the librte_kvargs code. Is it just no allocation, rte_kvargs needs to be build before parts of EAL or what?

My concern is we have now two flavors one in EAL and one in librte_kvargs, would it not be more reasonable to improve rte_kvargs to remove your objections? I am all for fast, better, stronger code :-)

Quoted 5 lines

+1, this really doesn't make much sense to me. Two parsing routines seems like its just asking for us to have to fix parsing bugs in two places. If allocation is a concern, I don't see why you can't just change the malloc in rte_kvargs_parse to an automatic allocation on the stack, or a preallocation set of kvargs that can be shared from init time.

I think the existing allocation scheme is fine for other usages (in drivers and so on). Not for what I wanted to do.

Quoted 4 lines

librte_kvargs isn't necessecarily
the best parsing library ever, but its not bad, and it just seems wrong to go re-inventing the wheel.

It serves a different purpose than the one I'm pursuing.

This helper is lightweight and private. If I wanted to integrate my needs with librte_kvargs, I would be adding new functionalities, making it more complex, and for a use-case that is useless for the vast majority of users of the lib.

If that's really an issue, I'm better off simply removing rte_parse_kv and writing the parsing by hand within my function. This would be ugly and tedious, but less than moving librte_kvargs within EAL and changing it to my needs.

Signature · 3 lines

-- 
Gaëtan Rivet
6WIND

Quoted 26 lines · Gaëtan Rivet

Quoted 24 lines · Neil Horman

Quoted 11 lines · Keith

Quoted 11 lines · Gaetan Rivet

This library offers a quick way to parse parameters passed with a key=value syntax.

A single function is needed and finds the relevant element within the text. No dynamic allocation is performed. It is possible to chain the parsing of each pairs for quickly scanning a list.

Quoted 7 lines

Quoted 2 lines

This utility is private to the EAL and should allow avoiding having to move around the more complete librte_kvargs.

What is the big advantage with this code and the librte_kvargs code. Is it just no allocation, rte_kvargs needs to be build before parts of EAL or what?

My concern is we have now two flavors one in EAL and one in librte_kvargs, would it not be more reasonable to improve rte_kvargs to remove your objections? I am all for fast, better, stronger code :-)

+1, this really doesn't make much sense to me. Two parsing routines seems like its just asking for us to have to fix parsing bugs in two places. If allocation is a concern, I don't see why you can't just change the malloc in rte_kvargs_parse to an automatic allocation on the stack, or a preallocation set of kvargs that can be shared from init time.

Quoted 3 lines

I think the existing allocation scheme is fine for other usages (in drivers and so on). Not for what I wanted to do.

Ok, but thats an adressable issue. you can bifurcate the parse function to an internal function that accepts any preallocated kvargs struct, and export two wrapper functions, one which allocates the struct from the heap, another which allocated automatically on the stack.

Quoted 5 lines

Quoted 4 lines

librte_kvargs isn't necessecarily
the best parsing library ever, but its not bad, and it just seems wrong to go re-inventing the wheel.

Quoted 7 lines

It serves a different purpose than the one I'm pursuing.

This helper is lightweight and private. If I wanted to integrate my needs with librte_kvargs, I would be adding new functionalities, making it more complex, and for a use-case that is useless for the vast majority of users of the lib.

Ok, to that end:

1) Privacy is not an issue (at least from my understanding of what your doing). If we start with the assumption that librte_kvargs is capable of satisfying your needs (even if its not done in an optimal way), the fact that your version of the function is internal to the library doesn't seem overly relevant, unless theres something critical to that privacy that I'm missing.

2) Lightweight function seems like something that can be integrated with librte_kvargs. Looking at it, what may I ask in librte_kvargs is insufficiently non-performant for your needs, specifically? We talked about the heap allocation above, is there something else? The string duplication perhaps?

Quoted 4 lines

If that's really an issue, I'm better off simply removing rte_parse_kv and writing the parsing by hand within my function. This would be ugly and tedious, but less than moving librte_kvargs within EAL and changing it to my needs.

I don't think thats necessecary, I just think if you can ennumerate the items that are non-performant for your needs we can make some changes to librte_kvargs to optimize around them, or offer parsing options to avoid them, and in the process avoid some code duplication

Quoted 30 lines · Neil Horman

Quoted 29 lines · Gaëtan Rivet

Quoted 17 lines · Neil Horman

Quoted 16 lines · Keith

Quoted 7 lines · Gaetan Rivet

This library offers a quick way to parse parameters passed with a key=value syntax.

Quoted 6 lines

A single function is needed and finds the relevant element within the text. No dynamic allocation is performed. It is possible to chain the parsing of each pairs for quickly scanning a list.

This utility is private to the EAL and should allow avoiding having to move around the more complete librte_kvargs.

What is the big advantage with this code and the librte_kvargs code. Is it just no allocation, rte_kvargs needs to be build before parts of EAL or what?

Quoted 7 lines

Quoted 2 lines

My concern is we have now two flavors one in EAL and one in librte_kvargs, would it not be more reasonable to improve rte_kvargs to remove your objections? I am all for fast, better, stronger code :-)

+1, this really doesn't make much sense to me. Two parsing routines seems like its just asking for us to have to fix parsing bugs in two places. If allocation is a concern, I don't see why you can't just change the malloc in rte_kvargs_parse to an automatic allocation on the stack, or a preallocation set of kvargs that can be shared from init time.

I think the existing allocation scheme is fine for other usages (in drivers and so on). Not for what I wanted to do.

Quoted 5 lines

Ok, but thats an adressable issue. you can bifurcate the parse function to an internal function that accepts any preallocated kvargs struct, and export two wrapper functions, one which allocates the struct from the heap, another which allocated automatically on the stack.

Sure, everything is possible.

Quoted 12 lines

Quoted 5 lines

Quoted 4 lines

librte_kvargs isn't necessecarily
the best parsing library ever, but its not bad, and it just seems wrong to go re-inventing the wheel.

Quoted 7 lines

It serves a different purpose than the one I'm pursuing.

This helper is lightweight and private. If I wanted to integrate my needs with librte_kvargs, I would be adding new functionalities, making it more complex, and for a use-case that is useless for the vast majority of users of the lib.

Quoted 8 lines

Ok, to that end:

1) Privacy is not an issue (at least from my understanding of what your doing). If we start with the assumption that librte_kvargs is capable of satisfying your needs (even if its not done in an optimal way), the fact that your version of the function is internal to the library doesn't seem overly relevant, unless theres something critical to that privacy that I'm missing.

Privacy is only a point I brought up to say that the impact of this function is minimal. People looking to parse their kvargs should not have any ambiguity regarding how they should do so. Only librte_kvargs is available.

Quoted 6 lines

2) Lightweight function seems like something that can be integrated with librte_kvargs. Looking at it, what may I ask in librte_kvargs is insufficiently non-performant for your needs, specifically? We talked about the heap allocation above, is there something else? The string duplication perhaps?

Mostly the way to use it.
The filter strings are
bus=value,.../class=value,...

where either bus= list or class= list can be omitted, but at least one must appear.

I want to read a single kvarg. I do not want to parse the whole string. the '/' signifies the end of the current layer.

librte_kvargs does not care about those points. I cannot ask it to only read either bus or class, as it would then throw an error for all the other keys (which the EAL has necessarily no knowledge of).

So I would need to:

* Add a custom storage scheme
* Add a custom parsing mode stopping at the first kvarg
* Add an edge-case to ignore the '/', so as not to throw off the rest of the parsing (least it be considered part of the previous kvarg value field).

Seeing this, does adding those really specifics functionality help librte_kvargs to be more useful and usable? I do not think so.

It would only serve to disrupt the library for a marginal use-case, with the introduction of edge-cases that will blur the specs of the lib's API, making it harder to avoid subtle bugs.

Only way to do so sanely would be to add rte_parse_kv as part of librte_kvargs, as is. But then the whole thing does not make sense IMO:
no one would care to use it, the maintainance effort is the same, the likelyhood of bugs as well (but in the process we would disrupt the distribution of librte_kvargs by moving it within the EAL).

I see no benefit to either solution.

Quoted 9 lines

Quoted 4 lines

If that's really an issue, I'm better off simply removing rte_parse_kv and writing the parsing by hand within my function. This would be ugly and tedious, but less than moving librte_kvargs within EAL and changing it to my needs.

I don't think thats necessecary, I just think if you can ennumerate the items that are non-performant for your needs we can make some changes to librte_kvargs to optimize around them, or offer parsing options to avoid them, and in the process avoid some code duplication

I think it makes sense to have specialized functions for specialized use-cases, and forcing the code to be generic and work with all of them will make it more complicated.

The genericity would only be worth it if people actually needed to parse the device strings the same way I do. No one has any business doing so. This genericity adds complexity and issues, without even being useful in the first place.

Signature · 2 lines

Gaëtan Rivet
6WIND

Quoted 31 lines · Gaëtan Rivet

Quoted 30 lines · Neil Horman

Quoted 29 lines · Gaëtan Rivet

Quoted 17 lines · Neil Horman

Quoted 16 lines · Keith

Quoted 7 lines · Gaetan Rivet

This library offers a quick way to parse parameters passed with a key=value syntax.

Quoted 6 lines

A single function is needed and finds the relevant element within the text. No dynamic allocation is performed. It is possible to chain the parsing of each pairs for quickly scanning a list.

This utility is private to the EAL and should allow avoiding having to move around the more complete librte_kvargs.

What is the big advantage with this code and the librte_kvargs code. Is it just no allocation, rte_kvargs needs to be build before parts of EAL or what?

Quoted 7 lines

Quoted 2 lines

My concern is we have now two flavors one in EAL and one in librte_kvargs, would it not be more reasonable to improve rte_kvargs to remove your objections? I am all for fast, better, stronger code :-)

+1, this really doesn't make much sense to me. Two parsing routines seems like its just asking for us to have to fix parsing bugs in two places. If allocation is a concern, I don't see why you can't just change the malloc in rte_kvargs_parse to an automatic allocation on the stack, or a preallocation set of kvargs that can be shared from init time.

I think the existing allocation scheme is fine for other usages (in drivers and so on). Not for what I wanted to do.

Quoted 8 lines

Quoted 5 lines

Ok, but thats an adressable issue. you can bifurcate the parse function to an internal function that accepts any preallocated kvargs struct, and export two wrapper functions, one which allocates the struct from the heap, another which allocated automatically on the stack.

Sure, everything is possible.

Ok.

Quoted 21 lines

Quoted 12 lines

Quoted 5 lines

Quoted 4 lines

librte_kvargs isn't necessecarily
the best parsing library ever, but its not bad, and it just seems wrong to go re-inventing the wheel.

Quoted 7 lines

It serves a different purpose than the one I'm pursuing.

This helper is lightweight and private. If I wanted to integrate my needs with librte_kvargs, I would be adding new functionalities, making it more complex, and for a use-case that is useless for the vast majority of users of the lib.

Quoted 8 lines

Ok, to that end:

1) Privacy is not an issue (at least from my understanding of what your doing). If we start with the assumption that librte_kvargs is capable of satisfying your needs (even if its not done in an optimal way), the fact that your version of the function is internal to the library doesn't seem overly relevant, unless theres something critical to that privacy that I'm missing.

Quoted 5 lines

Privacy is only a point I brought up to say that the impact of this function is minimal. People looking to parse their kvargs should not have any ambiguity regarding how they should do so. Only librte_kvargs is available.

Ok, would you also council others developing dpdk apps to write their own parsing routines when what they needed was trivial for the existing library? You are people too :)

Quoted 7 lines

Quoted 6 lines

2) Lightweight function seems like something that can be integrated with librte_kvargs. Looking at it, what may I ask in librte_kvargs is insufficiently non-performant for your needs, specifically? We talked about the heap allocation above, is there something else? The string duplication perhaps?

Quoted 7 lines

Mostly the way to use it.
The filter strings are
bus=value,.../class=value,...

where either bus= list or class= list can be omitted, but at least one must appear.

Ok, so whats the problem with using librte_kvargs for that? Is it that the list that acts as the value to the key isn't parsed out into its own set of tokens? That seems entirely addressable.

Quoted 3 lines

I want to read a single kvarg. I do not want to parse the whole string. the '/' signifies the end of the current layer.

This makes it seem like librte_kvargs can handle this as a trivial case of its functionality.

Quoted 4 lines

librte_kvargs does not care about those points. I cannot ask it to only read either bus or class, as it would then throw an error for all the other keys (which the EAL has necessarily no knowledge of).

But you can ask it to read both, and within your libraries logic make the determination as to the validitiy of receiving both. Alternatively you can modify the valid_keys check in kvargs to be a regex that matches on either bus or class, or accept an ignore parameter for keys that may appear but should be ignored in the light of other keys. Theres lots of options here.

Quoted 8 lines

So I would need to:

* Add a custom storage scheme
* Add a custom parsing mode stopping at the first kvarg
* Add an edge-case to ignore the '/', so as not to throw off the rest of the parsing (least it be considered part of the previous kvarg value field).

Quoted 3 lines

Seeing this, does adding those really specifics functionality help librte_kvargs to be more useful and usable? I do not think so.

I think you're overcomplicating this.
How about enhancing librte_kvargs to make parsing configurable such that invalid keys get ignored rather than generate errors?

Quoted 4 lines

It would only serve to disrupt the library for a marginal use-case, with the introduction of edge-cases that will blur the specs of the lib's API, making it harder to avoid subtle bugs.

What do you mean "disrupt the library"? What is its purpose of a library if not to do the jobs we want it to? If everyone created their own routine to do something that a library could do with some modifications, we'd be left with 1000 versions of the same routine. If the existing library does 99% of what you want it to, lets ennumerate what the missing %1 is and make the change, not throw the baby out with the bathwater.

Quoted 8 lines

Only way to do so sanely would be to add rte_parse_kv as part of librte_kvargs, as is. But then the whole thing does not make sense IMO:
no one would care to use it, the maintainance effort is the same, the likelyhood of bugs as well (but in the process we would disrupt the distribution of librte_kvargs by moving it within the EAL).

I see no benefit to either solution.

Again, thats an overcomplication. As I read it, you have a need to interrogate a key/value string, whos contents may contain invalid keys (for your parsing purposes), and whos values may themselves be lists, correct? If so, I don't see the problem in enhancing libkvargs to:

1) Allow for the parsing routine to ignore invalid keys (or even ignore specific invalid keys, and trigger on any unknown keys)

2) Allows for the subparsing of lists into their own set of tokens.

Quoted 10 lines

Quoted 9 lines

Quoted 4 lines

If that's really an issue, I'm better off simply removing rte_parse_kv and writing the parsing by hand within my function. This would be ugly and tedious, but less than moving librte_kvargs within EAL and changing it to my needs.

I don't think thats necessecary, I just think if you can ennumerate the items that are non-performant for your needs we can make some changes to librte_kvargs to optimize around them, or offer parsing options to avoid them, and in the process avoid some code duplication

Quoted 4 lines

I think it makes sense to have specialized functions for specialized use-cases, and forcing the code to be generic and work with all of them will make it more complicated.

This isn't specialized, its trivial. Its just a trivial case that libkvargs isn't built to handle at the moment. Lets get it there.

Quoted 5 lines

The genericity would only be worth it if people actually needed to parse the device strings the same way I do. No one has any business doing so. This genericity adds complexity and issues, without even being useful in the first place.

I really think you're trying to take a short cut here where none is needed, and I'm sorry, but I can't support that.

Signed-off-by Gaetan Rivet <[email protected]>

---

Cc Neil Horman <[email protected]>

I find using rte_parse_kv cleaner.
The function rte_dev_iterator_init is already ugly enough as it is. This is really not helping.

 lib/librte_eal/common/eal_common_dev.c | 127 +++++++++++++++++++++------------
 lib/librte_eal/linuxapp/eal/Makefile   |   1 +
 2 files changed, 83 insertions(+), 45 deletions(-)

lib/librte_eal/common/eal_common_dev.c +82 -45

index 21703b777..9f1a0ebda 100644
@@ -15,6 +15,7 @@
 #include <rte_devargs.h>
 #include <rte_debug.h>
 #include <rte_errno.h>
+#include <rte_kvargs.h>
 #include <rte_log.h>
 
 #include "eal_private.h"

@@ -270,12 +271,15 @@ rte_eal_hotplug_remove(const char *busname, const char *devname)
 }
 
 int __rte_experimental
-rte_dev_iterator_init(struct rte_dev_iterator *it, const char *str)
+rte_dev_iterator_init(struct rte_dev_iterator *it,
+		      const char *devstr)
 {
-	struct rte_bus *bus = NULL;
+	struct rte_kvargs *kvlist = NULL;
 	struct rte_class *cls = NULL;
-	struct rte_kvarg kv;
-	char *slash;
+	struct rte_bus *bus = NULL;
+	struct rte_kvargs_pair *kv;
+	char *slash = NULL;
+	char *str = NULL;
 
 	/* Having both busstr and clsstr NULL is illegal,
 	 * marking this iterator as invalid unless

@@ -283,98 +287,131 @@ rte_dev_iterator_init(struct rte_dev_iterator *it, const char *str)
 	 */
 	it->busstr = NULL;
 	it->clsstr = NULL;
+	str = strdup(devstr);
+	if (str == NULL) {
+		rte_errno = ENOMEM;
+		goto get_out;
+	}
+	slash = strchr(str, '/');
+	if (slash != NULL) {
+		slash[0] = '\0';
+		slash = strchr(devstr, '/') + 1;
+	}
 	/* Safety checks and prep-work */
-	if (rte_parse_kv(str, &kv)) {
+	kvlist = rte_kvargs_parse(str, NULL);
+	if (kvlist == NULL) {
 		RTE_LOG(ERR, EAL, "Could not parse: %s\n", str);
 		rte_errno = EINVAL;
-		return -rte_errno;
+		goto get_out;
 	}
 	it->device = NULL;
 	it->class_device = NULL;
-	if (strcmp(kv.key, "bus") == 0) {
-		bus = rte_bus_find_by_name(kv.value);
+	kv = &kvlist->pairs[0];
+	if (strcmp(kv->key, "bus") == 0) {
+		bus = rte_bus_find_by_name(kv->value);
 		if (bus == NULL) {
 			RTE_LOG(ERR, EAL, "Could not find bus \"%s\"\n",
-				kv.value);
+				kv->value);
 			rte_errno = EFAULT;
-			return -rte_errno;
+			goto get_out;
 		}
-		slash = strchr(str, '/');
 		if (slash != NULL) {
-			if (rte_parse_kv(slash + 1, &kv)) {
+			rte_kvargs_free(kvlist);
+			kvlist = rte_kvargs_parse(slash, NULL);
+			if (kvlist == NULL) {
 				RTE_LOG(ERR, EAL, "Could not parse: %s\n",
-					slash + 1);
+					slash);
 				rte_errno = EINVAL;
-				return -rte_errno;
+				goto get_out;
 			}
-			cls = rte_class_find_by_name(kv.value);
+			kv = &kvlist->pairs[0];
+			if (strcmp(kv->key, "class")) {
+				RTE_LOG(ERR, EAL, "Additional layer must be a class\n");
+				rte_errno = EINVAL;
+				goto get_out;
+			}
+			cls = rte_class_find_by_name(kv->value);
 			if (cls == NULL) {
 				RTE_LOG(ERR, EAL, "Could not find class \"%s\"\n",
-					kv.value);
+					kv->value);
 				rte_errno = EFAULT;
-				return -rte_errno;
+				goto get_out;
 			}
 		}
-	} else if (strcmp(kv.key, "class") == 0) {
-		cls = rte_class_find_by_name(kv.value);
+	} else if (strcmp(kv->key, "class") == 0) {
+		cls = rte_class_find_by_name(kv->value);
 		if (cls == NULL) {
 			RTE_LOG(ERR, EAL, "Could not find class \"%s\"\n",
-				kv.value);
+				kv->value);
 			rte_errno = EFAULT;
-			return -rte_errno;
+			goto get_out;
 		}
 	} else {
 		rte_errno = EINVAL;
-		return -rte_errno;
+		goto get_out;
 	}
 	/* The string should have at least
 	 * one layer specified.
 	 */
 	if (bus == NULL && cls == NULL) {
 		rte_errno = EINVAL;
-		return -rte_errno;
+		goto get_out;
 	}
 	if ((bus != NULL && bus->dev_iterate == NULL) ||
 	    (cls != NULL && cls->dev_iterate == NULL)) {
 		rte_errno = ENOTSUP;
-		return -rte_errno;
+		goto get_out;
 	}
 	if (bus != NULL) {
-		it->busstr = str;
+		it->busstr = devstr;
 		if (cls != NULL)
-			it->clsstr = slash + 1;
+			it->clsstr = slash;
 	} else if (cls != NULL) {
-		it->clsstr = str;
+		it->clsstr = devstr;
 	}
-	it->devstr = str;
+	it->devstr = devstr;
 	it->bus = bus;
 	it->cls = cls;
-	return 0;
+get_out:
+	rte_kvargs_free(kvlist);
+	free(str);
+	return -rte_errno;
+}
+
+/* '\0' forbidden in sym */
+static const char *
+strfirstof(const char *str,
+	   const char *sym)
+{
+	const char *s;
+
+	for (s = str; s[0] != '\0'; s++) {
+		const char *c;
+
+		for (c = sym; c[0] != '\0'; c++) {
+			if (c[0] == s[0])
+				return s;
+		}
+	}
+	return NULL;
 }
 
 static char *
 dev_str_sane_cpy(const char *str)
 {
-	struct rte_kvarg kv;
-	char *end;
+	const char *end;
 	char *cpy;
 
-	if (rte_parse_kv(str, &kv)) {
-		rte_errno = EINVAL;
-		return NULL;
-	}
-	/* copying '\0' is valid. */
-	if (kv.next != NULL)
-		cpy = strdup(kv.next);
-	else
+	end = strfirstof(str, ",/");
+	if (end != NULL &&
+	    end[0] == ',') {
+		cpy = strdup(end + 1);
+	} else {
+		/* '/' or '\0' */
 		cpy = strdup("");
-	if (cpy == NULL) {
-		rte_errno = ENOMEM;
-		return NULL;
 	}
-	end = strchr(cpy, '/');
-	if (end != NULL)
-		end[0] = '\0';
+	if (cpy == NULL)
+		rte_errno = ENOMEM;
 	return cpy;
 }
 

lib/librte_eal/linuxapp/eal/Makefile +1 -0

index a3edbbe76..87caa23a1 100644
@@ -27,6 +27,7 @@ LDLIBS += -lrt
 ifeq ($(CONFIG_RTE_EAL_NUMA_AWARE_HUGEPAGES),y)
 LDLIBS += -lnuma
 endif
+LDLIBS += -lrte_kvargs
 
 # specific to linuxapp exec-env
 SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) := eal.c

Signature · 1 line

2.11.0

Quoted 5 lines · Gaetan Rivet

Signed-off-by Gaetan Rivet <[email protected]>

---

Cc Neil Horman <[email protected]>

I'm actually ok with this but as Keith noted, I'm not sure why you didn't just:

1) Add the ability to create a grouping key, so that key value pairs could contain a list of comma separated values (something like '{}' to denote that everything between the characters was the value in a kv pair, regardless of other tokenizing characters in the value).

2) Add the ability to recursively parse the value into a list of tokens

3) Layer your functionality on top of (1) and (2), as Keith noted

Hi Neil,

Quoted 6 lines · Neil Horman

Quoted 5 lines · Gaetan Rivet

Signed-off-by Gaetan Rivet <[email protected]>

---

Cc Neil Horman <[email protected]>

Keep in mind that all of this is to achieve the trivial task I was doing in 20 lines or so.

Quoted 9 lines

I'm actually ok with this but as Keith noted, I'm not sure why you didn't just:

1) Add the ability to create a grouping key, so that key value pairs could contain a list of comma separated values (something like '{}' to denote that everything between the characters was the value in a kv pair, regardless of other tokenizing characters in the value).

2) Add the ability to recursively parse the value into a list of tokens

I don't need a recursive construct or a tree-like structure. I only need an alternative to '\0' to signify "end-of-list".
This seems like an edge-case to librte_kvargs that would only be useful to a specific case. It does not seem a wise addition.

So maybe I did not understand your suggestion. Can you give an example of inputs?

I need to parse something like

"bus=pci,vendor_id=0x8086/class=eth"
(and I only care about bus=pci and class=eth).

how can grouping help? My issue is that librte_kvargs would parse

key:vendor_id
value:0x8086/class

and would then stumble on the unexpected '=‘.

Quoted 2 lines

3) Layer your functionality on top of (1) and (2), as Keith noted

The stack allocator seems like a nice-to-have that would interest people using librte_kvargs. I find librte_kvargs to be cumbersome. I cannot rewrite it from scratch, unless I update everything that relies on it as well. So I do not touch it, because I don't care *that* much.

Why not simply leave my helper alongside? If people care enough about it and would prefer to use it over librte_kvargs, then maybe we could think about doing the effort of exposing it cleanly (or maybe they could). Right now, I see only me needing it and I do not see this effort as worth it.

Regards,

Signature · 2 lines

Gaëtan Rivet
6WIND

Quoted 100 lines · Neil Horman

Quoted 31 lines · Gaëtan Rivet

Quoted 30 lines · Neil Horman

Quoted 29 lines · Gaëtan Rivet

Quoted 17 lines · Neil Horman

Quoted 16 lines · Keith

Quoted 7 lines · Gaetan Rivet

This library offers a quick way to parse parameters passed with a key=value syntax.

Quoted 6 lines

A single function is needed and finds the relevant element within the text. No dynamic allocation is performed. It is possible to chain the parsing of each pairs for quickly scanning a list.

This utility is private to the EAL and should allow avoiding having to move around the more complete librte_kvargs.

What is the big advantage with this code and the librte_kvargs code. Is it just no allocation, rte_kvargs needs to be build before parts of EAL or what?

Quoted 7 lines

Quoted 2 lines

My concern is we have now two flavors one in EAL and one in librte_kvargs, would it not be more reasonable to improve rte_kvargs to remove your objections? I am all for fast, better, stronger code :-)

+1, this really doesn't make much sense to me. Two parsing routines seems like its just asking for us to have to fix parsing bugs in two places. If allocation is a concern, I don't see why you can't just change the malloc in rte_kvargs_parse to an automatic allocation on the stack, or a preallocation set of kvargs that can be shared from init time.

I think the existing allocation scheme is fine for other usages (in drivers and so on). Not for what I wanted to do.

Quoted 8 lines

Quoted 5 lines

Ok, but thats an adressable issue. you can bifurcate the parse function to an internal function that accepts any preallocated kvargs struct, and export two wrapper functions, one which allocates the struct from the heap, another which allocated automatically on the stack.

Sure, everything is possible.

Ok.

Quoted 21 lines

Quoted 12 lines

Quoted 5 lines

Quoted 4 lines

librte_kvargs isn't necessecarily
the best parsing library ever, but its not bad, and it just seems wrong to go re-inventing the wheel.

Quoted 7 lines

It serves a different purpose than the one I'm pursuing.

This helper is lightweight and private. If I wanted to integrate my needs with librte_kvargs, I would be adding new functionalities, making it more complex, and for a use-case that is useless for the vast majority of users of the lib.

Quoted 8 lines

Ok, to that end:

1) Privacy is not an issue (at least from my understanding of what your doing). If we start with the assumption that librte_kvargs is capable of satisfying your needs (even if its not done in an optimal way), the fact that your version of the function is internal to the library doesn't seem overly relevant, unless theres something critical to that privacy that I'm missing.

Quoted 5 lines

Privacy is only a point I brought up to say that the impact of this function is minimal. People looking to parse their kvargs should not have any ambiguity regarding how they should do so. Only librte_kvargs is available.

Ok, would you also council others developing dpdk apps to write their own parsing routines when what they needed was trivial for the existing library? You are people too :)

Quoted 7 lines

Quoted 6 lines

2) Lightweight function seems like something that can be integrated with librte_kvargs. Looking at it, what may I ask in librte_kvargs is insufficiently non-performant for your needs, specifically? We talked about the heap allocation above, is there something else? The string duplication perhaps?

Quoted 7 lines

Mostly the way to use it.
The filter strings are
bus=value,.../class=value,...

where either bus= list or class= list can be omitted, but at least one must appear.

Ok, so whats the problem with using librte_kvargs for that? Is it that the list that acts as the value to the key isn't parsed out into its own set of tokens? That seems entirely addressable.

Quoted 3 lines

I want to read a single kvarg. I do not want to parse the whole string. the '/' signifies the end of the current layer.

This makes it seem like librte_kvargs can handle this as a trivial case of its functionality.

Quoted 4 lines

librte_kvargs does not care about those points. I cannot ask it to only read either bus or class, as it would then throw an error for all the other keys (which the EAL has necessarily no knowledge of).

Quoted 6 lines

But you can ask it to read both, and within your libraries logic make the determination as to the validitiy of receiving both. Alternatively you can modify the valid_keys check in kvargs to be a regex that matches on either bus or class, or accept an ignore parameter for keys that may appear but should be ignored in the light of other keys. Theres lots of options here.

No, I will not be adding regex parsing to express a set of acceptable token :)

Quoted 11 lines

Quoted 11 lines

So I would need to:

* Add a custom storage scheme
* Add a custom parsing mode stopping at the first kvarg
* Add an edge-case to ignore the '/', so as not to throw off the rest of the parsing (least it be considered part of the previous kvarg value field).

Seeing this, does adding those really specifics functionality help librte_kvargs to be more useful and usable? I do not think so.

Quoted 4 lines

I think you're overcomplicating this.
How about enhancing librte_kvargs to make parsing configurable such that invalid keys get ignored rather than generate errors?

Invalid keys can already be ignored, it's not an issue.

Quoted 24 lines

Quoted 4 lines

It would only serve to disrupt the library for a marginal use-case, with the introduction of edge-cases that will blur the specs of the lib's API, making it harder to avoid subtle bugs.

What do you mean "disrupt the library"? What is its purpose of a library if not to do the jobs we want it to? If everyone created their own routine to do something that a library could do with some modifications, we'd be left with 1000 versions of the same routine. If the existing library does 99% of what you want it to, lets ennumerate what the missing %1 is and make the change, not throw the baby out with the bathwater.

Quoted 8 lines

Only way to do so sanely would be to add rte_parse_kv as part of librte_kvargs, as is. But then the whole thing does not make sense IMO:
no one would care to use it, the maintainance effort is the same, the likelyhood of bugs as well (but in the process we would disrupt the distribution of librte_kvargs by moving it within the EAL).

I see no benefit to either solution.

Again, thats an overcomplication. As I read it, you have a need to interrogate a key/value string, whos contents may contain invalid keys (for your parsing purposes), and whos values may themselves be lists, correct? If so, I don't see the problem in enhancing libkvargs to:

Quoted 5 lines

1) Allow for the parsing routine to ignore invalid keys (or even ignore specific invalid keys, and trigger on any unknown keys)

2) Allows for the subparsing of lists into their own set of tokens.

What I would do if I wanted to use librte_kvargs, would be to duplicate the input text, mangle it to respect the intended format, and feed it to librte_kvargs for parsing. Then I would iterate over the pairs, and stop on the two I'm concerned about.

What I dislike here:

* I actually do the bulk of the parsing by hand (recognizing first a valid input, modifying it to respect the lib, and after iterating on the list of pairs and strcmp-ing the ones I want). This is approximately as complicated as the current helper function.

* I have to move librte_kvargs in the EAL.

Quoted 14 lines

Quoted 14 lines

Quoted 9 lines

Quoted 4 lines

If that's really an issue, I'm better off simply removing rte_parse_kv and writing the parsing by hand within my function. This would be ugly and tedious, but less than moving librte_kvargs within EAL and changing it to my needs.

I don't think thats necessecary, I just think if you can ennumerate the items that are non-performant for your needs we can make some changes to librte_kvargs to optimize around them, or offer parsing options to avoid them, and in the process avoid some code duplication

I think it makes sense to have specialized functions for specialized use-cases, and forcing the code to be generic and work with all of them will make it more complicated.

Quoted 3 lines

This isn't specialized, its trivial. Its just a trivial case that libkvargs isn't built to handle at the moment. Lets get it there.

My use-case is trivial. Putting it within the librte_kvargs makes it more complicated to write. I hate this.

Quoted 7 lines

Quoted 5 lines

The genericity would only be worth it if people actually needed to parse the device strings the same way I do. No one has any business doing so. This genericity adds complexity and issues, without even being useful in the first place.

I really think you're trying to take a short cut here where none is needed, and I'm sorry, but I can't support that.

There are countably infinite solutions, we will probably reach one (and it might even do what we want).

Quoted 27 lines · Neil Horman

Quoted 26 lines · Gaëtan Rivet

Quoted 24 lines · Neil Horman

Quoted 11 lines · Keith

Quoted 11 lines · Gaetan Rivet

This library offers a quick way to parse parameters passed with a key=value syntax.

A single function is needed and finds the relevant element within the text. No dynamic allocation is performed. It is possible to chain the parsing of each pairs for quickly scanning a list.

Quoted 7 lines

Quoted 2 lines

This utility is private to the EAL and should allow avoiding having to move around the more complete librte_kvargs.

What is the big advantage with this code and the librte_kvargs code. Is it just no allocation, rte_kvargs needs to be build before parts of EAL or what?

My concern is we have now two flavors one in EAL and one in librte_kvargs, would it not be more reasonable to improve rte_kvargs to remove your objections? I am all for fast, better, stronger code :-)

+1, this really doesn't make much sense to me. Two parsing routines seems like its just asking for us to have to fix parsing bugs in two places. If allocation is a concern, I don't see why you can't just change the malloc in rte_kvargs_parse to an automatic allocation on the stack, or a preallocation set of kvargs that can be shared from init time.

Quoted 8 lines

Quoted 3 lines

I think the existing allocation scheme is fine for other usages (in drivers and so on). Not for what I wanted to do.

Ok, but thats an adressable issue. you can bifurcate the parse function to an internal function that accepts any preallocated kvargs struct, and export two wrapper functions, one which allocates the struct from the heap, another which allocated automatically on the stack.

Sure, everything is possible.

Quoted 12 lines

Quoted 5 lines

Quoted 4 lines

librte_kvargs isn't necessecarily
the best parsing library ever, but its not bad, and it just seems wrong to go re-inventing the wheel.

Quoted 7 lines

It serves a different purpose than the one I'm pursuing.

This helper is lightweight and private. If I wanted to integrate my needs with librte_kvargs, I would be adding new functionalities, making it more complex, and for a use-case that is useless for the vast majority of users of the lib.

Quoted 8 lines

Ok, to that end:

1) Privacy is not an issue (at least from my understanding of what your doing). If we start with the assumption that librte_kvargs is capable of satisfying your needs (even if its not done in an optimal way), the fact that your version of the function is internal to the library doesn't seem overly relevant, unless theres something critical to that privacy that I'm missing.

Privacy is only a point I brought up to say that the impact of this function is minimal. People looking to parse their kvargs should not have any ambiguity regarding how they should do so. Only librte_kvargs is available.

Quoted 6 lines

2) Lightweight function seems like something that can be integrated with librte_kvargs. Looking at it, what may I ask in librte_kvargs is insufficiently non-performant for your needs, specifically? We talked about the heap allocation above, is there something else? The string duplication perhaps?

Mostly the way to use it.
The filter strings are
bus=value,.../class=value,...

where either bus= list or class= list can be omitted, but at least one must appear.

I want to read a single kvarg. I do not want to parse the whole string. the '/' signifies the end of the current layer.

librte_kvargs does not care about those points. I cannot ask it to only read either bus or class, as it would then throw an error for all the other keys (which the EAL has necessarily no knowledge of).

So I would need to:

* Add a custom storage scheme
* Add a custom parsing mode stopping at the first kvarg
* Add an edge-case to ignore the '/', so as not to throw off the rest of the parsing (least it be considered part of the previous kvarg value field).

Seeing this, does adding those really specifics functionality help librte_kvargs to be more useful and usable? I do not think so.

It would only serve to disrupt the library for a marginal use-case, with the introduction of edge-cases that will blur the specs of the lib's API, making it harder to avoid subtle bugs.

Only way to do so sanely would be to add rte_parse_kv as part of librte_kvargs, as is. But then the whole thing does not make sense IMO:
no one would care to use it, the maintainance effort is the same, the likelyhood of bugs as well (but in the process we would disrupt the distribution of librte_kvargs by moving it within the EAL).

I see no benefit to either solution.

Quoted 9 lines

Quoted 4 lines

If that's really an issue, I'm better off simply removing rte_parse_kv and writing the parsing by hand within my function. This would be ugly and tedious, but less than moving librte_kvargs within EAL and changing it to my needs.

I don't think thats necessecary, I just think if you can ennumerate the items that are non-performant for your needs we can make some changes to librte_kvargs to optimize around them, or offer parsing options to avoid them, and in the process avoid some code duplication

I think it makes sense to have specialized functions for specialized use-cases, and forcing the code to be generic and work with all of them will make it more complicated.

The genericity would only be worth it if people actually needed to parse the device strings the same way I do. No one has any business doing so. This genericity adds complexity and issues, without even being useful in the first place.

Signature · 2 lines

Gaëtan Rivet
6WIND