[v4,2/6] ethdev: add iterator to match devargs input

Message ID 20181009223338.18307-3-thomas@monjalon.net
State Superseded, archived
Delegated to: Ferruh Yigit
Headers show
Series
  • replace attach/detach functions
Related show

Checks

Context Check Description
ci/Intel-compilation fail Compilation issues
ci/checkpatch success coding style OK

Commit Message

Thomas Monjalon Oct. 9, 2018, 10:33 p.m.
The iterator will return the ethdev port ids matching a devargs string.
It is recommended to use the macro RTE_ETH_FOREACH_MATCHING_DEV()
for usage convenience.

The class string is prefixed with '+' in order to skip the validation
of the parameter keys. It is tolerated for the compatibility with
the old (current) syntax where all parameters (bus, class and driver)
are mixed in the same string without any delimiter.
Thanks to this compatibility prefix, the driver parameters will be
skipped during the ethdev parsing, and not considered invalid.

A macro is introduced in rte_common.h to workaround a const field.
This hack is needed to free const strings in the iterator.
It is preferred to keep the const for these fields, because it gives
a hint that they are not changed at each iteration.

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>
---
 lib/librte_eal/common/include/rte_common.h |   6 +
 lib/librte_ethdev/ethdev_private.c         |  10 +-
 lib/librte_ethdev/ethdev_private.h         |   6 +
 lib/librte_ethdev/rte_class_eth.c          |   7 +-
 lib/librte_ethdev/rte_ethdev.c             | 123 +++++++++++++++++++++
 lib/librte_ethdev/rte_ethdev.h             |  79 +++++++++++++
 lib/librte_ethdev/rte_ethdev_version.map   |   3 +
 7 files changed, 232 insertions(+), 2 deletions(-)

Comments

Ferruh Yigit Oct. 16, 2018, 10:58 a.m. | #1
On 10/9/2018 11:33 PM, Thomas Monjalon wrote:
> The iterator will return the ethdev port ids matching a devargs string.
> It is recommended to use the macro RTE_ETH_FOREACH_MATCHING_DEV()
> for usage convenience.
> 
> The class string is prefixed with '+' in order to skip the validation
> of the parameter keys. It is tolerated for the compatibility with
> the old (current) syntax where all parameters (bus, class and driver)
> are mixed in the same string without any delimiter.
> Thanks to this compatibility prefix, the driver parameters will be
> skipped during the ethdev parsing, and not considered invalid.
> 
> A macro is introduced in rte_common.h to workaround a const field.
> This hack is needed to free const strings in the iterator.
> It is preferred to keep the const for these fields, because it gives
> a hint that they are not changed at each iteration.
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>

<...>

> @@ -237,6 +237,9 @@ EXPERIMENTAL {
>  	rte_eth_dev_owner_unset;
>  	rte_eth_dev_rx_offload_name;
>  	rte_eth_dev_tx_offload_name;
> +	rte_eth_iterator_cleanup;
> +	rte_eth_iterator_init;
> +	rte_eth_iterator_next;

It would be great to add some unit test for these APIs, it would both show how
they are used and verify this functionality with all that "+" trick etc..

Similar to previous patch, vdev iterator.

Is it too late to add some unit test for these functionality?
Thomas Monjalon Oct. 16, 2018, 12:06 p.m. | #2
16/10/2018 12:58, Ferruh Yigit:
> On 10/9/2018 11:33 PM, Thomas Monjalon wrote:
> > The iterator will return the ethdev port ids matching a devargs string.
> > It is recommended to use the macro RTE_ETH_FOREACH_MATCHING_DEV()
> > for usage convenience.
> > 
> > The class string is prefixed with '+' in order to skip the validation
> > of the parameter keys. It is tolerated for the compatibility with
> > the old (current) syntax where all parameters (bus, class and driver)
> > are mixed in the same string without any delimiter.
> > Thanks to this compatibility prefix, the driver parameters will be
> > skipped during the ethdev parsing, and not considered invalid.
> > 
> > A macro is introduced in rte_common.h to workaround a const field.
> > This hack is needed to free const strings in the iterator.
> > It is preferred to keep the const for these fields, because it gives
> > a hint that they are not changed at each iteration.
> > 
> > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> > Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>
> 
> <...>
> > +	rte_eth_iterator_cleanup;
> > +	rte_eth_iterator_init;
> > +	rte_eth_iterator_next;
> 
> It would be great to add some unit test for these APIs, it would both show how
> they are used and verify this functionality with all that "+" trick etc..
> 
> Similar to previous patch, vdev iterator.
> 
> Is it too late to add some unit test for these functionality?

We do not have similar ethdev unit test yet.
So for -rc1, yes it is too late to introduce a new class of unit test.
I will try to work on it for -rc2.

Patch

diff --git a/lib/librte_eal/common/include/rte_common.h b/lib/librte_eal/common/include/rte_common.h
index 069c13ec7..e3c0407a9 100644
--- a/lib/librte_eal/common/include/rte_common.h
+++ b/lib/librte_eal/common/include/rte_common.h
@@ -164,6 +164,12 @@  static void __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
  */
 #define RTE_PTR_DIFF(ptr1, ptr2) ((uintptr_t)(ptr1) - (uintptr_t)(ptr2))
 
+/**
+ * Workaround to cast a const field of a structure to non-const type.
+ */
+#define RTE_CAST_FIELD(var, field, type) \
+	(*(type *)((uintptr_t)(var) + offsetof(typeof(*(var)), field)))
+
 /*********** Macros/static functions for doing alignment ********/
 
 
diff --git a/lib/librte_ethdev/ethdev_private.c b/lib/librte_ethdev/ethdev_private.c
index 768c8b2ed..acc787dba 100644
--- a/lib/librte_ethdev/ethdev_private.c
+++ b/lib/librte_ethdev/ethdev_private.c
@@ -5,6 +5,14 @@ 
 #include "rte_ethdev.h"
 #include "ethdev_private.h"
 
+uint16_t
+eth_dev_to_id(const struct rte_eth_dev *dev)
+{
+	if (dev == NULL)
+		return RTE_MAX_ETHPORTS;
+	return dev - rte_eth_devices;
+}
+
 struct rte_eth_dev *
 eth_find_device(const struct rte_eth_dev *start, rte_eth_cmp_t cmp,
 		const void *data)
@@ -18,7 +26,7 @@  eth_find_device(const struct rte_eth_dev *start, rte_eth_cmp_t cmp,
 	     start > &rte_eth_devices[RTE_MAX_ETHPORTS]))
 		return NULL;
 	if (start != NULL)
-		idx = start - &rte_eth_devices[0] + 1;
+		idx = eth_dev_to_id(start) + 1;
 	else
 		idx = 0;
 	for (; idx < RTE_MAX_ETHPORTS; idx++) {
diff --git a/lib/librte_ethdev/ethdev_private.h b/lib/librte_ethdev/ethdev_private.h
index 0f5c6d5c4..e67cf6831 100644
--- a/lib/librte_ethdev/ethdev_private.h
+++ b/lib/librte_ethdev/ethdev_private.h
@@ -11,6 +11,12 @@ 
 extern "C" {
 #endif
 
+/*
+ * Convert rte_eth_dev pointer to port id.
+ * NULL will be translated to RTE_MAX_ETHPORTS.
+ */
+uint16_t eth_dev_to_id(const struct rte_eth_dev *dev);
+
 /* Generic rte_eth_dev comparison function. */
 typedef int (*rte_eth_cmp_t)(const struct rte_eth_dev *, const void *);
 
diff --git a/lib/librte_ethdev/rte_class_eth.c b/lib/librte_ethdev/rte_class_eth.c
index 84b646291..c04279ec6 100644
--- a/lib/librte_ethdev/rte_class_eth.c
+++ b/lib/librte_ethdev/rte_class_eth.c
@@ -57,9 +57,14 @@  eth_dev_iterate(const void *start,
 {
 	struct rte_kvargs *kvargs = NULL;
 	struct rte_eth_dev *edev = NULL;
+	const char * const *valid_keys = NULL;
 
 	if (str != NULL) {
-		kvargs = rte_kvargs_parse(str, eth_params_keys);
+		if (str[0] == '+') /* no validation of keys */
+			str++;
+		else
+			valid_keys = eth_params_keys;
+		kvargs = rte_kvargs_parse(str, valid_keys);
 		if (kvargs == NULL) {
 			RTE_LOG(ERR, EAL, "cannot parse argument list\n");
 			rte_errno = EINVAL;
diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 734f86a71..73919149a 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -36,11 +36,13 @@ 
 #include <rte_spinlock.h>
 #include <rte_string_fns.h>
 #include <rte_kvargs.h>
+#include <rte_class.h>
 
 #include "rte_ether.h"
 #include "rte_ethdev.h"
 #include "rte_ethdev_driver.h"
 #include "ethdev_profile.h"
+#include "ethdev_private.h"
 
 int rte_eth_dev_logtype;
 
@@ -181,6 +183,127 @@  enum {
 	STAT_QMAP_RX
 };
 
+int __rte_experimental
+rte_eth_iterator_init(struct rte_dev_iterator *iter, const char *devargs_str)
+{
+	int ret;
+	struct rte_devargs devargs = {.args = NULL};
+	const char *bus_param_key;
+	char *bus_str = NULL;
+	char *cls_str = NULL;
+	int str_size;
+
+	memset(iter, 0, sizeof(*iter));
+
+	/*
+	 * The devargs string may use various syntaxes:
+	 *   - 0000:08:00.0,representor=[1-3]
+	 *   - pci:0000:06:00.0,representor=[0,5]
+	 * A new syntax is in development (not yet supported):
+	 *   - bus=X,paramX=x/class=Y,paramY=y/driver=Z,paramZ=z
+	 */
+
+	/* Split bus, device and parameters. */
+	ret = rte_devargs_parse(&devargs, devargs_str);
+	if (ret != 0)
+		goto error;
+
+	/*
+	 * Assume parameters of old syntax can match only at ethdev level.
+	 * Extra parameters will be ignored, thanks to "+" prefix.
+	 */
+	str_size = strlen(devargs.args) + 2;
+	cls_str = malloc(str_size);
+	if (cls_str == NULL) {
+		ret = -ENOMEM;
+		goto error;
+	}
+	ret = snprintf(cls_str, str_size, "+%s", devargs.args);
+	if (ret != str_size - 1) {
+		ret = -EINVAL;
+		goto error;
+	}
+	iter->cls_str = cls_str;
+	free(devargs.args); /* allocated by rte_devargs_parse() */
+	devargs.args = NULL;
+
+	iter->bus = devargs.bus;
+	if (iter->bus->dev_iterate == NULL) {
+		ret = -ENOTSUP;
+		goto error;
+	}
+
+	/* Convert bus args to new syntax for use with new API dev_iterate. */
+	if (strcmp(iter->bus->name, "vdev") == 0) {
+		bus_param_key = "name";
+	} else if (strcmp(iter->bus->name, "pci") == 0) {
+		bus_param_key = "addr";
+	} else {
+		ret = -ENOTSUP;
+		goto error;
+	}
+	str_size = strlen(bus_param_key) + strlen(devargs.name) + 2;
+	bus_str = malloc(str_size);
+	if (bus_str == NULL) {
+		ret = -ENOMEM;
+		goto error;
+	}
+	ret = snprintf(bus_str, str_size, "%s=%s",
+			bus_param_key, devargs.name);
+	if (ret != str_size - 1) {
+		ret = -EINVAL;
+		goto error;
+	}
+	iter->bus_str = bus_str;
+
+	iter->cls = rte_class_find_by_name("eth");
+	return 0;
+
+error:
+	if (ret == -ENOTSUP)
+		RTE_LOG(ERR, EAL, "Bus %s does not support iterating.\n",
+				iter->bus->name);
+	free(devargs.args);
+	free(bus_str);
+	free(cls_str);
+	return ret;
+}
+
+uint16_t __rte_experimental
+rte_eth_iterator_next(struct rte_dev_iterator *iter)
+{
+	if (iter->cls == NULL) /* invalid ethdev iterator */
+		return RTE_MAX_ETHPORTS;
+
+	do { /* loop to try all matching rte_device */
+		/* If not in middle of rte_eth_dev iteration, */
+		if (iter->class_device == NULL) {
+			/* get next rte_device to try. */
+			iter->device = iter->bus->dev_iterate(
+					iter->device, iter->bus_str, iter);
+			if (iter->device == NULL)
+				break; /* no more rte_device candidate */
+		}
+		/* A device is matching bus part, need to check ethdev part. */
+		iter->class_device = iter->cls->dev_iterate(
+				iter->class_device, iter->cls_str, iter);
+		if (iter->class_device != NULL)
+			return eth_dev_to_id(iter->class_device); /* match */
+	} while (1); /* need to try next rte_device */
+
+	/* No more ethdev port to iterate. */
+	rte_eth_iterator_cleanup(iter);
+	return RTE_MAX_ETHPORTS;
+}
+
+void __rte_experimental
+rte_eth_iterator_cleanup(struct rte_dev_iterator *iter)
+{
+	free(RTE_CAST_FIELD(iter, bus_str, char *)); /* workaround const */
+	free(RTE_CAST_FIELD(iter, cls_str, char *)); /* workaround const */
+	memset(iter, 0, sizeof(*iter));
+}
+
 uint16_t
 rte_eth_find_next(uint16_t port_id)
 {
diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index bd6195c40..1cb2b553b 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -166,6 +166,85 @@  extern int rte_eth_dev_logtype;
 
 struct rte_mbuf;
 
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice.
+ *
+ * Initializes a device iterator.
+ *
+ * This iterator allows accessing a list of devices matching some devargs.
+ *
+ * @param iter
+ *   Device iterator handle initialized by the function.
+ *   The fields bus_str and cls_str might be dynamically allocated,
+ *   and could be freed by calling rte_eth_iterator_cleanup().
+ *
+ * @param devargs
+ *   Device description string.
+ *
+ * @return
+ *   0 on successful initialization, negative otherwise.
+ */
+__rte_experimental
+int rte_eth_iterator_init(struct rte_dev_iterator *iter, const char *devargs);
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice.
+ *
+ * Iterates on devices with devargs filter.
+ * The ownership is not checked.
+ *
+ * The next port id is returned, and the iterator is updated.
+ *
+ * @param iter
+ *   Device iterator handle initialized by rte_eth_iterator_init().
+ *   Some fields bus_str and cls_str might be freed when no more port is found,
+ *   by calling rte_eth_iterator_cleanup().
+ *
+ * @return
+ *   A port id if found, RTE_MAX_ETHPORTS otherwise.
+ */
+__rte_experimental
+uint16_t rte_eth_iterator_next(struct rte_dev_iterator *iter);
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice.
+ *
+ * Free some allocated fields of the iterator.
+ *
+ * This function is automatically called by rte_eth_iterator_next()
+ * on the last iteration (i.e. when no more matching port is found).
+ *
+ * It is safe to call this function twice; it will do nothing more.
+ *
+ * @param iter
+ *   Device iterator handle initialized by rte_eth_iterator_init().
+ *   The fields bus_str and cls_str are freed if needed.
+ */
+__rte_experimental
+void rte_eth_iterator_cleanup(struct rte_dev_iterator *iter);
+
+/**
+ * Macro to iterate over all ethdev ports matching some devargs.
+ *
+ * If a break is done before the end of the loop,
+ * the function rte_eth_iterator_cleanup() must be called.
+ *
+ * @param id
+ *   Iterated port id of type uint16_t.
+ * @param devargs
+ *   Device parameters input as string of type char*.
+ * @param iter
+ *   Iterator handle of type struct rte_dev_iterator, used internally.
+ */
+#define RTE_ETH_FOREACH_MATCHING_DEV(id, devargs, iter) \
+	for (rte_eth_iterator_init(iter, devargs), \
+	     id = rte_eth_iterator_next(iter); \
+	     id != RTE_MAX_ETHPORTS; \
+	     id = rte_eth_iterator_next(iter))
+
 /**
  * A structure used to retrieve statistics for an Ethernet port.
  * Not all statistics fields in struct rte_eth_stats are supported
diff --git a/lib/librte_ethdev/rte_ethdev_version.map b/lib/librte_ethdev/rte_ethdev_version.map
index 38f117f01..b4042e398 100644
--- a/lib/librte_ethdev/rte_ethdev_version.map
+++ b/lib/librte_ethdev/rte_ethdev_version.map
@@ -237,6 +237,9 @@  EXPERIMENTAL {
 	rte_eth_dev_owner_unset;
 	rte_eth_dev_rx_offload_name;
 	rte_eth_dev_tx_offload_name;
+	rte_eth_iterator_cleanup;
+	rte_eth_iterator_init;
+	rte_eth_iterator_next;
 	rte_eth_switch_domain_alloc;
 	rte_eth_switch_domain_free;
 	rte_flow_expand_rss;