All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] ethdev: add siblings iterator
@ 2018-11-30  0:27 Thomas Monjalon
  2018-12-11 16:31 ` Ferruh Yigit
                   ` (2 more replies)
  0 siblings, 3 replies; 41+ messages in thread
From: Thomas Monjalon @ 2018-11-30  0:27 UTC (permalink / raw)
  To: ferruh.yigit, arybchenko; +Cc: dev

If multiple ports share the same hardware device (rte_device),
they are siblings and can be found thanks to the new function
and loop macro.

The ownership is not checked because siblings may have
different owners.

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
 lib/librte_ethdev/rte_ethdev.c           | 15 +++++++++++++++
 lib/librte_ethdev/rte_ethdev.h           | 23 +++++++++++++++++++++++
 lib/librte_ethdev/rte_ethdev_version.map |  1 +
 3 files changed, 39 insertions(+)

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 5f858174b..11e0ade6e 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -341,6 +341,21 @@ rte_eth_find_next(uint16_t port_id)
 	return port_id;
 }
 
+uint16_t __rte_experimental
+rte_eth_find_next_sibling(uint16_t port_id, uint16_t ref)
+{
+	while (port_id < RTE_MAX_ETHPORTS &&
+			rte_eth_devices[port_id].state == RTE_ETH_DEV_UNUSED &&
+			rte_eth_devices[port_id].device !=
+				rte_eth_devices[ref].device)
+		port_id++;
+
+	if (port_id >= RTE_MAX_ETHPORTS)
+		return RTE_MAX_ETHPORTS;
+
+	return port_id;
+}
+
 static void
 rte_eth_dev_shared_data_prepare(void)
 {
diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index 1960f3a2d..647e6634d 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -1383,6 +1383,29 @@ uint16_t rte_eth_find_next(uint16_t port_id);
 #define RTE_ETH_FOREACH_DEV(p) \
 	RTE_ETH_FOREACH_DEV_OWNED_BY(p, RTE_ETH_DEV_NO_OWNER)
 
+/**
+ * Iterates over sibling ethdev ports (i.e. sharing the same rte_device).
+ *
+ * @param port_id_start
+ *   The id of the next possible valid sibling port.
+ * @param ref
+ *   The id of a reference port to compare rte_device with.
+ * @return
+ *   Next sibling port id (or ref itself), RTE_MAX_ETHPORTS if there is none.
+ */
+__rte_experimental
+uint16_t rte_eth_find_next_sibling(uint16_t port_id_start, uint16_t ref);
+
+/**
+ * Macro to iterate over all ethdev ports sharing the same rte_device
+ * as the specified port.
+ * Note: the specified port is part of the loop iterations.
+ */
+#define RTE_ETH_FOREACH_DEV_SIBLING(p, ref) \
+	for (p = rte_eth_find_next_sibling(0, ref); \
+		p < RTE_MAX_ETHPORTS; \
+		p = rte_eth_find_next_sibling(p + 1, ref))
+
 
 /**
  * @warning
diff --git a/lib/librte_ethdev/rte_ethdev_version.map b/lib/librte_ethdev/rte_ethdev_version.map
index 92ac3de25..a0c930d04 100644
--- a/lib/librte_ethdev/rte_ethdev_version.map
+++ b/lib/librte_ethdev/rte_ethdev_version.map
@@ -245,6 +245,7 @@ EXPERIMENTAL {
 	rte_eth_dev_owner_set;
 	rte_eth_dev_owner_unset;
 	rte_eth_dev_rx_intr_ctl_q_get_fd;
+	rte_eth_find_next_sibling;
 	rte_eth_switch_domain_alloc;
 	rte_eth_switch_domain_free;
 	rte_flow_conv;
-- 
2.19.0

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH] ethdev: add siblings iterator
  2018-11-30  0:27 [PATCH] ethdev: add siblings iterator Thomas Monjalon
@ 2018-12-11 16:31 ` Ferruh Yigit
  2018-12-11 18:19   ` Thomas Monjalon
  2019-02-20 22:10 ` [PATCH v2 0/4] ethdev iterators for multi-ports device Thomas Monjalon
  2019-04-01  2:26 ` [PATCH v3 0/4] ethdev iterators for multi-ports device Thomas Monjalon
  2 siblings, 1 reply; 41+ messages in thread
From: Ferruh Yigit @ 2018-12-11 16:31 UTC (permalink / raw)
  To: Thomas Monjalon, arybchenko; +Cc: dev

On 11/30/2018 12:27 AM, Thomas Monjalon wrote:
> If multiple ports share the same hardware device (rte_device),
> they are siblings and can be found thanks to the new function
> and loop macro.
> 
> The ownership is not checked because siblings may have
> different owners.

Looks good on its own, but I think now we require an implementation of any new
API, so it can be good to have:
- a sample implementation of this new API and the macro
- an unit test for the API and the macro

> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> ---
>  lib/librte_ethdev/rte_ethdev.c           | 15 +++++++++++++++
>  lib/librte_ethdev/rte_ethdev.h           | 23 +++++++++++++++++++++++
>  lib/librte_ethdev/rte_ethdev_version.map |  1 +
>  3 files changed, 39 insertions(+)
> 
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 5f858174b..11e0ade6e 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -341,6 +341,21 @@ rte_eth_find_next(uint16_t port_id)
>  	return port_id;
>  }
>  
> +uint16_t __rte_experimental

I think __rte_experimental tag only in header file is sufficient

> +rte_eth_find_next_sibling(uint16_t port_id, uint16_t ref)
> +{
> +	while (port_id < RTE_MAX_ETHPORTS &&
> +			rte_eth_devices[port_id].state == RTE_ETH_DEV_UNUSED &&
> +			rte_eth_devices[port_id].device !=
> +				rte_eth_devices[ref].device)
> +		port_id++;
> +
> +	if (port_id >= RTE_MAX_ETHPORTS)
> +		return RTE_MAX_ETHPORTS;
> +
> +	return port_id;
> +}
> +
>  static void
>  rte_eth_dev_shared_data_prepare(void)
>  {
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index 1960f3a2d..647e6634d 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -1383,6 +1383,29 @@ uint16_t rte_eth_find_next(uint16_t port_id);
>  #define RTE_ETH_FOREACH_DEV(p) \
>  	RTE_ETH_FOREACH_DEV_OWNED_BY(p, RTE_ETH_DEV_NO_OWNER)
>  
> +/**
> + * Iterates over sibling ethdev ports (i.e. sharing the same rte_device).
> + *
> + * @param port_id_start
> + *   The id of the next possible valid sibling port.
> + * @param ref
> + *   The id of a reference port to compare rte_device with.
> + * @return
> + *   Next sibling port id (or ref itself), RTE_MAX_ETHPORTS if there is none.
> + */
> +__rte_experimental
> +uint16_t rte_eth_find_next_sibling(uint16_t port_id_start, uint16_t ref);
> +
> +/**
> + * Macro to iterate over all ethdev ports sharing the same rte_device
> + * as the specified port.
> + * Note: the specified port is part of the loop iterations.
> + */
> +#define RTE_ETH_FOREACH_DEV_SIBLING(p, ref) \
> +	for (p = rte_eth_find_next_sibling(0, ref); \
> +		p < RTE_MAX_ETHPORTS; \
> +		p = rte_eth_find_next_sibling(p + 1, ref))
> +
>  
>  /**
>   * @warning
> diff --git a/lib/librte_ethdev/rte_ethdev_version.map b/lib/librte_ethdev/rte_ethdev_version.map
> index 92ac3de25..a0c930d04 100644
> --- a/lib/librte_ethdev/rte_ethdev_version.map
> +++ b/lib/librte_ethdev/rte_ethdev_version.map
> @@ -245,6 +245,7 @@ EXPERIMENTAL {
>  	rte_eth_dev_owner_set;
>  	rte_eth_dev_owner_unset;
>  	rte_eth_dev_rx_intr_ctl_q_get_fd;
> +	rte_eth_find_next_sibling;
>  	rte_eth_switch_domain_alloc;
>  	rte_eth_switch_domain_free;
>  	rte_flow_conv;
> 

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH] ethdev: add siblings iterator
  2018-12-11 16:31 ` Ferruh Yigit
@ 2018-12-11 18:19   ` Thomas Monjalon
  0 siblings, 0 replies; 41+ messages in thread
From: Thomas Monjalon @ 2018-12-11 18:19 UTC (permalink / raw)
  To: Ferruh Yigit; +Cc: arybchenko, dev

11/12/2018 17:31, Ferruh Yigit:
> On 11/30/2018 12:27 AM, Thomas Monjalon wrote:
> > If multiple ports share the same hardware device (rte_device),
> > they are siblings and can be found thanks to the new function
> > and loop macro.
> > 
> > The ownership is not checked because siblings may have
> > different owners.
> 
> Looks good on its own, but I think now we require an implementation of any new
> API, so it can be good to have:
> - a sample implementation of this new API and the macro
> - an unit test for the API and the macro

Yes sure.
I should have added "RFC" in the title.
v2 will have some usage of this API.
About the unit test, I'm really not sure whether we should test the ehtdev
API in test/test/ or just inside testpmd. We used to implement ethdev tests
only in testpmd. Opinions?

^ permalink raw reply	[flat|nested] 41+ messages in thread

* [PATCH v2 0/4] ethdev iterators for multi-ports device
  2018-11-30  0:27 [PATCH] ethdev: add siblings iterator Thomas Monjalon
  2018-12-11 16:31 ` Ferruh Yigit
@ 2019-02-20 22:10 ` Thomas Monjalon
  2019-02-20 22:10   ` [PATCH v2 1/4] ethdev: simplify port state comparisons Thomas Monjalon
                     ` (3 more replies)
  2019-04-01  2:26 ` [PATCH v3 0/4] ethdev iterators for multi-ports device Thomas Monjalon
  2 siblings, 4 replies; 41+ messages in thread
From: Thomas Monjalon @ 2019-02-20 22:10 UTC (permalink / raw)
  Cc: dev

Add port iterators in order to allow listing easily
the ports of the same device.

The iterators can be tested by using mlx5 or testpmd.


Thomas Monjalon (4):
  ethdev: simplify port state comparisons
  ethdev: add siblings iterators
  net/mlx5: use port sibling iterators
  app/testpmd: use port sibling iterator in device cleanup

 app/test-pmd/testpmd.c                   |  4 +--
 drivers/net/mlx5/mlx5.c                  | 34 +++++++-----------
 drivers/net/mlx5/mlx5_ethdev.c           |  6 +---
 lib/librte_ethdev/rte_ethdev.c           | 26 +++++++++++---
 lib/librte_ethdev/rte_ethdev.h           | 46 ++++++++++++++++++++++++
 lib/librte_ethdev/rte_ethdev_version.map |  2 ++
 6 files changed, 85 insertions(+), 33 deletions(-)

-- 
2.20.1

^ permalink raw reply	[flat|nested] 41+ messages in thread

* [PATCH v2 1/4] ethdev: simplify port state comparisons
  2019-02-20 22:10 ` [PATCH v2 0/4] ethdev iterators for multi-ports device Thomas Monjalon
@ 2019-02-20 22:10   ` Thomas Monjalon
  2019-02-24 17:18     ` Andrew Rybchenko
  2019-02-20 22:10   ` [PATCH v2 2/4] ethdev: add siblings iterators Thomas Monjalon
                     ` (2 subsequent siblings)
  3 siblings, 1 reply; 41+ messages in thread
From: Thomas Monjalon @ 2019-02-20 22:10 UTC (permalink / raw)
  To: Ferruh Yigit, Andrew Rybchenko; +Cc: dev

There are three states for an ethdev port.
Checking that the port is unused looks simpler than
checking it is neither attached nor removed.

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
 lib/librte_ethdev/rte_ethdev.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 0d192a24b2..b3b2fb1dba 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -331,8 +331,7 @@ uint16_t
 rte_eth_find_next(uint16_t port_id)
 {
 	while (port_id < RTE_MAX_ETHPORTS &&
-	       rte_eth_devices[port_id].state != RTE_ETH_DEV_ATTACHED &&
-	       rte_eth_devices[port_id].state != RTE_ETH_DEV_REMOVED)
+			rte_eth_devices[port_id].state == RTE_ETH_DEV_UNUSED)
 		port_id++;
 
 	if (port_id >= RTE_MAX_ETHPORTS)
@@ -558,8 +557,7 @@ uint64_t
 rte_eth_find_next_owned_by(uint16_t port_id, const uint64_t owner_id)
 {
 	while (port_id < RTE_MAX_ETHPORTS &&
-	       ((rte_eth_devices[port_id].state != RTE_ETH_DEV_ATTACHED &&
-	       rte_eth_devices[port_id].state != RTE_ETH_DEV_REMOVED) ||
+	       (rte_eth_devices[port_id].state == RTE_ETH_DEV_UNUSED ||
 	       rte_eth_devices[port_id].data->owner.id != owner_id))
 		port_id++;
 
-- 
2.20.1

^ permalink raw reply	[flat|nested] 41+ messages in thread

* [PATCH v2 2/4] ethdev: add siblings iterators
  2019-02-20 22:10 ` [PATCH v2 0/4] ethdev iterators for multi-ports device Thomas Monjalon
  2019-02-20 22:10   ` [PATCH v2 1/4] ethdev: simplify port state comparisons Thomas Monjalon
@ 2019-02-20 22:10   ` Thomas Monjalon
  2019-02-24 17:22     ` Andrew Rybchenko
                       ` (2 more replies)
  2019-02-20 22:10   ` [PATCH v2 3/4] net/mlx5: use port sibling iterators Thomas Monjalon
  2019-02-20 22:10   ` [PATCH v2 4/4] app/testpmd: use port sibling iterator in device cleanup Thomas Monjalon
  3 siblings, 3 replies; 41+ messages in thread
From: Thomas Monjalon @ 2019-02-20 22:10 UTC (permalink / raw)
  To: Ferruh Yigit, Andrew Rybchenko; +Cc: dev

If multiple ports share the same hardware device (rte_device),
they are siblings and can be found thanks to the new functions
and loop macros.
One iterator takes a port id as reference,
while the other one directly refers to the parent device.

The ownership is not checked because siblings may have
different owners.

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
 lib/librte_ethdev/rte_ethdev.c           | 20 +++++++++++
 lib/librte_ethdev/rte_ethdev.h           | 46 ++++++++++++++++++++++++
 lib/librte_ethdev/rte_ethdev_version.map |  2 ++
 3 files changed, 68 insertions(+)

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index b3b2fb1dba..42154787f8 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -340,6 +340,26 @@ rte_eth_find_next(uint16_t port_id)
 	return port_id;
 }
 
+uint16_t __rte_experimental
+rte_eth_find_next_of(uint16_t port_id, const struct rte_device *parent)
+{
+	while (port_id < RTE_MAX_ETHPORTS &&
+			rte_eth_devices[port_id].state == RTE_ETH_DEV_UNUSED &&
+			rte_eth_devices[port_id].device != parent)
+		port_id++;
+
+	if (port_id >= RTE_MAX_ETHPORTS)
+		return RTE_MAX_ETHPORTS;
+
+	return port_id;
+}
+
+uint16_t __rte_experimental
+rte_eth_find_next_sibling(uint16_t port_id, uint16_t ref)
+{
+	return rte_eth_find_next_of(port_id, rte_eth_devices[ref].device);
+}
+
 static void
 rte_eth_dev_shared_data_prepare(void)
 {
diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index a3c864a134..a7c5c36277 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -1383,6 +1383,52 @@ uint16_t rte_eth_find_next(uint16_t port_id);
 #define RTE_ETH_FOREACH_DEV(p) \
 	RTE_ETH_FOREACH_DEV_OWNED_BY(p, RTE_ETH_DEV_NO_OWNER)
 
+/**
+ * Iterates over ethdev ports of a specified device.
+ *
+ * @param port_id_start
+ *   The id of the next possible valid port.
+ * @param parent
+ *   The generic device behind the ports to iterate.
+ * @return
+ *   Next port id of the device, RTE_MAX_ETHPORTS if there is none.
+ */
+__rte_experimental
+uint16_t rte_eth_find_next_of(uint16_t port_id_start,
+		const struct rte_device *parent);
+
+/**
+ * Macro to iterate over all ethdev ports sharing the same rte_device
+ * as the specified port.
+ * Note: the specified port is part of the loop iterations.
+ */
+#define RTE_ETH_FOREACH_DEV_OF(p, parent) \
+	for (p = rte_eth_find_next_of(0, parent); \
+		p < RTE_MAX_ETHPORTS; \
+		p = rte_eth_find_next_of(p + 1, parent))
+
+/**
+ * Iterates over sibling ethdev ports (i.e. sharing the same rte_device).
+ *
+ * @param port_id_start
+ *   The id of the next possible valid sibling port.
+ * @param ref
+ *   The id of a reference port to compare rte_device with.
+ * @return
+ *   Next sibling port id (or ref itself), RTE_MAX_ETHPORTS if there is none.
+ */
+__rte_experimental
+uint16_t rte_eth_find_next_sibling(uint16_t port_id_start, uint16_t ref);
+
+/**
+ * Macro to iterate over all ethdev ports sharing the same rte_device
+ * as the specified port.
+ * Note: the specified port is part of the loop iterations.
+ */
+#define RTE_ETH_FOREACH_DEV_SIBLING(p, ref) \
+	for (p = rte_eth_find_next_sibling(0, ref); \
+		p < RTE_MAX_ETHPORTS; \
+		p = rte_eth_find_next_sibling(p + 1, ref))
 
 /**
  * @warning
diff --git a/lib/librte_ethdev/rte_ethdev_version.map b/lib/librte_ethdev/rte_ethdev_version.map
index 92ac3de250..b37a4167d7 100644
--- a/lib/librte_ethdev/rte_ethdev_version.map
+++ b/lib/librte_ethdev/rte_ethdev_version.map
@@ -245,6 +245,8 @@ EXPERIMENTAL {
 	rte_eth_dev_owner_set;
 	rte_eth_dev_owner_unset;
 	rte_eth_dev_rx_intr_ctl_q_get_fd;
+	rte_eth_find_next_of;
+	rte_eth_find_next_sibling;
 	rte_eth_switch_domain_alloc;
 	rte_eth_switch_domain_free;
 	rte_flow_conv;
-- 
2.20.1

^ permalink raw reply	[flat|nested] 41+ messages in thread

* [PATCH v2 3/4] net/mlx5: use port sibling iterators
  2019-02-20 22:10 ` [PATCH v2 0/4] ethdev iterators for multi-ports device Thomas Monjalon
  2019-02-20 22:10   ` [PATCH v2 1/4] ethdev: simplify port state comparisons Thomas Monjalon
  2019-02-20 22:10   ` [PATCH v2 2/4] ethdev: add siblings iterators Thomas Monjalon
@ 2019-02-20 22:10   ` Thomas Monjalon
  2019-02-20 22:10   ` [PATCH v2 4/4] app/testpmd: use port sibling iterator in device cleanup Thomas Monjalon
  3 siblings, 0 replies; 41+ messages in thread
From: Thomas Monjalon @ 2019-02-20 22:10 UTC (permalink / raw)
  To: Shahaf Shuler, Yongseok Koh; +Cc: dev

Iterating over siblings was done with RTE_ETH_FOREACH_DEV()
which skips the owned ports.
The new iterators RTE_ETH_FOREACH_DEV_SIBLING()
and RTE_ETH_FOREACH_DEV_OF() are more appropriate and more correct.

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
 drivers/net/mlx5/mlx5.c        | 34 +++++++++++++---------------------
 drivers/net/mlx5/mlx5_ethdev.c |  6 +-----
 2 files changed, 14 insertions(+), 26 deletions(-)

diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
index a913a5955f..6ed1ea0d5b 100644
--- a/drivers/net/mlx5/mlx5.c
+++ b/drivers/net/mlx5/mlx5.c
@@ -330,17 +330,15 @@ mlx5_dev_close(struct rte_eth_dev *dev)
 			dev->data->port_id);
 	if (priv->domain_id != RTE_ETH_DEV_SWITCH_DOMAIN_ID_INVALID) {
 		unsigned int c = 0;
-		unsigned int i = mlx5_dev_to_port_id(dev->device, NULL, 0);
-		uint16_t port_id[i];
+		uint16_t port_id;
 
-		i = RTE_MIN(mlx5_dev_to_port_id(dev->device, port_id, i), i);
-		while (i--) {
+		RTE_ETH_FOREACH_DEV_OF(port_id, dev->device) {
 			struct priv *opriv =
-				rte_eth_devices[port_id[i]].data->dev_private;
+				rte_eth_devices[port_id].data->dev_private;
 
 			if (!opriv ||
 			    opriv->domain_id != priv->domain_id ||
-			    &rte_eth_devices[port_id[i]] == dev)
+			    &rte_eth_devices[port_id] == dev)
 				continue;
 			++c;
 		}
@@ -1001,22 +999,16 @@ mlx5_dev_spawn(struct rte_device *dpdk_dev,
 	 * Look for sibling devices in order to reuse their switch domain
 	 * if any, otherwise allocate one.
 	 */
-	i = mlx5_dev_to_port_id(dpdk_dev, NULL, 0);
-	if (i > 0) {
-		uint16_t port_id[i];
+	RTE_ETH_FOREACH_DEV_OF(port_id, dpdk_dev) {
+		const struct priv *opriv =
+			rte_eth_devices[port_id].data->dev_private;
 
-		i = RTE_MIN(mlx5_dev_to_port_id(dpdk_dev, port_id, i), i);
-		while (i--) {
-			const struct priv *opriv =
-				rte_eth_devices[port_id[i]].data->dev_private;
-
-			if (!opriv ||
-			    opriv->domain_id ==
-			    RTE_ETH_DEV_SWITCH_DOMAIN_ID_INVALID)
-				continue;
-			priv->domain_id = opriv->domain_id;
-			break;
-		}
+		if (!opriv ||
+			opriv->domain_id ==
+			RTE_ETH_DEV_SWITCH_DOMAIN_ID_INVALID)
+			continue;
+		priv->domain_id = opriv->domain_id;
+		break;
 	}
 	if (priv->domain_id == RTE_ETH_DEV_SWITCH_DOMAIN_ID_INVALID) {
 		err = rte_eth_switch_domain_alloc(&priv->domain_id);
diff --git a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c
index d178ed6a18..039582a321 100644
--- a/drivers/net/mlx5/mlx5_ethdev.c
+++ b/drivers/net/mlx5/mlx5_ethdev.c
@@ -1300,11 +1300,7 @@ mlx5_dev_to_port_id(const struct rte_device *dev, uint16_t *port_list,
 	uint16_t id;
 	unsigned int n = 0;
 
-	RTE_ETH_FOREACH_DEV(id) {
-		struct rte_eth_dev *ldev = &rte_eth_devices[id];
-
-		if (ldev->device != dev)
-			continue;
+	RTE_ETH_FOREACH_DEV_OF(id, dev) {
 		if (n < port_list_n)
 			port_list[n] = id;
 		n++;
-- 
2.20.1

^ permalink raw reply	[flat|nested] 41+ messages in thread

* [PATCH v2 4/4] app/testpmd: use port sibling iterator in device cleanup
  2019-02-20 22:10 ` [PATCH v2 0/4] ethdev iterators for multi-ports device Thomas Monjalon
                     ` (2 preceding siblings ...)
  2019-02-20 22:10   ` [PATCH v2 3/4] net/mlx5: use port sibling iterators Thomas Monjalon
@ 2019-02-20 22:10   ` Thomas Monjalon
  3 siblings, 0 replies; 41+ messages in thread
From: Thomas Monjalon @ 2019-02-20 22:10 UTC (permalink / raw)
  To: Wenzhuo Lu, Jingjing Wu, Bernard Iremonger; +Cc: dev

When removing a rte_device on a port-based request,
all the sibling ports must be marked as closed.
The iterator loop can be simplified by using the dedicated macro.

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
 app/test-pmd/testpmd.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 98c1baa8b1..fcc479aa39 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -2361,9 +2361,7 @@ detach_port_device(portid_t port_id)
 		return;
 	}
 
-	for (sibling = 0; sibling < RTE_MAX_ETHPORTS; sibling++) {
-		if (rte_eth_devices[sibling].device != dev)
-			continue;
+	RTE_ETH_FOREACH_DEV_SIBLING(sibling, port_id) {
 		/* reset mapping between old ports and removed device */
 		rte_eth_devices[sibling].device = NULL;
 		if (ports[sibling].port_status != RTE_PORT_CLOSED) {
-- 
2.20.1

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH v2 1/4] ethdev: simplify port state comparisons
  2019-02-20 22:10   ` [PATCH v2 1/4] ethdev: simplify port state comparisons Thomas Monjalon
@ 2019-02-24 17:18     ` Andrew Rybchenko
  0 siblings, 0 replies; 41+ messages in thread
From: Andrew Rybchenko @ 2019-02-24 17:18 UTC (permalink / raw)
  To: Thomas Monjalon, Ferruh Yigit; +Cc: dev

On 2/21/19 1:10 AM, Thomas Monjalon wrote:
> There are three states for an ethdev port.
> Checking that the port is unused looks simpler than
> checking it is neither attached nor removed.
>
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>

It is not always equivalent (if/when more states added), but I think
comparison to RTE_ETH_DEV_UNUSED is really better here.

Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH v2 2/4] ethdev: add siblings iterators
  2019-02-20 22:10   ` [PATCH v2 2/4] ethdev: add siblings iterators Thomas Monjalon
@ 2019-02-24 17:22     ` Andrew Rybchenko
  2019-02-27 10:07     ` Gaëtan Rivet
  2019-03-19 15:47     ` Ferruh Yigit
  2 siblings, 0 replies; 41+ messages in thread
From: Andrew Rybchenko @ 2019-02-24 17:22 UTC (permalink / raw)
  To: Thomas Monjalon, Ferruh Yigit; +Cc: dev

On 2/21/19 1:10 AM, Thomas Monjalon wrote:
> If multiple ports share the same hardware device (rte_device),
> they are siblings and can be found thanks to the new functions
> and loop macros.
> One iterator takes a port id as reference,
> while the other one directly refers to the parent device.
>
> The ownership is not checked because siblings may have
> different owners.
>
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>

LGTM

Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH v2 2/4] ethdev: add siblings iterators
  2019-02-20 22:10   ` [PATCH v2 2/4] ethdev: add siblings iterators Thomas Monjalon
  2019-02-24 17:22     ` Andrew Rybchenko
@ 2019-02-27 10:07     ` Gaëtan Rivet
  2019-02-27 10:51       ` Thomas Monjalon
  2019-03-19 15:47     ` Ferruh Yigit
  2 siblings, 1 reply; 41+ messages in thread
From: Gaëtan Rivet @ 2019-02-27 10:07 UTC (permalink / raw)
  To: Thomas Monjalon; +Cc: Ferruh Yigit, Andrew Rybchenko, dev

Hi Thomas,

On Wed, Feb 20, 2019 at 11:10:49PM +0100, Thomas Monjalon wrote:
> If multiple ports share the same hardware device (rte_device),
> they are siblings and can be found thanks to the new functions
> and loop macros.
> One iterator takes a port id as reference,
> while the other one directly refers to the parent device.
> 
> The ownership is not checked because siblings may have
> different owners.
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> ---
>  lib/librte_ethdev/rte_ethdev.c           | 20 +++++++++++
>  lib/librte_ethdev/rte_ethdev.h           | 46 ++++++++++++++++++++++++
>  lib/librte_ethdev/rte_ethdev_version.map |  2 ++
>  3 files changed, 68 insertions(+)
> 
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index b3b2fb1dba..42154787f8 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -340,6 +340,26 @@ rte_eth_find_next(uint16_t port_id)
>  	return port_id;
>  }
>  
> +uint16_t __rte_experimental
> +rte_eth_find_next_of(uint16_t port_id, const struct rte_device *parent)
> +{
> +	while (port_id < RTE_MAX_ETHPORTS &&
> +			rte_eth_devices[port_id].state == RTE_ETH_DEV_UNUSED &&
> +			rte_eth_devices[port_id].device != parent)
> +		port_id++;

Why not call rte_eth_find_next directly from this function, and
add your specific test on top of it?

Something like:

    	while (port_id < RTE_MAX_ETHPORTS &&
    	       rte_eth_devices[port_id].device != parent)
    		port_id = rte_eth_find_next(port_id + 1);

this way you won't have to rewrite the test on the device state. Having the
logic expressed in several places would make reworking the device states more
complicated than necessary if it were to happen (just as you did when switching
the test from !(ATTACHED || REMOVED) to (UNUSED).

-- 
Gaëtan Rivet
6WIND

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH v2 2/4] ethdev: add siblings iterators
  2019-02-27 10:07     ` Gaëtan Rivet
@ 2019-02-27 10:51       ` Thomas Monjalon
  2019-04-01  1:59         ` Thomas Monjalon
  0 siblings, 1 reply; 41+ messages in thread
From: Thomas Monjalon @ 2019-02-27 10:51 UTC (permalink / raw)
  To: Gaëtan Rivet; +Cc: Ferruh Yigit, Andrew Rybchenko, dev

27/02/2019 11:07, Gaëtan Rivet:
> Hi Thomas,
> 
> On Wed, Feb 20, 2019 at 11:10:49PM +0100, Thomas Monjalon wrote:
> > If multiple ports share the same hardware device (rte_device),
> > they are siblings and can be found thanks to the new functions
> > and loop macros.
> > One iterator takes a port id as reference,
> > while the other one directly refers to the parent device.
> > 
> > The ownership is not checked because siblings may have
> > different owners.
> > 
> > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> > ---
> >  lib/librte_ethdev/rte_ethdev.c           | 20 +++++++++++
> >  lib/librte_ethdev/rte_ethdev.h           | 46 ++++++++++++++++++++++++
> >  lib/librte_ethdev/rte_ethdev_version.map |  2 ++
> >  3 files changed, 68 insertions(+)
> > 
> > diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> > index b3b2fb1dba..42154787f8 100644
> > --- a/lib/librte_ethdev/rte_ethdev.c
> > +++ b/lib/librte_ethdev/rte_ethdev.c
> > @@ -340,6 +340,26 @@ rte_eth_find_next(uint16_t port_id)
> >  	return port_id;
> >  }
> >  
> > +uint16_t __rte_experimental
> > +rte_eth_find_next_of(uint16_t port_id, const struct rte_device *parent)
> > +{
> > +	while (port_id < RTE_MAX_ETHPORTS &&
> > +			rte_eth_devices[port_id].state == RTE_ETH_DEV_UNUSED &&
> > +			rte_eth_devices[port_id].device != parent)
> > +		port_id++;
> 
> Why not call rte_eth_find_next directly from this function, and
> add your specific test on top of it?
> 
> Something like:
> 
>     	while (port_id < RTE_MAX_ETHPORTS &&
>     	       rte_eth_devices[port_id].device != parent)
>     		port_id = rte_eth_find_next(port_id + 1);
> 
> this way you won't have to rewrite the test on the device state. Having the
> logic expressed in several places would make reworking the device states more
> complicated than necessary if it were to happen (just as you did when switching
> the test from !(ATTACHED || REMOVED) to (UNUSED).

About the intent, you are right.
About the solution, it seems buggy. We can try to find another way
of coding this loop by using rte_eth_find_next()
and adding the parent condition.

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH v2 2/4] ethdev: add siblings iterators
  2019-02-20 22:10   ` [PATCH v2 2/4] ethdev: add siblings iterators Thomas Monjalon
  2019-02-24 17:22     ` Andrew Rybchenko
  2019-02-27 10:07     ` Gaëtan Rivet
@ 2019-03-19 15:47     ` Ferruh Yigit
  2019-03-19 17:34       ` Thomas Monjalon
  2 siblings, 1 reply; 41+ messages in thread
From: Ferruh Yigit @ 2019-03-19 15:47 UTC (permalink / raw)
  To: Thomas Monjalon, Andrew Rybchenko; +Cc: dev

On 2/20/2019 10:10 PM, Thomas Monjalon wrote:
> If multiple ports share the same hardware device (rte_device),
> they are siblings and can be found thanks to the new functions
> and loop macros.
> One iterator takes a port id as reference,
> while the other one directly refers to the parent device.
> 
> The ownership is not checked because siblings may have
> different owners.
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> ---
>  lib/librte_ethdev/rte_ethdev.c           | 20 +++++++++++
>  lib/librte_ethdev/rte_ethdev.h           | 46 ++++++++++++++++++++++++
>  lib/librte_ethdev/rte_ethdev_version.map |  2 ++
>  3 files changed, 68 insertions(+)
> 
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index b3b2fb1dba..42154787f8 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -340,6 +340,26 @@ rte_eth_find_next(uint16_t port_id)
>  	return port_id;
>  }
>  
> +uint16_t __rte_experimental

Do we need _rte_experimental on function definitions? I guess only in .h file,
function declaration is enough.

> +rte_eth_find_next_of(uint16_t port_id, const struct rte_device *parent)

Out of curiosity, what '_of' refers to?

> +{
> +	while (port_id < RTE_MAX_ETHPORTS &&
> +			rte_eth_devices[port_id].state == RTE_ETH_DEV_UNUSED &&
> +			rte_eth_devices[port_id].device != parent)
> +		port_id++;

Is this logic correct, or am I missing something.
When port status is ATTACHED, check will return false and exit from loop without
checking if the 'device' is same.
+1 to Gaetan's suggestion to use 'rte_eth_find_next()', which moves status
concern to that function.

> +
> +	if (port_id >= RTE_MAX_ETHPORTS)
> +		return RTE_MAX_ETHPORTS;
> +
> +	return port_id;
> +}
> +
> +uint16_t __rte_experimental
> +rte_eth_find_next_sibling(uint16_t port_id, uint16_t ref)

I think better to say 'ref_port_id' to clarify what we expect here is a port_id

> +{
> +	return rte_eth_find_next_of(port_id, rte_eth_devices[ref].device);

This is a public API, shouldn't we check if 'ref' if valid port_id value, before
accessing the '.device' field?

> +}
> +
>  static void
>  rte_eth_dev_shared_data_prepare(void)
>  {
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index a3c864a134..a7c5c36277 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -1383,6 +1383,52 @@ uint16_t rte_eth_find_next(uint16_t port_id);
>  #define RTE_ETH_FOREACH_DEV(p) \
>  	RTE_ETH_FOREACH_DEV_OWNED_BY(p, RTE_ETH_DEV_NO_OWNER)
>  
> +/**
> + * Iterates over ethdev ports of a specified device.
> + *
> + * @param port_id_start
> + *   The id of the next possible valid port.
> + * @param parent
> + *   The generic device behind the ports to iterate.
> + * @return
> + *   Next port id of the device, RTE_MAX_ETHPORTS if there is none.

Can return 'port_id_start', right? Should it be documented as it is done in
below 'next_sibling' one?

> + */
> +__rte_experimental
> +uint16_t rte_eth_find_next_of(uint16_t port_id_start,
> +		const struct rte_device *parent);
> +
> +/**
> + * Macro to iterate over all ethdev ports sharing the same rte_device
> + * as the specified port.

'specified port'? No port specified, a device pointer is specified.

> + * Note: the specified port is part of the loop iterations.
> + */

Does it make sense to clarify what 'p' is and what 'parent' is as we do in
function doxygen comments? Since these are macros, harder to grasp the types, I
think better to describe more in macro documentation.

> +#define RTE_ETH_FOREACH_DEV_OF(p, parent) \
> +	for (p = rte_eth_find_next_of(0, parent); \
> +		p < RTE_MAX_ETHPORTS; \
> +		p = rte_eth_find_next_of(p + 1, parent))
> +
> +/**
> + * Iterates over sibling ethdev ports (i.e. sharing the same rte_device).
> + *
> + * @param port_id_start
> + *   The id of the next possible valid sibling port.
> + * @param ref
> + *   The id of a reference port to compare rte_device with.
> + * @return
> + *   Next sibling port id (or ref itself), RTE_MAX_ETHPORTS if there is none.
> + */
> +__rte_experimental
> +uint16_t rte_eth_find_next_sibling(uint16_t port_id_start, uint16_t ref);
> +
> +/**
> + * Macro to iterate over all ethdev ports sharing the same rte_device
> + * as the specified port.
> + * Note: the specified port is part of the loop iterations.
> + */
> +#define RTE_ETH_FOREACH_DEV_SIBLING(p, ref) \
> +	for (p = rte_eth_find_next_sibling(0, ref); \
> +		p < RTE_MAX_ETHPORTS; \
> +		p = rte_eth_find_next_sibling(p + 1, ref))
>  
>  /**
>   * @warning
> diff --git a/lib/librte_ethdev/rte_ethdev_version.map b/lib/librte_ethdev/rte_ethdev_version.map
> index 92ac3de250..b37a4167d7 100644
> --- a/lib/librte_ethdev/rte_ethdev_version.map
> +++ b/lib/librte_ethdev/rte_ethdev_version.map
> @@ -245,6 +245,8 @@ EXPERIMENTAL {
>  	rte_eth_dev_owner_set;
>  	rte_eth_dev_owner_unset;
>  	rte_eth_dev_rx_intr_ctl_q_get_fd;
> +	rte_eth_find_next_of;
> +	rte_eth_find_next_sibling;
>  	rte_eth_switch_domain_alloc;
>  	rte_eth_switch_domain_free;
>  	rte_flow_conv;
> 

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH v2 2/4] ethdev: add siblings iterators
  2019-03-19 15:47     ` Ferruh Yigit
@ 2019-03-19 17:34       ` Thomas Monjalon
  2019-03-19 18:04         ` Ferruh Yigit
  0 siblings, 1 reply; 41+ messages in thread
From: Thomas Monjalon @ 2019-03-19 17:34 UTC (permalink / raw)
  To: Ferruh Yigit; +Cc: Andrew Rybchenko, dev

19/03/2019 16:47, Ferruh Yigit:
> On 2/20/2019 10:10 PM, Thomas Monjalon wrote:
> > If multiple ports share the same hardware device (rte_device),
> > they are siblings and can be found thanks to the new functions
> > and loop macros.
> > One iterator takes a port id as reference,
> > while the other one directly refers to the parent device.
> > 
> > The ownership is not checked because siblings may have
> > different owners.
> > 
> > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> > ---
> >  lib/librte_ethdev/rte_ethdev.c           | 20 +++++++++++
> >  lib/librte_ethdev/rte_ethdev.h           | 46 ++++++++++++++++++++++++
> >  lib/librte_ethdev/rte_ethdev_version.map |  2 ++
> >  3 files changed, 68 insertions(+)
> > 
> > diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> > index b3b2fb1dba..42154787f8 100644
> > --- a/lib/librte_ethdev/rte_ethdev.c
> > +++ b/lib/librte_ethdev/rte_ethdev.c
> > @@ -340,6 +340,26 @@ rte_eth_find_next(uint16_t port_id)
> >  	return port_id;
> >  }
> >  
> > +uint16_t __rte_experimental
> 
> Do we need _rte_experimental on function definitions? I guess only in .h file,
> function declaration is enough.

Yes we need them both in .h and .c files.

> > +rte_eth_find_next_of(uint16_t port_id, const struct rte_device *parent)
> 
> Out of curiosity, what '_of' refers to?

It means "next port of parent".
Is there a better word than "of"?

> > +{
> > +	while (port_id < RTE_MAX_ETHPORTS &&
> > +			rte_eth_devices[port_id].state == RTE_ETH_DEV_UNUSED &&
> > +			rte_eth_devices[port_id].device != parent)
> > +		port_id++;
> 
> Is this logic correct, or am I missing something.
> When port status is ATTACHED, check will return false and exit from loop without
> checking if the 'device' is same.

Oops, I think you are right.

> +1 to Gaetan's suggestion to use 'rte_eth_find_next()', which moves status
> concern to that function.

OK, will change.

> > +
> > +	if (port_id >= RTE_MAX_ETHPORTS)
> > +		return RTE_MAX_ETHPORTS;
> > +
> > +	return port_id;
> > +}
> > +
> > +uint16_t __rte_experimental
> > +rte_eth_find_next_sibling(uint16_t port_id, uint16_t ref)
> 
> I think better to say 'ref_port_id' to clarify what we expect here is a port_id

OK

> > +{
> > +	return rte_eth_find_next_of(port_id, rte_eth_devices[ref].device);
> 
> This is a public API, shouldn't we check if 'ref' if valid port_id value, before
> accessing the '.device' field?

I'm not a fan of extra checks, but yes we may add one.

> > --- a/lib/librte_ethdev/rte_ethdev.h
> > +++ b/lib/librte_ethdev/rte_ethdev.h
> > +/**
> > + * Iterates over ethdev ports of a specified device.
> > + *
> > + * @param port_id_start
> > + *   The id of the next possible valid port.
> > + * @param parent
> > + *   The generic device behind the ports to iterate.
> > + * @return
> > + *   Next port id of the device, RTE_MAX_ETHPORTS if there is none.
> 
> Can return 'port_id_start', right? Should it be documented as it is done in
> below 'next_sibling' one?

Yes, OK.

> > + */
> > +__rte_experimental
> > +uint16_t rte_eth_find_next_of(uint16_t port_id_start,
> > +		const struct rte_device *parent);
> > +
> > +/**
> > + * Macro to iterate over all ethdev ports sharing the same rte_device
> > + * as the specified port.
> 
> 'specified port'? No port specified, a device pointer is specified.

Right, looks like a wrong a copy/paste.

> > + * Note: the specified port is part of the loop iterations.
> > + */
> 
> Does it make sense to clarify what 'p' is and what 'parent' is as we do in
> function doxygen comments? Since these are macros, harder to grasp the types, I
> think better to describe more in macro documentation.

Absolutely, yes. I thought I did it.

> > +#define RTE_ETH_FOREACH_DEV_OF(p, parent) \
> > +	for (p = rte_eth_find_next_of(0, parent); \
> > +		p < RTE_MAX_ETHPORTS; \
> > +		p = rte_eth_find_next_of(p + 1, parent))
> > +
> > +/**
> > + * Iterates over sibling ethdev ports (i.e. sharing the same rte_device).
> > + *
> > + * @param port_id_start
> > + *   The id of the next possible valid sibling port.
> > + * @param ref
> > + *   The id of a reference port to compare rte_device with.
> > + * @return
> > + *   Next sibling port id (or ref itself), RTE_MAX_ETHPORTS if there is none.
> > + */
> > +__rte_experimental
> > +uint16_t rte_eth_find_next_sibling(uint16_t port_id_start, uint16_t ref);
> > +
> > +/**
> > + * Macro to iterate over all ethdev ports sharing the same rte_device
> > + * as the specified port.
> > + * Note: the specified port is part of the loop iterations.
> > + */
> > +#define RTE_ETH_FOREACH_DEV_SIBLING(p, ref) \
> > +	for (p = rte_eth_find_next_sibling(0, ref); \
> > +		p < RTE_MAX_ETHPORTS; \
> > +		p = rte_eth_find_next_sibling(p + 1, ref))

Thanks for the complete review Ferruh.

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH v2 2/4] ethdev: add siblings iterators
  2019-03-19 17:34       ` Thomas Monjalon
@ 2019-03-19 18:04         ` Ferruh Yigit
  2019-04-01  2:16           ` Thomas Monjalon
  0 siblings, 1 reply; 41+ messages in thread
From: Ferruh Yigit @ 2019-03-19 18:04 UTC (permalink / raw)
  To: Thomas Monjalon; +Cc: Andrew Rybchenko, dev

On 3/19/2019 5:34 PM, Thomas Monjalon wrote:
>>> +uint16_t __rte_experimental
>> Do we need _rte_experimental on function definitions? I guess only in .h file,
>> function declaration is enough.
> Yes we need them both in .h and .c files.
> 

Why we need them in .c file?
I think the compiler is interested in ones in .h file, because of the
experimental checks.

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH v2 2/4] ethdev: add siblings iterators
  2019-02-27 10:51       ` Thomas Monjalon
@ 2019-04-01  1:59         ` Thomas Monjalon
  0 siblings, 0 replies; 41+ messages in thread
From: Thomas Monjalon @ 2019-04-01  1:59 UTC (permalink / raw)
  To: Gaëtan Rivet; +Cc: dev, Ferruh Yigit, Andrew Rybchenko

27/02/2019 11:51, Thomas Monjalon:
> 27/02/2019 11:07, Gaëtan Rivet:
> > On Wed, Feb 20, 2019 at 11:10:49PM +0100, Thomas Monjalon wrote:
> > > +uint16_t __rte_experimental
> > > +rte_eth_find_next_of(uint16_t port_id, const struct rte_device *parent)
> > > +{
> > > +	while (port_id < RTE_MAX_ETHPORTS &&
> > > +			rte_eth_devices[port_id].state == RTE_ETH_DEV_UNUSED &&
> > > +			rte_eth_devices[port_id].device != parent)
> > > +		port_id++;
> > 
> > Why not call rte_eth_find_next directly from this function, and
> > add your specific test on top of it?
> > 
> > Something like:
> > 
> >     	while (port_id < RTE_MAX_ETHPORTS &&
> >     	       rte_eth_devices[port_id].device != parent)
> >     		port_id = rte_eth_find_next(port_id + 1);
> > 
> > this way you won't have to rewrite the test on the device state. Having the
> > logic expressed in several places would make reworking the device states more
> > complicated than necessary if it were to happen (just as you did when switching
> > the test from !(ATTACHED || REMOVED) to (UNUSED).
> 
> About the intent, you are right.
> About the solution, it seems buggy. We can try to find another way
> of coding this loop by using rte_eth_find_next()
> and adding the parent condition.

Your proposal is correct if adding a first call before the loop:
	port_id = rte_eth_find_next(port_id);

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH v2 2/4] ethdev: add siblings iterators
  2019-03-19 18:04         ` Ferruh Yigit
@ 2019-04-01  2:16           ` Thomas Monjalon
  2019-04-01  6:46             ` David Marchand
  0 siblings, 1 reply; 41+ messages in thread
From: Thomas Monjalon @ 2019-04-01  2:16 UTC (permalink / raw)
  To: Ferruh Yigit; +Cc: dev, Andrew Rybchenko, david.marchand

19/03/2019 19:04, Ferruh Yigit:
> On 3/19/2019 5:34 PM, Thomas Monjalon wrote:
> >>> +uint16_t __rte_experimental
> >> 
> >> Do we need _rte_experimental on function definitions? I guess only in .h file,
> >> function declaration is enough.
> > 
> > Yes we need them both in .h and .c files.
> 
> Why we need them in .c file?
> I think the compiler is interested in ones in .h file, because of the
> experimental checks.

We need the tag in .c file because a check is done in the ELF object
by buildtools/check-experimental-syms.sh

David tried a replacement of this script to run on header files,
but it looks a bit slow:
	https://patches.dpdk.org/patch/49118/

^ permalink raw reply	[flat|nested] 41+ messages in thread

* [PATCH v3 0/4] ethdev iterators for multi-ports device
  2018-11-30  0:27 [PATCH] ethdev: add siblings iterator Thomas Monjalon
  2018-12-11 16:31 ` Ferruh Yigit
  2019-02-20 22:10 ` [PATCH v2 0/4] ethdev iterators for multi-ports device Thomas Monjalon
@ 2019-04-01  2:26 ` Thomas Monjalon
  2019-04-01  2:26   ` [PATCH v3 1/4] ethdev: simplify port state comparisons Thomas Monjalon
                     ` (4 more replies)
  2 siblings, 5 replies; 41+ messages in thread
From: Thomas Monjalon @ 2019-04-01  2:26 UTC (permalink / raw)
  To: gaetan.rivet; +Cc: dev

Add port iterators in order to allow listing easily
the ports of the same device.

The iterators can be tested by using mlx5 or testpmd.


v3: changes only in the (main) patch 2


Thomas Monjalon (4):
  ethdev: simplify port state comparisons
  ethdev: add siblings iterators
  net/mlx5: use port sibling iterators
  app/testpmd: use port sibling iterator in device cleanup

 app/test-pmd/testpmd.c                   |  4 +-
 drivers/net/mlx5/mlx5.c                  | 34 +++++--------
 drivers/net/mlx5/mlx5_ethdev.c           |  6 +--
 lib/librte_ethdev/rte_ethdev.c           | 25 ++++++++--
 lib/librte_ethdev/rte_ethdev.h           | 63 ++++++++++++++++++++++++
 lib/librte_ethdev/rte_ethdev_version.map |  2 +
 6 files changed, 101 insertions(+), 33 deletions(-)

-- 
2.21.0

^ permalink raw reply	[flat|nested] 41+ messages in thread

* [PATCH v3 1/4] ethdev: simplify port state comparisons
  2019-04-01  2:26 ` [PATCH v3 0/4] ethdev iterators for multi-ports device Thomas Monjalon
@ 2019-04-01  2:26   ` Thomas Monjalon
  2019-04-01 14:58     ` Stephen Hemminger
  2019-04-03 15:03     ` Slava Ovsiienko
  2019-04-01  2:26   ` [PATCH v3 2/4] ethdev: add siblings iterators Thomas Monjalon
                     ` (3 subsequent siblings)
  4 siblings, 2 replies; 41+ messages in thread
From: Thomas Monjalon @ 2019-04-01  2:26 UTC (permalink / raw)
  To: gaetan.rivet, Ferruh Yigit, Andrew Rybchenko; +Cc: dev

There are three states for an ethdev port.
Checking that the port is unused looks simpler than
checking it is neither attached nor removed.

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>
---
 lib/librte_ethdev/rte_ethdev.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 10bdfb37e..33cffc498 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -330,8 +330,7 @@ uint16_t
 rte_eth_find_next(uint16_t port_id)
 {
 	while (port_id < RTE_MAX_ETHPORTS &&
-	       rte_eth_devices[port_id].state != RTE_ETH_DEV_ATTACHED &&
-	       rte_eth_devices[port_id].state != RTE_ETH_DEV_REMOVED)
+			rte_eth_devices[port_id].state == RTE_ETH_DEV_UNUSED)
 		port_id++;
 
 	if (port_id >= RTE_MAX_ETHPORTS)
@@ -567,8 +566,7 @@ uint64_t
 rte_eth_find_next_owned_by(uint16_t port_id, const uint64_t owner_id)
 {
 	while (port_id < RTE_MAX_ETHPORTS &&
-	       ((rte_eth_devices[port_id].state != RTE_ETH_DEV_ATTACHED &&
-	       rte_eth_devices[port_id].state != RTE_ETH_DEV_REMOVED) ||
+	       (rte_eth_devices[port_id].state == RTE_ETH_DEV_UNUSED ||
 	       rte_eth_devices[port_id].data->owner.id != owner_id))
 		port_id++;
 
-- 
2.21.0

^ permalink raw reply	[flat|nested] 41+ messages in thread

* [PATCH v3 2/4] ethdev: add siblings iterators
  2019-04-01  2:26 ` [PATCH v3 0/4] ethdev iterators for multi-ports device Thomas Monjalon
  2019-04-01  2:26   ` [PATCH v3 1/4] ethdev: simplify port state comparisons Thomas Monjalon
@ 2019-04-01  2:26   ` Thomas Monjalon
  2019-04-01  7:23     ` Andrew Rybchenko
                       ` (2 more replies)
  2019-04-01  2:26   ` [PATCH v3 3/4] net/mlx5: use port sibling iterators Thomas Monjalon
                     ` (2 subsequent siblings)
  4 siblings, 3 replies; 41+ messages in thread
From: Thomas Monjalon @ 2019-04-01  2:26 UTC (permalink / raw)
  To: gaetan.rivet, Ferruh Yigit, Andrew Rybchenko; +Cc: dev

If multiple ports share the same hardware device (rte_device),
they are siblings and can be found thanks to the new functions
and loop macros.
One iterator takes a port id as reference,
while the other one directly refers to the parent device.

The ownership is not checked because siblings may have
different owners.

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
v2: Reviewed-by: Andrew Rybchenko - not kept because of changes in v3

v3:
	- fix logic + re-use rte_eth_find_next()
	- longer parameter names
	- more and better doxygen comments
---
 lib/librte_ethdev/rte_ethdev.c           | 19 +++++++
 lib/librte_ethdev/rte_ethdev.h           | 63 ++++++++++++++++++++++++
 lib/librte_ethdev/rte_ethdev_version.map |  2 +
 3 files changed, 84 insertions(+)

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 33cffc498..3b125a642 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -339,6 +339,25 @@ rte_eth_find_next(uint16_t port_id)
 	return port_id;
 }
 
+uint16_t __rte_experimental
+rte_eth_find_next_of(uint16_t port_id, const struct rte_device *parent)
+{
+	port_id = rte_eth_find_next(port_id);
+	while (port_id < RTE_MAX_ETHPORTS &&
+			rte_eth_devices[port_id].device != parent)
+		port_id = rte_eth_find_next(port_id + 1);
+
+	return port_id;
+}
+
+uint16_t __rte_experimental
+rte_eth_find_next_sibling(uint16_t port_id, uint16_t ref_port_id)
+{
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(ref_port_id, RTE_MAX_ETHPORTS);
+	return rte_eth_find_next_of(port_id,
+			rte_eth_devices[ref_port_id].device);
+}
+
 static void
 rte_eth_dev_shared_data_prepare(void)
 {
diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index b6023c050..3d5bacaee 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -1387,6 +1387,69 @@ uint16_t rte_eth_find_next(uint16_t port_id);
 #define RTE_ETH_FOREACH_DEV(p) \
 	RTE_ETH_FOREACH_DEV_OWNED_BY(p, RTE_ETH_DEV_NO_OWNER)
 
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice.
+ *
+ * Iterates over ethdev ports of a specified device.
+ *
+ * @param port_id_start
+ *   The id of the next possible valid port.
+ * @param parent
+ *   The generic device behind the ports to iterate.
+ * @return
+ *   Next port id of the device, possibly port_id_start,
+ *   RTE_MAX_ETHPORTS if there is none.
+ */
+__rte_experimental
+uint16_t rte_eth_find_next_of(uint16_t port_id_start,
+		const struct rte_device *parent);
+
+/**
+ * Macro to iterate over all ethdev ports of a specified device.
+ *
+ * @param port_id
+ *   The id of the matching port being iterated.
+ * @param parent
+ *   The rte_device pointer matching the iterated ports.
+ */
+#define RTE_ETH_FOREACH_DEV_OF(port_id, parent) \
+	for (port_id = rte_eth_find_next_of(0, parent); \
+		port_id < RTE_MAX_ETHPORTS; \
+		port_id = rte_eth_find_next_of(port_id + 1, parent))
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice.
+ *
+ * Iterates over sibling ethdev ports (i.e. sharing the same rte_device).
+ *
+ * @param port_id_start
+ *   The id of the next possible valid sibling port.
+ * @param ref_port_id
+ *   The id of a reference port to compare rte_device with.
+ * @return
+ *   Next sibling port id, possibly port_id_start or ref_port_id itself,
+ *   RTE_MAX_ETHPORTS if there is none.
+ */
+__rte_experimental
+uint16_t rte_eth_find_next_sibling(uint16_t port_id_start,
+		uint16_t ref_port_id);
+
+/**
+ * Macro to iterate over all ethdev ports sharing the same rte_device
+ * as the specified port.
+ * Note: the specified reference port is part of the loop iterations.
+ *
+ * @param port_id
+ *   The id of the matching port being iterated.
+ * @param ref_port_id
+ *   The id of the port being compared.
+ */
+#define RTE_ETH_FOREACH_DEV_SIBLING(port_id, ref_port_id) \
+	for (port_id = rte_eth_find_next_sibling(0, ref_port_id); \
+		port_id < RTE_MAX_ETHPORTS; \
+		port_id = rte_eth_find_next_sibling(port_id + 1, ref_port_id))
 
 /**
  * @warning
diff --git a/lib/librte_ethdev/rte_ethdev_version.map b/lib/librte_ethdev/rte_ethdev_version.map
index 92ac3de25..b37a4167d 100644
--- a/lib/librte_ethdev/rte_ethdev_version.map
+++ b/lib/librte_ethdev/rte_ethdev_version.map
@@ -245,6 +245,8 @@ EXPERIMENTAL {
 	rte_eth_dev_owner_set;
 	rte_eth_dev_owner_unset;
 	rte_eth_dev_rx_intr_ctl_q_get_fd;
+	rte_eth_find_next_of;
+	rte_eth_find_next_sibling;
 	rte_eth_switch_domain_alloc;
 	rte_eth_switch_domain_free;
 	rte_flow_conv;
-- 
2.21.0

^ permalink raw reply	[flat|nested] 41+ messages in thread

* [PATCH v3 3/4] net/mlx5: use port sibling iterators
  2019-04-01  2:26 ` [PATCH v3 0/4] ethdev iterators for multi-ports device Thomas Monjalon
  2019-04-01  2:26   ` [PATCH v3 1/4] ethdev: simplify port state comparisons Thomas Monjalon
  2019-04-01  2:26   ` [PATCH v3 2/4] ethdev: add siblings iterators Thomas Monjalon
@ 2019-04-01  2:26   ` Thomas Monjalon
  2019-04-03 14:19     ` Ferruh Yigit
  2019-04-03 15:04     ` Slava Ovsiienko
  2019-04-01  2:27   ` [PATCH v3 4/4] app/testpmd: use port sibling iterator in device cleanup Thomas Monjalon
  2019-04-03 16:42   ` [PATCH v3 0/4] ethdev iterators for multi-ports device Ferruh Yigit
  4 siblings, 2 replies; 41+ messages in thread
From: Thomas Monjalon @ 2019-04-01  2:26 UTC (permalink / raw)
  To: gaetan.rivet, Shahaf Shuler, Yongseok Koh; +Cc: dev

Iterating over siblings was done with RTE_ETH_FOREACH_DEV()
which skips the owned ports.
The new iterators RTE_ETH_FOREACH_DEV_SIBLING()
and RTE_ETH_FOREACH_DEV_OF() are more appropriate and more correct.

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
 drivers/net/mlx5/mlx5.c        | 34 +++++++++++++---------------------
 drivers/net/mlx5/mlx5_ethdev.c |  6 +-----
 2 files changed, 14 insertions(+), 26 deletions(-)

diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
index 1d7ca615b..3287a3d78 100644
--- a/drivers/net/mlx5/mlx5.c
+++ b/drivers/net/mlx5/mlx5.c
@@ -493,17 +493,15 @@ mlx5_dev_close(struct rte_eth_dev *dev)
 			dev->data->port_id);
 	if (priv->domain_id != RTE_ETH_DEV_SWITCH_DOMAIN_ID_INVALID) {
 		unsigned int c = 0;
-		unsigned int i = mlx5_dev_to_port_id(dev->device, NULL, 0);
-		uint16_t port_id[i];
+		uint16_t port_id;
 
-		i = RTE_MIN(mlx5_dev_to_port_id(dev->device, port_id, i), i);
-		while (i--) {
+		RTE_ETH_FOREACH_DEV_OF(port_id, dev->device) {
 			struct mlx5_priv *opriv =
-				rte_eth_devices[port_id[i]].data->dev_private;
+				rte_eth_devices[port_id].data->dev_private;
 
 			if (!opriv ||
 			    opriv->domain_id != priv->domain_id ||
-			    &rte_eth_devices[port_id[i]] == dev)
+			    &rte_eth_devices[port_id] == dev)
 				continue;
 			++c;
 		}
@@ -1147,22 +1145,16 @@ mlx5_dev_spawn(struct rte_device *dpdk_dev,
 	 * Look for sibling devices in order to reuse their switch domain
 	 * if any, otherwise allocate one.
 	 */
-	i = mlx5_dev_to_port_id(dpdk_dev, NULL, 0);
-	if (i > 0) {
-		uint16_t port_id[i];
+	RTE_ETH_FOREACH_DEV_OF(port_id, dpdk_dev) {
+		const struct mlx5_priv *opriv =
+			rte_eth_devices[port_id].data->dev_private;
 
-		i = RTE_MIN(mlx5_dev_to_port_id(dpdk_dev, port_id, i), i);
-		while (i--) {
-			const struct mlx5_priv *opriv =
-				rte_eth_devices[port_id[i]].data->dev_private;
-
-			if (!opriv ||
-			    opriv->domain_id ==
-			    RTE_ETH_DEV_SWITCH_DOMAIN_ID_INVALID)
-				continue;
-			priv->domain_id = opriv->domain_id;
-			break;
-		}
+		if (!opriv ||
+			opriv->domain_id ==
+			RTE_ETH_DEV_SWITCH_DOMAIN_ID_INVALID)
+			continue;
+		priv->domain_id = opriv->domain_id;
+		break;
 	}
 	if (priv->domain_id == RTE_ETH_DEV_SWITCH_DOMAIN_ID_INVALID) {
 		err = rte_eth_switch_domain_alloc(&priv->domain_id);
diff --git a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c
index 7273bd940..e01b698fc 100644
--- a/drivers/net/mlx5/mlx5_ethdev.c
+++ b/drivers/net/mlx5/mlx5_ethdev.c
@@ -1398,11 +1398,7 @@ mlx5_dev_to_port_id(const struct rte_device *dev, uint16_t *port_list,
 	uint16_t id;
 	unsigned int n = 0;
 
-	RTE_ETH_FOREACH_DEV(id) {
-		struct rte_eth_dev *ldev = &rte_eth_devices[id];
-
-		if (ldev->device != dev)
-			continue;
+	RTE_ETH_FOREACH_DEV_OF(id, dev) {
 		if (n < port_list_n)
 			port_list[n] = id;
 		n++;
-- 
2.21.0

^ permalink raw reply	[flat|nested] 41+ messages in thread

* [PATCH v3 4/4] app/testpmd: use port sibling iterator in device cleanup
  2019-04-01  2:26 ` [PATCH v3 0/4] ethdev iterators for multi-ports device Thomas Monjalon
                     ` (2 preceding siblings ...)
  2019-04-01  2:26   ` [PATCH v3 3/4] net/mlx5: use port sibling iterators Thomas Monjalon
@ 2019-04-01  2:27   ` Thomas Monjalon
  2019-04-02 23:43     ` Ferruh Yigit
  2019-04-03 15:04     ` Slava Ovsiienko
  2019-04-03 16:42   ` [PATCH v3 0/4] ethdev iterators for multi-ports device Ferruh Yigit
  4 siblings, 2 replies; 41+ messages in thread
From: Thomas Monjalon @ 2019-04-01  2:27 UTC (permalink / raw)
  To: gaetan.rivet, Wenzhuo Lu, Jingjing Wu, Bernard Iremonger; +Cc: dev

When removing a rte_device on a port-based request,
all the sibling ports must be marked as closed.
The iterator loop can be simplified by using the dedicated macro.

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
 app/test-pmd/testpmd.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 40c873b97..aeaa74c98 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -2370,9 +2370,7 @@ detach_port_device(portid_t port_id)
 		return;
 	}
 
-	for (sibling = 0; sibling < RTE_MAX_ETHPORTS; sibling++) {
-		if (rte_eth_devices[sibling].device != dev)
-			continue;
+	RTE_ETH_FOREACH_DEV_SIBLING(sibling, port_id) {
 		/* reset mapping between old ports and removed device */
 		rte_eth_devices[sibling].device = NULL;
 		if (ports[sibling].port_status != RTE_PORT_CLOSED) {
-- 
2.21.0

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH v2 2/4] ethdev: add siblings iterators
  2019-04-01  2:16           ` Thomas Monjalon
@ 2019-04-01  6:46             ` David Marchand
  2019-04-01  8:09               ` Thomas Monjalon
  0 siblings, 1 reply; 41+ messages in thread
From: David Marchand @ 2019-04-01  6:46 UTC (permalink / raw)
  To: Thomas Monjalon; +Cc: Ferruh Yigit, dev, Andrew Rybchenko

On Mon, Apr 1, 2019 at 4:16 AM Thomas Monjalon <thomas@monjalon.net> wrote:

> 19/03/2019 19:04, Ferruh Yigit:
> > On 3/19/2019 5:34 PM, Thomas Monjalon wrote:
> > >>> +uint16_t __rte_experimental
> > >>
> > >> Do we need _rte_experimental on function definitions? I guess only in
> .h file,
> > >> function declaration is enough.
> > >
> > > Yes we need them both in .h and .c files.
> >
> > Why we need them in .c file?
> > I think the compiler is interested in ones in .h file, because of the
> > experimental checks.
>
> We need the tag in .c file because a check is done in the ELF object
> by buildtools/check-experimental-syms.sh
>

?
The attribute should be inherited from the declaration in the header.
If you have a case where it does not work, I'd like to look at it.



> David tried a replacement of this script to run on header files,
> but it looks a bit slow:
>         https://patches.dpdk.org/patch/49118/


Never got any review/comment, will do a quick update in this thread.


-- 
David Marchand

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH v3 2/4] ethdev: add siblings iterators
  2019-04-01  2:26   ` [PATCH v3 2/4] ethdev: add siblings iterators Thomas Monjalon
@ 2019-04-01  7:23     ` Andrew Rybchenko
  2019-04-02 23:42     ` Ferruh Yigit
  2019-04-03 15:03     ` Slava Ovsiienko
  2 siblings, 0 replies; 41+ messages in thread
From: Andrew Rybchenko @ 2019-04-01  7:23 UTC (permalink / raw)
  To: Thomas Monjalon, gaetan.rivet, Ferruh Yigit; +Cc: dev

On 4/1/19 5:26 AM, Thomas Monjalon wrote:
> If multiple ports share the same hardware device (rte_device),
> they are siblings and can be found thanks to the new functions
> and loop macros.
> One iterator takes a port id as reference,
> while the other one directly refers to the parent device.
>
> The ownership is not checked because siblings may have
> different owners.
>
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>

Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH v2 2/4] ethdev: add siblings iterators
  2019-04-01  6:46             ` David Marchand
@ 2019-04-01  8:09               ` Thomas Monjalon
  2019-04-02 23:35                 ` Ferruh Yigit
  0 siblings, 1 reply; 41+ messages in thread
From: Thomas Monjalon @ 2019-04-01  8:09 UTC (permalink / raw)
  To: David Marchand; +Cc: Ferruh Yigit, dev, Andrew Rybchenko

01/04/2019 08:46, David Marchand:
> On Mon, Apr 1, 2019 at 4:16 AM Thomas Monjalon <thomas@monjalon.net> wrote:
> 
> > 19/03/2019 19:04, Ferruh Yigit:
> > > On 3/19/2019 5:34 PM, Thomas Monjalon wrote:
> > > >>> +uint16_t __rte_experimental
> > > >>
> > > >> Do we need _rte_experimental on function definitions? I guess only in
> > .h file,
> > > >> function declaration is enough.
> > > >
> > > > Yes we need them both in .h and .c files.
> > >
> > > Why we need them in .c file?
> > > I think the compiler is interested in ones in .h file, because of the
> > > experimental checks.
> >
> > We need the tag in .c file because a check is done in the ELF object
> > by buildtools/check-experimental-syms.sh
> >
> 
> ?
> The attribute should be inherited from the declaration in the header.
> If you have a case where it does not work, I'd like to look at it.

I don't know such case, it was just a belief.

If we can confirm it works well with tag in headers only,
I suggest we remove all of them at once.
For this patch, I prefer being on the safe side for now.

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH v3 1/4] ethdev: simplify port state comparisons
  2019-04-01  2:26   ` [PATCH v3 1/4] ethdev: simplify port state comparisons Thomas Monjalon
@ 2019-04-01 14:58     ` Stephen Hemminger
  2019-04-01 15:17       ` Thomas Monjalon
  2019-04-03 15:03     ` Slava Ovsiienko
  1 sibling, 1 reply; 41+ messages in thread
From: Stephen Hemminger @ 2019-04-01 14:58 UTC (permalink / raw)
  To: Thomas Monjalon; +Cc: gaetan.rivet, Ferruh Yigit, Andrew Rybchenko, dev

On Mon,  1 Apr 2019 04:26:57 +0200
Thomas Monjalon <thomas@monjalon.net> wrote:

> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 10bdfb37e..33cffc498 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -330,8 +330,7 @@ uint16_t
>  rte_eth_find_next(uint16_t port_id)
>  {
>  	while (port_id < RTE_MAX_ETHPORTS &&
> -	       rte_eth_devices[port_id].state != RTE_ETH_DEV_ATTACHED &&
> -	       rte_eth_devices[port_id].state != RTE_ETH_DEV_REMOVED)
> +			rte_eth_devices[port_id].state == RTE_ETH_DEV_UNUSED)

For some applications that iterate over ports this is a hot path.
What about keeping an unused port bit mask and using ffs (in the future)?

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH v3 1/4] ethdev: simplify port state comparisons
  2019-04-01 14:58     ` Stephen Hemminger
@ 2019-04-01 15:17       ` Thomas Monjalon
  2019-04-01 16:07         ` Stephen Hemminger
  0 siblings, 1 reply; 41+ messages in thread
From: Thomas Monjalon @ 2019-04-01 15:17 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: gaetan.rivet, Ferruh Yigit, Andrew Rybchenko, dev

01/04/2019 16:58, Stephen Hemminger:
> On Mon,  1 Apr 2019 04:26:57 +0200
> Thomas Monjalon <thomas@monjalon.net> wrote:
> 
> > diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> > index 10bdfb37e..33cffc498 100644
> > --- a/lib/librte_ethdev/rte_ethdev.c
> > +++ b/lib/librte_ethdev/rte_ethdev.c
> > @@ -330,8 +330,7 @@ uint16_t
> >  rte_eth_find_next(uint16_t port_id)
> >  {
> >  	while (port_id < RTE_MAX_ETHPORTS &&
> > -	       rte_eth_devices[port_id].state != RTE_ETH_DEV_ATTACHED &&
> > -	       rte_eth_devices[port_id].state != RTE_ETH_DEV_REMOVED)
> > +			rte_eth_devices[port_id].state == RTE_ETH_DEV_UNUSED)
> 
> For some applications that iterate over ports this is a hot path.

Really?

> What about keeping an unused port bit mask and using ffs (in the future)?

I don't understand your proposal. Please could you elaborate?

Do you agree on this patch anyway?

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH v3 1/4] ethdev: simplify port state comparisons
  2019-04-01 15:17       ` Thomas Monjalon
@ 2019-04-01 16:07         ` Stephen Hemminger
  0 siblings, 0 replies; 41+ messages in thread
From: Stephen Hemminger @ 2019-04-01 16:07 UTC (permalink / raw)
  To: Thomas Monjalon; +Cc: gaetan.rivet, Ferruh Yigit, Andrew Rybchenko, dev

On Mon, 01 Apr 2019 17:17:35 +0200
Thomas Monjalon <thomas@monjalon.net> wrote:

> 01/04/2019 16:58, Stephen Hemminger:
> > On Mon,  1 Apr 2019 04:26:57 +0200
> > Thomas Monjalon <thomas@monjalon.net> wrote:
> >   
> > > diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> > > index 10bdfb37e..33cffc498 100644
> > > --- a/lib/librte_ethdev/rte_ethdev.c
> > > +++ b/lib/librte_ethdev/rte_ethdev.c
> > > @@ -330,8 +330,7 @@ uint16_t
> > >  rte_eth_find_next(uint16_t port_id)
> > >  {
> > >  	while (port_id < RTE_MAX_ETHPORTS &&
> > > -	       rte_eth_devices[port_id].state != RTE_ETH_DEV_ATTACHED &&
> > > -	       rte_eth_devices[port_id].state != RTE_ETH_DEV_REMOVED)
> > > +			rte_eth_devices[port_id].state == RTE_ETH_DEV_UNUSED)  
> > 
> > For some applications that iterate over ports this is a hot path.  
> 
> Really?
> 
> > What about keeping an unused port bit mask and using ffs (in the future)?  
> 
> I don't understand your proposal. Please could you elaborate?
> 
> Do you agree on this patch anyway?

I have seen some applications spend lots of time doing:

	RTE_ETH_FOREACH_DEV(portid) {


If ethdev kept a bitmask for unused ports then it could use the find
first set instruction.

Maybe the better way is to just fix the application to use its own
bitmask of ports instead.

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH v2 2/4] ethdev: add siblings iterators
  2019-04-01  8:09               ` Thomas Monjalon
@ 2019-04-02 23:35                 ` Ferruh Yigit
  2019-04-02 23:37                   ` Thomas Monjalon
  0 siblings, 1 reply; 41+ messages in thread
From: Ferruh Yigit @ 2019-04-02 23:35 UTC (permalink / raw)
  To: Thomas Monjalon, David Marchand; +Cc: dev, Andrew Rybchenko

On 4/1/2019 9:09 AM, Thomas Monjalon wrote:
> 01/04/2019 08:46, David Marchand:
>> On Mon, Apr 1, 2019 at 4:16 AM Thomas Monjalon <thomas@monjalon.net> wrote:
>>
>>> 19/03/2019 19:04, Ferruh Yigit:
>>>> On 3/19/2019 5:34 PM, Thomas Monjalon wrote:
>>>>>>> +uint16_t __rte_experimental
>>>>>>
>>>>>> Do we need _rte_experimental on function definitions? I guess only in
>>> .h file,
>>>>>> function declaration is enough.
>>>>>
>>>>> Yes we need them both in .h and .c files.
>>>>
>>>> Why we need them in .c file?
>>>> I think the compiler is interested in ones in .h file, because of the
>>>> experimental checks.
>>>
>>> We need the tag in .c file because a check is done in the ELF object
>>> by buildtools/check-experimental-syms.sh
>>>
>>
>> ?
>> The attribute should be inherited from the declaration in the header.
>> If you have a case where it does not work, I'd like to look at it.
> 
> I don't know such case, it was just a belief.

Putting the __rte_experimental tag into header files should be sufficient.

- buildtools/check-experimental-syms.sh
checks if symbols in .map file are marked with __rte_experimental, both putting
the tag to the symbol in .c file or .h file is working.

- tag should be in header file so that application using it can get the warning.

Briefly, __rte_experimental needs to be in .h file, it is optional to have it in
.c, I am for keeping it only in .h file function declaration.

> 
> If we can confirm it works well with tag in headers only,
> I suggest we remove all of them at once.
> For this patch, I prefer being on the safe side for now.
> 
> 

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH v2 2/4] ethdev: add siblings iterators
  2019-04-02 23:35                 ` Ferruh Yigit
@ 2019-04-02 23:37                   ` Thomas Monjalon
  0 siblings, 0 replies; 41+ messages in thread
From: Thomas Monjalon @ 2019-04-02 23:37 UTC (permalink / raw)
  To: Ferruh Yigit; +Cc: David Marchand, dev, Andrew Rybchenko

03/04/2019 01:35, Ferruh Yigit:
> On 4/1/2019 9:09 AM, Thomas Monjalon wrote:
> > 01/04/2019 08:46, David Marchand:
> >> On Mon, Apr 1, 2019 at 4:16 AM Thomas Monjalon <thomas@monjalon.net> wrote:
> >>
> >>> 19/03/2019 19:04, Ferruh Yigit:
> >>>> On 3/19/2019 5:34 PM, Thomas Monjalon wrote:
> >>>>>>> +uint16_t __rte_experimental
> >>>>>>
> >>>>>> Do we need _rte_experimental on function definitions? I guess only in
> >>> .h file,
> >>>>>> function declaration is enough.
> >>>>>
> >>>>> Yes we need them both in .h and .c files.
> >>>>
> >>>> Why we need them in .c file?
> >>>> I think the compiler is interested in ones in .h file, because of the
> >>>> experimental checks.
> >>>
> >>> We need the tag in .c file because a check is done in the ELF object
> >>> by buildtools/check-experimental-syms.sh
> >>>
> >>
> >> ?
> >> The attribute should be inherited from the declaration in the header.
> >> If you have a case where it does not work, I'd like to look at it.
> > 
> > I don't know such case, it was just a belief.
> 
> Putting the __rte_experimental tag into header files should be sufficient.
> 
> - buildtools/check-experimental-syms.sh
> checks if symbols in .map file are marked with __rte_experimental, both putting
> the tag to the symbol in .c file or .h file is working.
> 
> - tag should be in header file so that application using it can get the warning.
> 
> Briefly, __rte_experimental needs to be in .h file, it is optional to have it in
> .c, I am for keeping it only in .h file function declaration.

I agree
As I said below, we can remove all experimental tags from .c files
in a separate patch.

> > If we can confirm it works well with tag in headers only,
> > I suggest we remove all of them at once.
> > For this patch, I prefer being on the safe side for now.

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH v3 2/4] ethdev: add siblings iterators
  2019-04-01  2:26   ` [PATCH v3 2/4] ethdev: add siblings iterators Thomas Monjalon
  2019-04-01  7:23     ` Andrew Rybchenko
@ 2019-04-02 23:42     ` Ferruh Yigit
  2019-04-02 23:48       ` Thomas Monjalon
  2019-04-03 15:03     ` Slava Ovsiienko
  2 siblings, 1 reply; 41+ messages in thread
From: Ferruh Yigit @ 2019-04-02 23:42 UTC (permalink / raw)
  To: Thomas Monjalon, gaetan.rivet, Andrew Rybchenko; +Cc: dev

On 4/1/2019 3:26 AM, Thomas Monjalon wrote:
> If multiple ports share the same hardware device (rte_device),
> they are siblings and can be found thanks to the new functions
> and loop macros.
> One iterator takes a port id as reference,
> while the other one directly refers to the parent device.
> 
> The ownership is not checked because siblings may have
> different owners.
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>

<...>

> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice.
> + *
> + * Iterates over ethdev ports of a specified device.
> + *
> + * @param port_id_start
> + *   The id of the next possible valid port.
> + * @param parent
> + *   The generic device behind the ports to iterate.
> + * @return
> + *   Next port id of the device, possibly port_id_start,
> + *   RTE_MAX_ETHPORTS if there is none.
> + */
> +__rte_experimental
> +uint16_t rte_eth_find_next_of(uint16_t port_id_start,
> +		const struct rte_device *parent);

Minor nit, but other instances using the tag as:

uint16_t __rte_experimental
rte_eth_find_next_of(uint16_t port_id_start,
		const struct rte_device *parent);

What do you think updating it for consistency? Same for two APIs.

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH v3 4/4] app/testpmd: use port sibling iterator in device cleanup
  2019-04-01  2:27   ` [PATCH v3 4/4] app/testpmd: use port sibling iterator in device cleanup Thomas Monjalon
@ 2019-04-02 23:43     ` Ferruh Yigit
  2019-04-03 15:04     ` Slava Ovsiienko
  1 sibling, 0 replies; 41+ messages in thread
From: Ferruh Yigit @ 2019-04-02 23:43 UTC (permalink / raw)
  To: Thomas Monjalon, gaetan.rivet, Wenzhuo Lu, Jingjing Wu,
	Bernard Iremonger
  Cc: dev

On 4/1/2019 3:27 AM, Thomas Monjalon wrote:
> When removing a rte_device on a port-based request,
> all the sibling ports must be marked as closed.
> The iterator loop can be simplified by using the dedicated macro.
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>

Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH v3 2/4] ethdev: add siblings iterators
  2019-04-02 23:42     ` Ferruh Yigit
@ 2019-04-02 23:48       ` Thomas Monjalon
  0 siblings, 0 replies; 41+ messages in thread
From: Thomas Monjalon @ 2019-04-02 23:48 UTC (permalink / raw)
  To: Ferruh Yigit; +Cc: gaetan.rivet, Andrew Rybchenko, dev

03/04/2019 01:42, Ferruh Yigit:
> On 4/1/2019 3:26 AM, Thomas Monjalon wrote:
> > +__rte_experimental
> > +uint16_t rte_eth_find_next_of(uint16_t port_id_start,
> > +		const struct rte_device *parent);
> 
> Minor nit, but other instances using the tag as:
> 
> uint16_t __rte_experimental
> rte_eth_find_next_of(uint16_t port_id_start,
> 		const struct rte_device *parent);
> 
> What do you think updating it for consistency? Same for two APIs.

I think I did it this way to minimize the patch removing it later.
I'm OK to change it for consistency.

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH v3 3/4] net/mlx5: use port sibling iterators
  2019-04-01  2:26   ` [PATCH v3 3/4] net/mlx5: use port sibling iterators Thomas Monjalon
@ 2019-04-03 14:19     ` Ferruh Yigit
  2019-04-03 18:07       ` Yongseok Koh
  2019-04-03 15:04     ` Slava Ovsiienko
  1 sibling, 1 reply; 41+ messages in thread
From: Ferruh Yigit @ 2019-04-03 14:19 UTC (permalink / raw)
  To: Shahaf Shuler, Yongseok Koh; +Cc: Thomas Monjalon, gaetan.rivet, dev

On 4/1/2019 3:26 AM, Thomas Monjalon wrote:
> Iterating over siblings was done with RTE_ETH_FOREACH_DEV()
> which skips the owned ports.
> The new iterators RTE_ETH_FOREACH_DEV_SIBLING()
> and RTE_ETH_FOREACH_DEV_OF() are more appropriate and more correct.
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>

Hi Shahaf, Yongseok,

Any comment on this patch?

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH v3 1/4] ethdev: simplify port state comparisons
  2019-04-01  2:26   ` [PATCH v3 1/4] ethdev: simplify port state comparisons Thomas Monjalon
  2019-04-01 14:58     ` Stephen Hemminger
@ 2019-04-03 15:03     ` Slava Ovsiienko
  1 sibling, 0 replies; 41+ messages in thread
From: Slava Ovsiienko @ 2019-04-03 15:03 UTC (permalink / raw)
  To: Thomas Monjalon, gaetan.rivet, Ferruh Yigit, Andrew Rybchenko; +Cc: dev

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Thomas Monjalon
> Sent: Monday, April 1, 2019 5:27
> To: gaetan.rivet@6wind.com; Ferruh Yigit <ferruh.yigit@intel.com>; Andrew
> Rybchenko <arybchenko@solarflare.com>
> Cc: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v3 1/4] ethdev: simplify port state comparisons
> 
> There are three states for an ethdev port.
> Checking that the port is unused looks simpler than checking it is neither
> attached nor removed.
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>
Tested-by: Viacheslav Ovsiienko <mellanox.com>

> ---
>  lib/librte_ethdev/rte_ethdev.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 10bdfb37e..33cffc498 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -330,8 +330,7 @@ uint16_t
>  rte_eth_find_next(uint16_t port_id)
>  {
>  	while (port_id < RTE_MAX_ETHPORTS &&
> -	       rte_eth_devices[port_id].state != RTE_ETH_DEV_ATTACHED &&
> -	       rte_eth_devices[port_id].state != RTE_ETH_DEV_REMOVED)
> +			rte_eth_devices[port_id].state ==
> RTE_ETH_DEV_UNUSED)
>  		port_id++;
> 
>  	if (port_id >= RTE_MAX_ETHPORTS)
> @@ -567,8 +566,7 @@ uint64_t
>  rte_eth_find_next_owned_by(uint16_t port_id, const uint64_t owner_id)  {
>  	while (port_id < RTE_MAX_ETHPORTS &&
> -	       ((rte_eth_devices[port_id].state != RTE_ETH_DEV_ATTACHED &&
> -	       rte_eth_devices[port_id].state != RTE_ETH_DEV_REMOVED) ||
> +	       (rte_eth_devices[port_id].state == RTE_ETH_DEV_UNUSED ||
>  	       rte_eth_devices[port_id].data->owner.id != owner_id))
>  		port_id++;
> 
> --
> 2.21.0

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH v3 2/4] ethdev: add siblings iterators
  2019-04-01  2:26   ` [PATCH v3 2/4] ethdev: add siblings iterators Thomas Monjalon
  2019-04-01  7:23     ` Andrew Rybchenko
  2019-04-02 23:42     ` Ferruh Yigit
@ 2019-04-03 15:03     ` Slava Ovsiienko
  2 siblings, 0 replies; 41+ messages in thread
From: Slava Ovsiienko @ 2019-04-03 15:03 UTC (permalink / raw)
  To: Thomas Monjalon, gaetan.rivet, Ferruh Yigit, Andrew Rybchenko; +Cc: dev

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Thomas Monjalon
> Sent: Monday, April 1, 2019 5:27
> To: gaetan.rivet@6wind.com; Ferruh Yigit <ferruh.yigit@intel.com>; Andrew
> Rybchenko <arybchenko@solarflare.com>
> Cc: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v3 2/4] ethdev: add siblings iterators
> 
> If multiple ports share the same hardware device (rte_device), they are
> siblings and can be found thanks to the new functions and loop macros.
> One iterator takes a port id as reference, while the other one directly refers
> to the parent device.
> 
> The ownership is not checked because siblings may have different owners.
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
Tested-by: Viacheslav Ovsiienko <mellanox.com>

> ---
> v2: Reviewed-by: Andrew Rybchenko - not kept because of changes in v3
> 
> v3:
> 	- fix logic + re-use rte_eth_find_next()
> 	- longer parameter names
> 	- more and better doxygen comments
> ---
>  lib/librte_ethdev/rte_ethdev.c           | 19 +++++++
>  lib/librte_ethdev/rte_ethdev.h           | 63 ++++++++++++++++++++++++
>  lib/librte_ethdev/rte_ethdev_version.map |  2 +
>  3 files changed, 84 insertions(+)
> 
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 33cffc498..3b125a642 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -339,6 +339,25 @@ rte_eth_find_next(uint16_t port_id)
>  	return port_id;
>  }
> 
> +uint16_t __rte_experimental
> +rte_eth_find_next_of(uint16_t port_id, const struct rte_device *parent)
> +{
> +	port_id = rte_eth_find_next(port_id);
> +	while (port_id < RTE_MAX_ETHPORTS &&
> +			rte_eth_devices[port_id].device != parent)
> +		port_id = rte_eth_find_next(port_id + 1);
> +
> +	return port_id;
> +}
> +
> +uint16_t __rte_experimental
> +rte_eth_find_next_sibling(uint16_t port_id, uint16_t ref_port_id) {
> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(ref_port_id,
> RTE_MAX_ETHPORTS);
> +	return rte_eth_find_next_of(port_id,
> +			rte_eth_devices[ref_port_id].device);
> +}
> +
>  static void
>  rte_eth_dev_shared_data_prepare(void)
>  {
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index b6023c050..3d5bacaee 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -1387,6 +1387,69 @@ uint16_t rte_eth_find_next(uint16_t port_id);
> #define RTE_ETH_FOREACH_DEV(p) \
>  	RTE_ETH_FOREACH_DEV_OWNED_BY(p,
> RTE_ETH_DEV_NO_OWNER)
> 
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice.
> + *
> + * Iterates over ethdev ports of a specified device.
> + *
> + * @param port_id_start
> + *   The id of the next possible valid port.
> + * @param parent
> + *   The generic device behind the ports to iterate.
> + * @return
> + *   Next port id of the device, possibly port_id_start,
> + *   RTE_MAX_ETHPORTS if there is none.
> + */
> +__rte_experimental
> +uint16_t rte_eth_find_next_of(uint16_t port_id_start,
> +		const struct rte_device *parent);
> +
> +/**
> + * Macro to iterate over all ethdev ports of a specified device.
> + *
> + * @param port_id
> + *   The id of the matching port being iterated.
> + * @param parent
> + *   The rte_device pointer matching the iterated ports.
> + */
> +#define RTE_ETH_FOREACH_DEV_OF(port_id, parent) \
> +	for (port_id = rte_eth_find_next_of(0, parent); \
> +		port_id < RTE_MAX_ETHPORTS; \
> +		port_id = rte_eth_find_next_of(port_id + 1, parent))
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice.
> + *
> + * Iterates over sibling ethdev ports (i.e. sharing the same rte_device).
> + *
> + * @param port_id_start
> + *   The id of the next possible valid sibling port.
> + * @param ref_port_id
> + *   The id of a reference port to compare rte_device with.
> + * @return
> + *   Next sibling port id, possibly port_id_start or ref_port_id itself,
> + *   RTE_MAX_ETHPORTS if there is none.
> + */
> +__rte_experimental
> +uint16_t rte_eth_find_next_sibling(uint16_t port_id_start,
> +		uint16_t ref_port_id);
> +
> +/**
> + * Macro to iterate over all ethdev ports sharing the same rte_device
> + * as the specified port.
> + * Note: the specified reference port is part of the loop iterations.
> + *
> + * @param port_id
> + *   The id of the matching port being iterated.
> + * @param ref_port_id
> + *   The id of the port being compared.
> + */
> +#define RTE_ETH_FOREACH_DEV_SIBLING(port_id, ref_port_id) \
> +	for (port_id = rte_eth_find_next_sibling(0, ref_port_id); \
> +		port_id < RTE_MAX_ETHPORTS; \
> +		port_id = rte_eth_find_next_sibling(port_id + 1, ref_port_id))
> 
>  /**
>   * @warning
> diff --git a/lib/librte_ethdev/rte_ethdev_version.map
> b/lib/librte_ethdev/rte_ethdev_version.map
> index 92ac3de25..b37a4167d 100644
> --- a/lib/librte_ethdev/rte_ethdev_version.map
> +++ b/lib/librte_ethdev/rte_ethdev_version.map
> @@ -245,6 +245,8 @@ EXPERIMENTAL {
>  	rte_eth_dev_owner_set;
>  	rte_eth_dev_owner_unset;
>  	rte_eth_dev_rx_intr_ctl_q_get_fd;
> +	rte_eth_find_next_of;
> +	rte_eth_find_next_sibling;
>  	rte_eth_switch_domain_alloc;
>  	rte_eth_switch_domain_free;
>  	rte_flow_conv;
> --
> 2.21.0

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH v3 3/4] net/mlx5: use port sibling iterators
  2019-04-01  2:26   ` [PATCH v3 3/4] net/mlx5: use port sibling iterators Thomas Monjalon
  2019-04-03 14:19     ` Ferruh Yigit
@ 2019-04-03 15:04     ` Slava Ovsiienko
  1 sibling, 0 replies; 41+ messages in thread
From: Slava Ovsiienko @ 2019-04-03 15:04 UTC (permalink / raw)
  To: Thomas Monjalon, gaetan.rivet, Shahaf Shuler, Yongseok Koh; +Cc: dev

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Thomas Monjalon
> Sent: Monday, April 1, 2019 5:27
> To: gaetan.rivet@6wind.com; Shahaf Shuler <shahafs@mellanox.com>;
> Yongseok Koh <yskoh@mellanox.com>
> Cc: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v3 3/4] net/mlx5: use port sibling iterators
> 
> Iterating over siblings was done with RTE_ETH_FOREACH_DEV() which skips
> the owned ports.
> The new iterators RTE_ETH_FOREACH_DEV_SIBLING() and
> RTE_ETH_FOREACH_DEV_OF() are more appropriate and more correct.
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
Tested-by: Viacheslav Ovsiienko <mellanox.com>

> ---
>  drivers/net/mlx5/mlx5.c        | 34 +++++++++++++---------------------
>  drivers/net/mlx5/mlx5_ethdev.c |  6 +-----
>  2 files changed, 14 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c index
> 1d7ca615b..3287a3d78 100644
> --- a/drivers/net/mlx5/mlx5.c
> +++ b/drivers/net/mlx5/mlx5.c
> @@ -493,17 +493,15 @@ mlx5_dev_close(struct rte_eth_dev *dev)
>  			dev->data->port_id);
>  	if (priv->domain_id != RTE_ETH_DEV_SWITCH_DOMAIN_ID_INVALID)
> {
>  		unsigned int c = 0;
> -		unsigned int i = mlx5_dev_to_port_id(dev->device, NULL, 0);
> -		uint16_t port_id[i];
> +		uint16_t port_id;
> 
> -		i = RTE_MIN(mlx5_dev_to_port_id(dev->device, port_id, i), i);
> -		while (i--) {
> +		RTE_ETH_FOREACH_DEV_OF(port_id, dev->device) {
>  			struct mlx5_priv *opriv =
> -				rte_eth_devices[port_id[i]].data-
> >dev_private;
> +				rte_eth_devices[port_id].data->dev_private;
> 
>  			if (!opriv ||
>  			    opriv->domain_id != priv->domain_id ||
> -			    &rte_eth_devices[port_id[i]] == dev)
> +			    &rte_eth_devices[port_id] == dev)
>  				continue;
>  			++c;
>  		}
> @@ -1147,22 +1145,16 @@ mlx5_dev_spawn(struct rte_device *dpdk_dev,
>  	 * Look for sibling devices in order to reuse their switch domain
>  	 * if any, otherwise allocate one.
>  	 */
> -	i = mlx5_dev_to_port_id(dpdk_dev, NULL, 0);
> -	if (i > 0) {
> -		uint16_t port_id[i];
> +	RTE_ETH_FOREACH_DEV_OF(port_id, dpdk_dev) {
> +		const struct mlx5_priv *opriv =
> +			rte_eth_devices[port_id].data->dev_private;
> 
> -		i = RTE_MIN(mlx5_dev_to_port_id(dpdk_dev, port_id, i), i);
> -		while (i--) {
> -			const struct mlx5_priv *opriv =
> -				rte_eth_devices[port_id[i]].data-
> >dev_private;
> -
> -			if (!opriv ||
> -			    opriv->domain_id ==
> -			    RTE_ETH_DEV_SWITCH_DOMAIN_ID_INVALID)
> -				continue;
> -			priv->domain_id = opriv->domain_id;
> -			break;
> -		}
> +		if (!opriv ||
> +			opriv->domain_id ==
> +			RTE_ETH_DEV_SWITCH_DOMAIN_ID_INVALID)
> +			continue;
> +		priv->domain_id = opriv->domain_id;
> +		break;
>  	}
>  	if (priv->domain_id ==
> RTE_ETH_DEV_SWITCH_DOMAIN_ID_INVALID) {
>  		err = rte_eth_switch_domain_alloc(&priv->domain_id);
> diff --git a/drivers/net/mlx5/mlx5_ethdev.c
> b/drivers/net/mlx5/mlx5_ethdev.c index 7273bd940..e01b698fc 100644
> --- a/drivers/net/mlx5/mlx5_ethdev.c
> +++ b/drivers/net/mlx5/mlx5_ethdev.c
> @@ -1398,11 +1398,7 @@ mlx5_dev_to_port_id(const struct rte_device
> *dev, uint16_t *port_list,
>  	uint16_t id;
>  	unsigned int n = 0;
> 
> -	RTE_ETH_FOREACH_DEV(id) {
> -		struct rte_eth_dev *ldev = &rte_eth_devices[id];
> -
> -		if (ldev->device != dev)
> -			continue;
> +	RTE_ETH_FOREACH_DEV_OF(id, dev) {
>  		if (n < port_list_n)
>  			port_list[n] = id;
>  		n++;
> --
> 2.21.0

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH v3 4/4] app/testpmd: use port sibling iterator in device cleanup
  2019-04-01  2:27   ` [PATCH v3 4/4] app/testpmd: use port sibling iterator in device cleanup Thomas Monjalon
  2019-04-02 23:43     ` Ferruh Yigit
@ 2019-04-03 15:04     ` Slava Ovsiienko
  1 sibling, 0 replies; 41+ messages in thread
From: Slava Ovsiienko @ 2019-04-03 15:04 UTC (permalink / raw)
  To: Thomas Monjalon, gaetan.rivet, Wenzhuo Lu, Jingjing Wu,
	Bernard Iremonger
  Cc: dev

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Thomas Monjalon
> Sent: Monday, April 1, 2019 5:27
> To: gaetan.rivet@6wind.com; Wenzhuo Lu <wenzhuo.lu@intel.com>; Jingjing
> Wu <jingjing.wu@intel.com>; Bernard Iremonger
> <bernard.iremonger@intel.com>
> Cc: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v3 4/4] app/testpmd: use port sibling iterator in
> device cleanup
> 
> When removing a rte_device on a port-based request, all the sibling ports
> must be marked as closed.
> The iterator loop can be simplified by using the dedicated macro.
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
Tested-by: Viacheslav Ovsiienko <mellanox.com>

> ---
>  app/test-pmd/testpmd.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> 40c873b97..aeaa74c98 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -2370,9 +2370,7 @@ detach_port_device(portid_t port_id)
>  		return;
>  	}
> 
> -	for (sibling = 0; sibling < RTE_MAX_ETHPORTS; sibling++) {
> -		if (rte_eth_devices[sibling].device != dev)
> -			continue;
> +	RTE_ETH_FOREACH_DEV_SIBLING(sibling, port_id) {
>  		/* reset mapping between old ports and removed device */
>  		rte_eth_devices[sibling].device = NULL;
>  		if (ports[sibling].port_status != RTE_PORT_CLOSED) {
> --
> 2.21.0

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH v3 0/4] ethdev iterators for multi-ports device
  2019-04-01  2:26 ` [PATCH v3 0/4] ethdev iterators for multi-ports device Thomas Monjalon
                     ` (3 preceding siblings ...)
  2019-04-01  2:27   ` [PATCH v3 4/4] app/testpmd: use port sibling iterator in device cleanup Thomas Monjalon
@ 2019-04-03 16:42   ` Ferruh Yigit
  4 siblings, 0 replies; 41+ messages in thread
From: Ferruh Yigit @ 2019-04-03 16:42 UTC (permalink / raw)
  To: Thomas Monjalon, gaetan.rivet; +Cc: dev

On 4/1/2019 3:26 AM, Thomas Monjalon wrote:
> Add port iterators in order to allow listing easily
> the ports of the same device.
> 
> The iterators can be tested by using mlx5 or testpmd.
> 
> 
> v3: changes only in the (main) patch 2
> 
> 
> Thomas Monjalon (4):
>   ethdev: simplify port state comparisons
>   ethdev: add siblings iterators
>   net/mlx5: use port sibling iterators
>   app/testpmd: use port sibling iterator in device cleanup

Series applied to dpdk-next-net/master, thanks.

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH v3 3/4] net/mlx5: use port sibling iterators
  2019-04-03 14:19     ` Ferruh Yigit
@ 2019-04-03 18:07       ` Yongseok Koh
  2019-04-04 11:33         ` Ferruh Yigit
  0 siblings, 1 reply; 41+ messages in thread
From: Yongseok Koh @ 2019-04-03 18:07 UTC (permalink / raw)
  To: Ferruh Yigit; +Cc: Shahaf Shuler, Thomas Monjalon, gaetan.rivet, dev


> On Apr 3, 2019, at 7:19 AM, Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> 
> On 4/1/2019 3:26 AM, Thomas Monjalon wrote:
>> Iterating over siblings was done with RTE_ETH_FOREACH_DEV()
>> which skips the owned ports.
>> The new iterators RTE_ETH_FOREACH_DEV_SIBLING()
>> and RTE_ETH_FOREACH_DEV_OF() are more appropriate and more correct.
>> 
>> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> 
> Hi Shahaf, Yongseok,
> 
> Any comment on this patch?

Sorry for late reply.
I know it has been merged but it looks okay to me.

Thanks,
Yongseok

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH v3 3/4] net/mlx5: use port sibling iterators
  2019-04-03 18:07       ` Yongseok Koh
@ 2019-04-04 11:33         ` Ferruh Yigit
  0 siblings, 0 replies; 41+ messages in thread
From: Ferruh Yigit @ 2019-04-04 11:33 UTC (permalink / raw)
  To: Yongseok Koh; +Cc: Shahaf Shuler, Thomas Monjalon, gaetan.rivet, dev

On 4/3/2019 7:07 PM, Yongseok Koh wrote:
> 
>> On Apr 3, 2019, at 7:19 AM, Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>>
>> On 4/1/2019 3:26 AM, Thomas Monjalon wrote:
>>> Iterating over siblings was done with RTE_ETH_FOREACH_DEV()
>>> which skips the owned ports.
>>> The new iterators RTE_ETH_FOREACH_DEV_SIBLING()
>>> and RTE_ETH_FOREACH_DEV_OF() are more appropriate and more correct.
>>>
>>> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
>>
>> Hi Shahaf, Yongseok,
>>
>> Any comment on this patch?
> 
> Sorry for late reply.
> I know it has been merged but it looks okay to me.

Thanks, I am appending explicit ack to the patch:

Acked-by: Yongseok Koh <yskoh@mellanox.com>

^ permalink raw reply	[flat|nested] 41+ messages in thread

end of thread, other threads:[~2019-04-04 11:33 UTC | newest]

Thread overview: 41+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-30  0:27 [PATCH] ethdev: add siblings iterator Thomas Monjalon
2018-12-11 16:31 ` Ferruh Yigit
2018-12-11 18:19   ` Thomas Monjalon
2019-02-20 22:10 ` [PATCH v2 0/4] ethdev iterators for multi-ports device Thomas Monjalon
2019-02-20 22:10   ` [PATCH v2 1/4] ethdev: simplify port state comparisons Thomas Monjalon
2019-02-24 17:18     ` Andrew Rybchenko
2019-02-20 22:10   ` [PATCH v2 2/4] ethdev: add siblings iterators Thomas Monjalon
2019-02-24 17:22     ` Andrew Rybchenko
2019-02-27 10:07     ` Gaëtan Rivet
2019-02-27 10:51       ` Thomas Monjalon
2019-04-01  1:59         ` Thomas Monjalon
2019-03-19 15:47     ` Ferruh Yigit
2019-03-19 17:34       ` Thomas Monjalon
2019-03-19 18:04         ` Ferruh Yigit
2019-04-01  2:16           ` Thomas Monjalon
2019-04-01  6:46             ` David Marchand
2019-04-01  8:09               ` Thomas Monjalon
2019-04-02 23:35                 ` Ferruh Yigit
2019-04-02 23:37                   ` Thomas Monjalon
2019-02-20 22:10   ` [PATCH v2 3/4] net/mlx5: use port sibling iterators Thomas Monjalon
2019-02-20 22:10   ` [PATCH v2 4/4] app/testpmd: use port sibling iterator in device cleanup Thomas Monjalon
2019-04-01  2:26 ` [PATCH v3 0/4] ethdev iterators for multi-ports device Thomas Monjalon
2019-04-01  2:26   ` [PATCH v3 1/4] ethdev: simplify port state comparisons Thomas Monjalon
2019-04-01 14:58     ` Stephen Hemminger
2019-04-01 15:17       ` Thomas Monjalon
2019-04-01 16:07         ` Stephen Hemminger
2019-04-03 15:03     ` Slava Ovsiienko
2019-04-01  2:26   ` [PATCH v3 2/4] ethdev: add siblings iterators Thomas Monjalon
2019-04-01  7:23     ` Andrew Rybchenko
2019-04-02 23:42     ` Ferruh Yigit
2019-04-02 23:48       ` Thomas Monjalon
2019-04-03 15:03     ` Slava Ovsiienko
2019-04-01  2:26   ` [PATCH v3 3/4] net/mlx5: use port sibling iterators Thomas Monjalon
2019-04-03 14:19     ` Ferruh Yigit
2019-04-03 18:07       ` Yongseok Koh
2019-04-04 11:33         ` Ferruh Yigit
2019-04-03 15:04     ` Slava Ovsiienko
2019-04-01  2:27   ` [PATCH v3 4/4] app/testpmd: use port sibling iterator in device cleanup Thomas Monjalon
2019-04-02 23:43     ` Ferruh Yigit
2019-04-03 15:04     ` Slava Ovsiienko
2019-04-03 16:42   ` [PATCH v3 0/4] ethdev iterators for multi-ports device Ferruh Yigit

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.