[PATCH] ethdev: add siblings iterator

[PATCH] ethdev: add siblings iterator

[Thomas Monjalon]

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

Re: [PATCH] ethdev: add siblings iterator

[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

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

Re: [PATCH] ethdev: add siblings iterator

[Thomas Monjalon]

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?

[PATCH v2 0/4] ethdev iterators for multi-ports device

[Thomas Monjalon]

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

[PATCH v2 1/4] ethdev: simplify port state comparisons

[Thomas Monjalon]

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

Re: [PATCH v2 1/4] ethdev: simplify port state comparisons

[Andrew Rybchenko]

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>

[PATCH v2 2/4] ethdev: add siblings iterators

[Thomas Monjalon]

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

Re: [PATCH v2 2/4] ethdev: add siblings iterators

[Andrew Rybchenko]

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>

Re: [PATCH v2 2/4] ethdev: add siblings iterators

[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).

-- 
Gaëtan Rivet
6WIND

Re: [PATCH v2 2/4] ethdev: add siblings iterators

[Thomas Monjalon]

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.

Re: [PATCH v2 2/4] ethdev: add siblings iterators

[Thomas Monjalon]

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

Re: [PATCH v2 2/4] ethdev: add siblings iterators

[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.

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

Re: [PATCH v2 2/4] ethdev: add siblings iterators

[Thomas Monjalon]

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.

Re: [PATCH v2 2/4] ethdev: add siblings iterators

[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.

Re: [PATCH v2 2/4] ethdev: add siblings iterators

[Thomas Monjalon]

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/

Re: [PATCH v2 2/4] ethdev: add siblings iterators

[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.



> 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

Re: [PATCH v2 2/4] ethdev: add siblings iterators

[Thomas Monjalon]

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.

Re: [PATCH v2 2/4] ethdev: add siblings iterators

[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.

> 
> 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.
> 
> 

Re: [PATCH v2 2/4] ethdev: add siblings iterators

[Thomas Monjalon]

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.

[PATCH v2 3/4] net/mlx5: use port sibling iterators

[Thomas Monjalon]

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

[PATCH v2 4/4] app/testpmd: use port sibling iterator in device cleanup

[Thomas Monjalon]

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

[PATCH v3 0/4] ethdev iterators for multi-ports device

[Thomas Monjalon]

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

[PATCH v3 1/4] ethdev: simplify port state comparisons

[Thomas Monjalon]

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

Re: [PATCH v3 1/4] ethdev: simplify port state comparisons

[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.
What about keeping an unused port bit mask and using ffs (in the future)?

Re: [PATCH v3 1/4] ethdev: simplify port state comparisons

[Thomas Monjalon]

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?

Re: [PATCH v3 1/4] ethdev: simplify port state comparisons

[Stephen Hemminger]

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.

Re: [PATCH v3 1/4] ethdev: simplify port state comparisons

[Slava Ovsiienko]

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

[PATCH v3 2/4] ethdev: add siblings iterators

[Thomas Monjalon]

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

Re: [PATCH v3 2/4] ethdev: add siblings iterators

[Andrew Rybchenko]

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>

Re: [PATCH v3 2/4] ethdev: add siblings iterators

[Ferruh Yigit]

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.

Re: [PATCH v3 2/4] ethdev: add siblings iterators

[Thomas Monjalon]

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.

Re: [PATCH v3 2/4] ethdev: add siblings iterators

[Slava Ovsiienko]

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

[PATCH v3 3/4] net/mlx5: use port sibling iterators

[Thomas Monjalon]

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

Re: [PATCH v3 3/4] net/mlx5: use port sibling iterators

[Ferruh Yigit]

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?

Re: [PATCH v3 3/4] net/mlx5: use port sibling iterators

[Yongseok Koh]

> 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

Re: [PATCH v3 3/4] net/mlx5: use port sibling iterators

[Ferruh Yigit]

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>

Re: [PATCH v3 3/4] net/mlx5: use port sibling iterators

[Slava Ovsiienko]

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

[PATCH v3 4/4] app/testpmd: use port sibling iterator in device cleanup

[Thomas Monjalon]

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

Re: [PATCH v3 4/4] app/testpmd: use port sibling iterator in device cleanup

[Ferruh Yigit]

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>

Re: [PATCH v3 4/4] app/testpmd: use port sibling iterator in device cleanup

[Slava Ovsiienko]

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

Re: [PATCH v3 0/4] ethdev iterators for multi-ports device

[Ferruh Yigit]

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.