replace attach/detach functions

A virtual device can be matched with following syntax:
bus=vdev,name=X

Signed-off-by Thomas Monjalon <[email protected]>

---
  drivers/bus/vdev/vdev_params.c | 21 ++++++++++++++++++---
  1 file changed, 18 insertions(+), 3 deletions(-)

drivers/bus/vdev/vdev_params.c +4 -0

index da270f2ec..133998c3e 100644
@@ -2,6 +2,8 @@
   * Copyright 2018 Gaëtan Rivet
   */
 
+#include <string.h>
+
  #include <rte_dev.h>
  #include <rte_bus.h>
  #include <rte_kvargs.h>

@@ -11,10 +13,12 @@
  #include "vdev_private.h"
 
  enum vdev_params {
+	RTE_VDEV_PARAM_NAME,
  	RTE_VDEV_PARAM_MAX,
  };
 
  static const char * const vdev_params_keys[] = {
+	[RTE_VDEV_PARAM_NAME] = "name",
  	[RTE_VDEV_PARAM_MAX] = NULL,
  };
 

@@ -22,11 +26,22 @@ static int
  vdev_dev_match(const struct rte_device *dev,

drivers/bus/vdev/vdev_params.c +0 -0

  }
 
  void *

Quoted 43 lines

A virtual device can be matched with following syntax:
bus=vdev,name=X

Signed-off-by Thomas Monjalon <[email protected]>

---
  drivers/bus/vdev/vdev_params.c | 21 ++++++++++++++++++---
  1 file changed, 18 insertions(+), 3 deletions(-)

drivers/bus/vdev/vdev_params.c +8 -0

index da270f2ec..133998c3e 100644
@@ -2,6 +2,8 @@
   * Copyright 2018 Gaëtan Rivet
   */
 
+#include <string.h>
+
  #include <rte_dev.h>
  #include <rte_bus.h>
  #include <rte_kvargs.h>

@@ -11,10 +13,12 @@
  #include "vdev_private.h"
 
  enum vdev_params {
+	RTE_VDEV_PARAM_NAME,
  	RTE_VDEV_PARAM_MAX,
  };
 
  static const char * const vdev_params_keys[] = {
+	[RTE_VDEV_PARAM_NAME] = "name",
  	[RTE_VDEV_PARAM_MAX] = NULL,
  };
 

@@ -22,11 +26,22 @@ static int
  vdev_dev_match(const struct rte_device *dev,
  	       const void *_kvlist)
  {
+	int ret;
  	const struct rte_kvargs *kvlist = _kvlist;
+	char *name;
+
+	/* cannot pass const dev->name to rte_kvargs_process() */

drivers/bus/vdev/vdev_params.c static int · line 36

Quoted 3 lines

drivers/bus/vdev/vdev_params.c +3 -0

+	name = strdup(dev->name);
+	if (name == NULL)
+		return -ENOMEM; /* interpreted as no match */

It is strange to see -ENOMEM and -1 returned from the same function. rte_dev_cmp_t does not return negative errno. It just says match / no match (greater / smaller than 0 if ordering is possible). So, -ENOMEM is really confusing here. I think just -1 should be used.

Quoted 7 lines

drivers/bus/vdev/vdev_params.c +6 -0

+	ret = rte_kvargs_process(kvlist,
+		vdev_params_keys[RTE_VDEV_PARAM_NAME],
+		rte_kvargs_strcmp, name);
+	free(name);
+	if (ret != 0)
+		return -1;
 

drivers/bus/vdev/vdev_params.c static int · line 44

Quoted 4 lines

drivers/bus/vdev/vdev_params.c +1 -3

-	(void) kvlist;
-	(void) dev;
-	return 0;
+	return ret;

I'm not sure that I understand why 'ret' is returned here instead of 0. Above check guarantees that ret==0. If you change it, it should be a good reason.

Signed-off-by Thomas Monjalon <[email protected]>

---
  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_ethdev.c             | 87 ++++++++++++++++++++++
  lib/librte_ethdev/rte_ethdev.h             | 56 ++++++++++++++
  lib/librte_ethdev/rte_ethdev_version.map   |  2 +
  6 files changed, 166 insertions(+), 1 deletion(-)

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

index 069c13ec7..2269e5456 100644

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

  /*********** Macros/static functions for doing alignment ********/
 
 

lib/librte_ethdev/ethdev_private.c +9 -1

index 768c8b2ed..acc787dba 100644
@@ -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++) {

lib/librte_ethdev/ethdev_private.h +6 -0

index 0f5c6d5c4..e67cf6831 100644
@@ -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 *);
 

lib/librte_ethdev/rte_ethdev.c +14 -0

index ef99f7068..83ab28c23 100644
@@ -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,91 @@ enum {
  	STAT_QMAP_RX
  };
 
+int __rte_experimental
+rte_eth_iterator_init(struct rte_dev_iterator *iter, const char *devargs_str)
+{
+#define iter_anybus_str "class=eth,"
+	int ret;
+	struct rte_devargs devargs;
+	const char *bus_param_key;
+	char *bus_str;
+	size_t bus_str_size;
+
+	memset(iter, 0, sizeof(*iter));
+

lib/librte_ethdev/rte_ethdev.c +46 -0

+	if (ret != 0)
+		return ret;
+
+	/* Assume parameters of old syntax can match only at ethdev level. */
+	iter->cls_str = devargs.args;
+
+	iter->bus = devargs.bus;
+	if (iter->bus->dev_iterate == NULL)
+		ret = -ENOTSUP; /* share error log with below */
+
+	/* 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;
+	if (ret < 0) {
+		RTE_LOG(ERR, EAL, "Bus %s does not support iterating.\n",
+				iter->bus->name);
+		return -ENOTSUP;
+	}
+	bus_str_size = strlen(bus_param_key) + strlen(devargs.name) + 2;
+	bus_str = malloc(bus_str_size);
+	if (bus_str == NULL)
+		return -ENOMEM;
+	ret = snprintf(bus_str, bus_str_size, "%s=%s",
+			bus_param_key, devargs.name);
+	if (ret < 0) {
+		free(bus_str);
+		return -EINVAL;
+	}
+	iter->bus_str = bus_str;
+
+	iter->cls = rte_class_find_by_name("eth");
+	return 0;
+}
+
+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 for matching rte_device */
+		if (iter->class_device == NULL) {

lib/librte_ethdev/rte_ethdev.c +3 -0

+	return RTE_MAX_ETHPORTS;
+}
+
  uint16_t
  rte_eth_find_next(uint16_t port_id)
  {

lib/librte_ethdev/rte_ethdev.h +46 -0

index 012577b0a..d5a457216 100644
@@ -166,6 +166,62 @@ 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 field bus_str is dynamically allocated and must be freed.
+ *
+ * @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().
+ *   The field bus_str is freed when no more port is found.
+ *
+ * @return
+ *   A port id if found, RTE_MAX_ETHPORTS otherwise.
+ */
+__rte_experimental
+uint16_t rte_eth_iterator_next(struct rte_dev_iterator *iter);
+
+/**
+ * Macro to iterate over all ethdev ports matching some devargs.
+ *
+ * @param id
+ *   Iterated port id of type uint16_t.
+ * @param devargs

lib/librte_ethdev/rte_ethdev.h +0 -0

  /**
   * A structure used to retrieve statistics for an Ethernet port.
   * Not all statistics fields in struct rte_eth_stats are supported

lib/librte_ethdev/rte_ethdev_version.map +2 -0

index 38f117f01..e009988fd 100644
@@ -237,6 +237,8 @@ EXPERIMENTAL {
  	rte_eth_dev_owner_unset;
  	rte_eth_dev_rx_offload_name;
  	rte_eth_dev_tx_offload_name;
+	rte_eth_iterator_init;
+	rte_eth_iterator_next;
  	rte_eth_switch_domain_alloc;
  	rte_eth_switch_domain_free;
  	rte_flow_expand_rss;

lib/librte_eal/common/include/rte_common.h static void __attribute__((destructor(RTE_PRIO(prio)), used)) func(void) · line 170

Quoted 10 lines

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

@@ -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) \

Missing space after each comma.

lib/librte_eal/common/include/rte_common.h static void __attribute__((destructor(RTE_PRIO(prio)), used)) func(void) · line 172

Quoted 2 lines

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

+	(*(type*)((uintptr_t)(var) + offsetof(typeof(*(var)), field)))
+

In general it is bad symptom that we need it. I'd think more that twice before adding it :)

lib/librte_ethdev/rte_ethdev.c enum { · line 207

Quoted 4 lines

lib/librte_ethdev/rte_ethdev.c +2 -0

+	/* Split bus, device and parameters. */
+	ret = rte_devargs_parse(&devargs, devargs_str);

rte_devargs_parse() does strdup() for args. It requires free() in the function
if devargs parsed successfully, but init fails.
In the case of success it is moved to cls_str which is 'const' and I see not code which frees it as well.

Quoted 7 lines

lib/librte_ethdev/rte_ethdev.c +7 -0

+	do { /* loop for matching rte_device */
+		if (iter->class_device == NULL) {
+			iter->device = iter->bus->dev_iterate(
+					iter->device, iter->bus_str, iter);
+			if (iter->device == NULL)
+				break;
+		}

lib/librte_ethdev/rte_ethdev.c enum { · line 263

Quoted 5 lines

lib/librte_ethdev/rte_ethdev.c +5 -0

+		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);
+	} while (iter->class_device == NULL);

It is non-obvious what is happening above. It would be very useful to add comments which explains it. May be just hints.

lib/librte_ethdev/rte_ethdev.c enum { · line 267

Quoted 4 lines

lib/librte_ethdev/rte_ethdev.c +4 -0

+
+	/* No more ethdev port to iterate. */
+	free(RTE_CAST_FIELD(iter, bus_str, char *)); /* workaround const */
+	iter->bus_str = NULL;

Who does it if RTE_ETH_FOREACH_MATCHING_DEV caller breaks the loop before reaching maximum?

lib/librte_ethdev/rte_ethdev.c

Quoted 6 lines

lib/librte_ethdev/rte_ethdev.c +6 -0

+#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))
+

Such iterators are very convenient, but in this particular case it is a source of non-obvious memory leaks and necessity of the hack to discard 'const'.

May be iterator free callback with its own opaque data can help to avoid these hacks with const discard? I.e. rte_eth_iterator_free() which does the job and mentioned in the rte_eth_iterator_init() and the macro description.

If no rte_device is given in the iterator, eth_dev_match() is looking at all ports without any restriction, except the ethdev kvargs filter.

It allows to iterate with a devargs filter referencing only some ethdev parameters. The format (from the new devargs syntax) is:
class=eth,paramY=Y

Fixes e815a7f69371 ("ethdev: register as a class")

Signed-off-by Thomas Monjalon <[email protected]>

---
  lib/librte_ethdev/rte_class_eth.c |  2 +-
  lib/librte_ethdev/rte_ethdev.c    | 13 +++++++++++--
  2 files changed, 12 insertions(+), 3 deletions(-)

lib/librte_ethdev/rte_class_eth.c +0 -0

index 84b646291..f0af51c36 100644

lib/librte_ethdev/rte_ethdev.c +1 -0

index 83ab28c23..a43e0ab3a 100644
@@ -199,10 +199,18 @@ rte_eth_iterator_init(struct rte_dev_iterator *iter, const char *devargs_str)
  	 * The devargs string may use various syntaxes:
  	 *   - 0000:08:00.0,representor=[1-3]
  	 *   - pci:0000:06:00.0,representor=[0,5]
+	 *   - class=eth,mac=00:11:22:33:44:55
  	 * A new syntax is in development (not yet supported):

lib/librte_ethdev/rte_ethdev.c +3 -2

  	/* Split bus, device and parameters. */
  	ret = rte_devargs_parse(&devargs, devargs_str);
  	if (ret != 0)

@@ -239,6 +247,7 @@ rte_eth_iterator_init(struct rte_dev_iterator *iter, const char *devargs_str)
  	}
  	iter->bus_str = bus_str;
 
+end:
  	iter->cls = rte_class_find_by_name("eth");
  	return 0;
  }

@@ -250,7 +259,7 @@ rte_eth_iterator_next(struct rte_dev_iterator *iter)
  		return RTE_MAX_ETHPORTS;
 
  	do { /* loop for matching rte_device */
-		if (iter->class_device == NULL) {
+		if (iter->bus != NULL && iter->class_device == NULL) {
  			iter->device = iter->bus->dev_iterate(
  					iter->device, iter->bus_str, iter);
  			if (iter->device == NULL)

@@ -260,7 +269,7 @@ rte_eth_iterator_next(struct rte_dev_iterator *iter)
  				iter->class_device, iter->cls_str, iter);
  		if (iter->class_device != NULL)
  			return eth_dev_to_id(iter->class_device);
-	} while (iter->class_device == NULL);
+	} while (iter->bus != NULL && iter->class_device == NULL);
 
  	/* No more ethdev port to iterate. */
  	free(RTE_CAST_FIELD(iter, bus_str, char *)); /* workaround const */

Quoted 15 lines

If no rte_device is given in the iterator, eth_dev_match() is looking at all ports without any restriction, except the ethdev kvargs filter.

It allows to iterate with a devargs filter referencing only some ethdev parameters. The format (from the new devargs syntax) is:
class=eth,paramY=Y

Fixes e815a7f69371 ("ethdev: register as a class")

Signed-off-by Thomas Monjalon <[email protected]>

---

lib/librte_ethdev/rte_class_eth.c eth_dev_match(const struct rte_eth_dev *edev, · line 48

Quoted 8 lines

lib/librte_ethdev/rte_class_eth.c +1 -1

@@ -42,7 +42,7 @@ eth_dev_match(const struct rte_eth_dev *edev,
 
  	if (edev->state == RTE_ETH_DEV_UNUSED)
  		return -1;
-	if (edev->device != arg->device)
+	if (arg->device != NULL && arg->device != edev->device)
  		return -1;
  	if (kvlist == NULL)
  		/* Empty string matches everything. */

It looks like it is the only hunk which

Fixes e815a7f69371 ("ethdev: register as a class")

everything else adjusts the previous patch.

I think this fix should go before and the rest should be squashed in the previous patch. It was really questionable why it is safe to dereference iter->bus without checking that it is not NULL.

lib/librte_ethdev/rte_class_eth.c

Quoted 7 lines

lib/librte_ethdev/rte_class_eth.c +7 -0

+	/* Handle a case from future syntax, without any bus-level argument. */
+	if (strncmp(devargs_str, iter_anybus_str,
+			strlen(iter_anybus_str)) == 0) {
+		iter->cls_str = devargs_str + strlen(iter_anybus_str);
+		goto end;
+	}
+

It looks like a hack, but I guess we need it since rte_devargs_parse() cannot handle the case. May be it is acceptable if we have no time to solve it, but it would be good to highlight it better in the comments.

08/10/2018 09:20, Andrew Rybchenko:

Quoted 25 lines · Thomas Monjalon

Quoted 15 lines

If no rte_device is given in the iterator, eth_dev_match() is looking at all ports without any restriction, except the ethdev kvargs filter.

It allows to iterate with a devargs filter referencing only some ethdev parameters. The format (from the new devargs syntax) is:
class=eth,paramY=Y

Fixes e815a7f69371 ("ethdev: register as a class")

Signed-off-by Thomas Monjalon <[email protected]>

---

lib/librte_ethdev/rte_class_eth.c eth_dev_match(const struct rte_eth_dev *edev, · line 48

Quoted 8 lines

lib/librte_ethdev/rte_class_eth.c +1 -1

@@ -42,7 +42,7 @@ eth_dev_match(const struct rte_eth_dev *edev,
 
   	if (edev->state == RTE_ETH_DEV_UNUSED)
   		return -1;
-	if (edev->device != arg->device)
+	if (arg->device != NULL && arg->device != edev->device)
   		return -1;
   	if (kvlist == NULL)
   		/* Empty string matches everything. */

It looks like it is the only hunk which

Quoted 1 line

Fixes e815a7f69371 ("ethdev: register as a class")

Yes this hunk is fixing above commit.

Quoted 1 line

everything else adjusts the previous patch.

Yes but the whole goal of this patch is to allow ethdev pure filter. All is related in this patch.

Quoted 3 lines

I think this fix should go before and the rest should be squashed in the previous patch. It was really questionable why it is safe to dereference iter->bus without checking that it is not NULL.

No iter->bus was safe because iter->cls was checked before and implied that iter->bus was successfully set.

I still think it is better for understanding to split different kind of filters in 2 patches.
I may remove the Fixes line however. Opinion?

[...]

Quoted 10 lines

lib/librte_ethdev/rte_class_eth.c

Quoted 7 lines

lib/librte_ethdev/rte_class_eth.c +7 -0

+	/* Handle a case from future syntax, without any bus-level argument. */
+	if (strncmp(devargs_str, iter_anybus_str,
+			strlen(iter_anybus_str)) == 0) {
+		iter->cls_str = devargs_str + strlen(iter_anybus_str);
+		goto end;
+	}
+

It looks like a hack, but I guess we need it since rte_devargs_parse() cannot handle the case. May be it is acceptable if we have no time to solve it, but it would be good to highlight it better in the comments.

This function is a mix of old and new syntax. When only new syntax will be supported, it will be better. It is a work in progress.
I do not know how to better explain it.

The hotplug attach/detach features are implemented in EAL layer. There is a new ethdev iterator to retrieve ports from ethdev layer.

As announced earlier, the (buggy) ethdev functions are now removed.

Signed-off-by Thomas Monjalon <[email protected]>

doc/guides/contributing/documentation.rst +2 -8

index 097575ad7..063c8b6c4 100644
@@ -615,19 +615,14 @@ The following are some guidelines for use of Doxygen in the DPDK API documentati
    .. code-block:: c
 
       /**
-      * Attach a new Ethernet device specified by arguments.
-      *
-      * @param devargs
-      *  A pointer to a strings array describing the new device
-      *  to be attached. The strings should be a pci address like
-      *  `0000:01:00.0` or **virtual** device name like `net_pcap0`.
-      * @param port_id
-      *  A pointer to a port identifier actually attached.
+      * Try to take the lock.
        *
+      * @param sl

Quoted 6 lines

The hotplug attach/detach features are implemented in EAL layer. There is a new ethdev iterator to retrieve ports from ethdev layer.

As announced earlier, the (buggy) ethdev functions are now removed.

Signed-off-by Thomas Monjalon <[email protected]>

<...>

Quoted 27 lines

doc/guides/contributing/documentation.rst +5 -10

index 097575ad7..063c8b6c4 100644
@@ -615,19 +615,14 @@ The following are some guidelines for use of Doxygen in the DPDK API documentati
    .. code-block:: c
 
       /**
-      * Attach a new Ethernet device specified by arguments.
-      *
-      * @param devargs
-      *  A pointer to a strings array describing the new device
-      *  to be attached. The strings should be a pci address like
-      *  `0000:01:00.0` or **virtual** device name like `net_pcap0`.
-      * @param port_id
-      *  A pointer to a port identifier actually attached.
+      * Try to take the lock.
        *
+      * @param sl
+      *   A pointer to the spinlock.
        * @return
-      *  0 on success and port_id is filled, negative on error.
+      *   1 if the lock is successfully taken; 0 otherwise.
        */
-     int rte_eth_dev_attach(const char *devargs, uint8_t *port_id);
+     int rte_spinlock_trylock (rte_spinlock_t *sl);
 

doc/guides/contributing/documentation.rst The following are some guidelines for use of Doxygen in the DPDK API documentati · line 628

Quoted 2 lines

doc/guides/contributing/documentation.rst +0 -0

  * Doxygen supports Markdown style syntax such as bold, italics, fixed width text and lists.
    For example the second line in the ``devargs`` parameter in the previous example will be rendered as:

I think it would be better if the hunk goes separately. It is really confusing in the changeset.

A virtual device can be matched with following syntax:
bus=vdev,name=X

Signed-off-by Thomas Monjalon <[email protected]>

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 <[email protected]>

---
  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             | 124 +++++++++++++++++++++
  lib/librte_ethdev/rte_ethdev.h             |  77 +++++++++++++
  lib/librte_ethdev/rte_ethdev_version.map   |   2 +
  7 files changed, 230 insertions(+), 2 deletions(-)

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

index 069c13ec7..e3c0407a9 100644
@@ -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 ********/
 
 

lib/librte_ethdev/ethdev_private.c +9 -1

index 768c8b2ed..acc787dba 100644
@@ -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++) {

lib/librte_ethdev/ethdev_private.h +6 -0

index 0f5c6d5c4..e67cf6831 100644
@@ -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 *);
 

lib/librte_ethdev/rte_class_eth.c +0 -0

index 84b646291..532080a58 100644

lib/librte_ethdev/rte_class_eth.c +3 -0

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

lib/librte_ethdev/rte_ethdev.c +29 -0

index ef99f7068..da20ab81f 100644
@@ -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,128 @@ 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;
+	size_t 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.

lib/librte_ethdev/rte_ethdev.c +8 -0

+		ret = -EINVAL;
+		goto error;
+	}
+	iter->cls_str = cls_str;
+	free(devargs.args); /* allocated by rte_devargs_parse() */
+	devargs.args = NULL;
+
+	iter->bus = devargs.bus;

lib/librte_ethdev/rte_ethdev.c +37 -0

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

lib/librte_ethdev/rte_ethdev.c +7 -0

+{
+	free(RTE_CAST_FIELD(iter, bus_str, char *)); /* workaround const */
+	iter->bus_str = NULL;
+	free(RTE_CAST_FIELD(iter, cls_str, char *)); /* workaround const */
+	iter->cls_str = NULL;
+}
+
  uint16_t
  rte_eth_find_next(uint16_t port_id)
  {

lib/librte_ethdev/rte_ethdev.h +41 -0

index 012577b0a..9c12c8cdf 100644
@@ -166,6 +166,83 @@ 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_free().
+ *
+ * @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_free().
+ *
+ * @return
+ *   A port id if found, RTE_MAX_ETHPORTS otherwise.
+ */
+__rte_experimental
+uint16_t rte_eth_iterator_next(struct rte_dev_iterator *iter);

lib/librte_ethdev/rte_ethdev.h +26 -0

+ * @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_free(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_free() 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

lib/librte_ethdev/rte_ethdev_version.map +0 -0

index 38f117f01..e009988fd 100644

lib/librte_ethdev/rte_ethdev_version.map +0 -0

  	rte_eth_switch_domain_alloc;
  	rte_eth_switch_domain_free;
  	rte_flow_expand_rss;

lib/librte_ethdev/rte_class_eth.c eth_dev_iterate(const void *start, · line 64

Quoted 4 lines

  	if (str != NULL) {

lib/librte_ethdev/rte_class_eth.c +2 -1

-		kvargs = rte_kvargs_parse(str, eth_params_keys);
+		if (str[0] == '+') /* no validation of keys */
+			str ++;

As I understand it should be no space before ++

Quoted 7 lines

lib/librte_ethdev/rte_ethdev.c +7 -0

+	/*
+	 * 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) {

lib/librte_ethdev/rte_ethdev.c enum { · line 222

Quoted 5 lines

lib/librte_ethdev/rte_ethdev.c +5 -0

+		ret = -ENOMEM;
+		goto error;
+	}
+	ret = snprintf(cls_str, str_size, "+%s", devargs.args);
+	if (ret < 0) {

As I understand we expect ret == str_size - 1 here. May be it makes sent to harden the check.

Quoted 13 lines

lib/librte_ethdev/rte_ethdev.c +13 -0

+		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;
+	}
+

lib/librte_ethdev/rte_ethdev.c enum { · line 240

Quoted 5 lines

lib/librte_ethdev/rte_ethdev.c +5 -0

+	/* 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";

I thought that if one branch has curly brackets other branches should have curly brackets as well.

Quoted 7 lines

lib/librte_ethdev/rte_ethdev.c +7 -0

+	else {
+		ret = -ENOTSUP;
+		goto error;
+	}
+	str_size = strlen(bus_param_key) + strlen(devargs.name) + 2;
+	bus_str = malloc(str_size);
+	if (bus_str == NULL) {

lib/librte_ethdev/rte_ethdev.c enum { · line 253

Quoted 6 lines

lib/librte_ethdev/rte_ethdev.c +6 -0

+		ret = -ENOMEM;
+		goto error;
+	}
+	ret = snprintf(bus_str, str_size, "%s=%s",
+			bus_param_key, devargs.name);
+	if (ret < 0) {

May be it makes sense to make the check more strict: ret != str_size - 1

Quoted 3 lines

lib/librte_ethdev/rte_ethdev.c +3 -0

+		ret = -EINVAL;
+		goto error;
+	}

lib/librte_ethdev/rte_ethdev.c enum { · line 300

Quoted 2 lines

lib/librte_ethdev/rte_ethdev.c +2 -0

+void __rte_experimental
+rte_eth_iterator_free(struct rte_dev_iterator *iter)

Yes, I know that the name is suggested by me, but maybe it should be rte_eth_iterator_fini() or _cleanup() as pair to _init.

Quoted 6 lines

lib/librte_ethdev/rte_ethdev.c +6 -0

+{
+	free(RTE_CAST_FIELD(iter, bus_str, char *)); /* workaround const */
+	iter->bus_str = NULL;
+	free(RTE_CAST_FIELD(iter, cls_str, char *)); /* workaround const */
+	iter->cls_str = NULL;
+}

Quoted 9 lines

+/**
+ * @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).
+ *

May be it makes sense to mention that it is safe to call it twice. It could be simpler to use it taking the possibility into account.

Quoted 6 lines

+ * @param iter
+ * Device iterator handle initialized by rte_eth_iterator_init().
+ * The fields bus_str and cls_str are freed if needed.
+ */

(quoted diff) +2 -0

+__rte_experimental
+void rte_eth_iterator_free(struct rte_dev_iterator *iter);

lib/librte_ethdev/rte_ethdev_version.map EXPERIMENTAL { · line 241

Quoted 8 lines

lib/librte_ethdev/rte_ethdev_version.map +2 -0

@@ -237,6 +237,8 @@ EXPERIMENTAL {
  	rte_eth_dev_owner_unset;
  	rte_eth_dev_rx_offload_name;
  	rte_eth_dev_tx_offload_name;
+	rte_eth_iterator_init;
+	rte_eth_iterator_next;

rte_eth_iterator_free() or renamed

If no rte_device is given in the iterator, eth_dev_match() is looking at all ports without any restriction, except the ethdev kvargs filter.

It allows to iterate with a devargs filter referencing only some ethdev parameters. The format (from the new devargs syntax) is:
class=eth,paramY=Y

Signed-off-by Thomas Monjalon <[email protected]>

The provided example of doxygen header is about a deprecated function. It is replaced by rte_spinlock_trylock() which is small and good enough for the purpose.

Signed-off-by Thomas Monjalon <[email protected]>

---
  doc/guides/contributing/documentation.rst | 15 +++++----------
  1 file changed, 5 insertions(+), 10 deletions(-)

doc/guides/contributing/documentation.rst +0 -8

index 097575ad7..063c8b6c4 100644
@@ -615,19 +615,14 @@ The following are some guidelines for use of Doxygen in the DPDK API documentati
    .. code-block:: c
 
       /**
-      * Attach a new Ethernet device specified by arguments.
-      *
-      * @param devargs
-      *  A pointer to a strings array describing the new device
-      *  to be attached. The strings should be a pci address like
-      *  `0000:01:00.0` or **virtual** device name like `net_pcap0`.
-      * @param port_id
-      *  A pointer to a port identifier actually attached.

doc/guides/contributing/documentation.rst +0 -0

  * Doxygen supports Markdown style syntax such as bold, italics, fixed width text and lists.
    For example the second line in the ``devargs`` parameter in the previous example will be rendered as:

The hotplug attach/detach features are implemented in EAL layer. There is a new ethdev iterator to retrieve ports from ethdev layer.

As announced earlier, the (buggy) ethdev functions are now removed.

Signed-off-by Thomas Monjalon <[email protected]>

These hotplug functions were deprecated and have some new replacements. As announced earlier, the oldest ones are now removed.

Signed-off-by Thomas Monjalon <[email protected]>

lib/librte_ethdev/rte_ethdev.c +5 -0

+       ret = snprintf(cls_str, str_size, "+%s", devargs.args);
+       if (ret != str_size - 1) {
+               ret = -EINVAL;
+               goto error;
+       }

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 <[email protected]>

Reviewed-by Andrew Rybchenko <[email protected]>

Quoted 18 lines

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 <[email protected]>

Reviewed-by Andrew Rybchenko <[email protected]>

<...>

lib/librte_ethdev/rte_ethdev_version.map EXPERIMENTAL { · line 242

Quoted 3 lines

lib/librte_ethdev/rte_ethdev_version.map +3 -0

+	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?