[dpdk-dev] [PATCH 1/2] ethdev: avoid explicit check of valid port state

[dpdk-dev] [PATCH 1/2] ethdev: avoid explicit check of valid port state

[Thomas Monjalon]

Some port iterations are manually checking against RTE_ETH_DEV_UNUSED
instead of using the iterators based on rte_eth_find_next().

A new macro RTE_ETH_FOREACH_VALID_DEV() is introduced, but kept private
because there should be no need of iterating over all devices in the API.
The public iterators have additional filters for ownership, parent device
or sibling ports.

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
 drivers/net/mlx5/mlx5.c        |  9 ++-------
 lib/librte_ethdev/rte_ethdev.c | 25 ++++++++++++-------------
 2 files changed, 14 insertions(+), 20 deletions(-)

diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
index 9ff50dfbe..4deaada5c 100644
--- a/drivers/net/mlx5/mlx5.c
+++ b/drivers/net/mlx5/mlx5.c
@@ -1964,14 +1964,9 @@ static int
 mlx5_pci_remove(struct rte_pci_device *pci_dev)
 {
 	uint16_t port_id;
-	struct rte_eth_dev *port;
 
-	for (port_id = 0; port_id < RTE_MAX_ETHPORTS; port_id++) {
-		port = &rte_eth_devices[port_id];
-		if (port->state != RTE_ETH_DEV_UNUSED &&
-				port->device == &pci_dev->device)
-			rte_eth_dev_close(port_id);
-	}
+	RTE_ETH_FOREACH_DEV_OF(port_id, &pci_dev->device)
+		rte_eth_dev_close(port_id);
 	return 0;
 }
 
diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 243beb4dd..cca15efca 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -339,6 +339,11 @@ rte_eth_find_next(uint16_t port_id)
 	return port_id;
 }
 
+#define RTE_ETH_FOREACH_VALID_DEV(port_id) \
+	for (port_id = rte_eth_find_next(0); \
+	     port_id < RTE_MAX_ETHPORTS; \
+	     port_id = rte_eth_find_next(port_id + 1))
+
 uint16_t
 rte_eth_find_next_of(uint16_t port_id, const struct rte_device *parent)
 {
@@ -584,13 +589,10 @@ rte_eth_is_valid_owner_id(uint64_t owner_id)
 uint64_t
 rte_eth_find_next_owned_by(uint16_t port_id, const uint64_t owner_id)
 {
+	port_id = rte_eth_find_next(port_id);
 	while (port_id < RTE_MAX_ETHPORTS &&
-	       (rte_eth_devices[port_id].state == RTE_ETH_DEV_UNUSED ||
-	       rte_eth_devices[port_id].data->owner.id != owner_id))
-		port_id++;
-
-	if (port_id >= RTE_MAX_ETHPORTS)
-		return RTE_MAX_ETHPORTS;
+			rte_eth_devices[port_id].data->owner.id != owner_id)
+		port_id = rte_eth_find_next(port_id + 1);
 
 	return port_id;
 }
@@ -768,9 +770,8 @@ rte_eth_dev_count_total(void)
 {
 	uint16_t port, count = 0;
 
-	for (port = 0; port < RTE_MAX_ETHPORTS; port++)
-		if (rte_eth_devices[port].state != RTE_ETH_DEV_UNUSED)
-			count++;
+	RTE_ETH_FOREACH_VALID_DEV(port)
+		count++;
 
 	return count;
 }
@@ -804,13 +805,11 @@ rte_eth_dev_get_port_by_name(const char *name, uint16_t *port_id)
 		return -EINVAL;
 	}
 
-	for (pid = 0; pid < RTE_MAX_ETHPORTS; pid++) {
-		if (rte_eth_devices[pid].state != RTE_ETH_DEV_UNUSED &&
-		    !strcmp(name, rte_eth_dev_shared_data->data[pid].name)) {
+	RTE_ETH_FOREACH_VALID_DEV(pid)
+		if (!strcmp(name, rte_eth_dev_shared_data->data[pid].name)) {
 			*port_id = pid;
 			return 0;
 		}
-	}
 
 	return -ENODEV;
 }
-- 
2.21.0

[dpdk-dev] [PATCH 2/2] ethdev: promote function for port count as stable

[Thomas Monjalon]

The function rte_eth_dev_count_total() was introduced one year ago
in the release 18.05. It can be declared non experimental in 19.05.

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

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index cca15efca..4d99186e8 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -765,7 +765,7 @@ rte_eth_dev_count_avail(void)
 	return count;
 }
 
-uint16_t __rte_experimental
+uint16_t
 rte_eth_dev_count_total(void)
 {
 	uint16_t port, count = 0;
diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index 40a068fe8..aafc503db 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -1560,7 +1560,7 @@ uint16_t rte_eth_dev_count_avail(void);
  * @return
  *   The total count of Ethernet devices.
  */
-uint16_t __rte_experimental rte_eth_dev_count_total(void);
+uint16_t rte_eth_dev_count_total(void);
 
 /**
  * Convert a numerical speed in Mbps to a bitmap flag that can be used in
diff --git a/lib/librte_ethdev/rte_ethdev_version.map b/lib/librte_ethdev/rte_ethdev_version.map
index b37a4167d..afcd25599 100644
--- a/lib/librte_ethdev/rte_ethdev_version.map
+++ b/lib/librte_ethdev/rte_ethdev_version.map
@@ -229,11 +229,17 @@ DPDK_18.11 {
 
 } DPDK_18.08;
 
+DPDK_19.05 {
+	global:
+
+	rte_eth_dev_count_total;
+
+} DPDK_18.11;
+
 EXPERIMENTAL {
 	global:
 
 	rte_eth_devargs_parse;
-	rte_eth_dev_count_total;
 	rte_eth_dev_create;
 	rte_eth_dev_destroy;
 	rte_eth_dev_get_module_eeprom;
-- 
2.21.0

Re: [dpdk-dev] [PATCH 1/2] ethdev: avoid explicit check of valid port state

[Ferruh Yigit]

On 4/17/2019 11:59 PM, Thomas Monjalon wrote:
> Some port iterations are manually checking against RTE_ETH_DEV_UNUSED
> instead of using the iterators based on rte_eth_find_next().
> 
> A new macro RTE_ETH_FOREACH_VALID_DEV() is introduced, but kept private
> because there should be no need of iterating over all devices in the API.
> The public iterators have additional filters for ownership, parent device
> or sibling ports.
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> ---
>  drivers/net/mlx5/mlx5.c        |  9 ++-------
>  lib/librte_ethdev/rte_ethdev.c | 25 ++++++++++++-------------

No strong opinion but should we separate patch for driver and the library,
logically both changes RTE_ETH_DEV_UNUSED check with macros, but there is no
dependency, I mean they are individual changes, driver patch will be valid on
its own.

>  2 files changed, 14 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
> index 9ff50dfbe..4deaada5c 100644
> --- a/drivers/net/mlx5/mlx5.c
> +++ b/drivers/net/mlx5/mlx5.c
> @@ -1964,14 +1964,9 @@ static int
>  mlx5_pci_remove(struct rte_pci_device *pci_dev)
>  {
>  	uint16_t port_id;
> -	struct rte_eth_dev *port;
>  
> -	for (port_id = 0; port_id < RTE_MAX_ETHPORTS; port_id++) {
> -		port = &rte_eth_devices[port_id];
> -		if (port->state != RTE_ETH_DEV_UNUSED &&
> -				port->device == &pci_dev->device)
> -			rte_eth_dev_close(port_id);
> -	}
> +	RTE_ETH_FOREACH_DEV_OF(port_id, &pci_dev->device)
> +		rte_eth_dev_close(port_id);
>  	return 0;
>  }
>  
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 243beb4dd..cca15efca 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -339,6 +339,11 @@ rte_eth_find_next(uint16_t port_id)
>  	return port_id;
>  }
>  
> +#define RTE_ETH_FOREACH_VALID_DEV(port_id) \
> +	for (port_id = rte_eth_find_next(0); \
> +	     port_id < RTE_MAX_ETHPORTS; \
> +	     port_id = rte_eth_find_next(port_id + 1))
> +

What do you think adding some documentation to the new macro, specially I think
documenting the difference between "RTE_ETH_FOREACH_DEV" and this one can be
good otherwise it may confuse people that "RTE_ETH_FOREACH_DEV" iterates on
invalid devices too?

>  uint16_t
>  rte_eth_find_next_of(uint16_t port_id, const struct rte_device *parent)
>  {
> @@ -584,13 +589,10 @@ rte_eth_is_valid_owner_id(uint64_t owner_id)
>  uint64_t
>  rte_eth_find_next_owned_by(uint16_t port_id, const uint64_t owner_id)
>  {
> +	port_id = rte_eth_find_next(port_id);
>  	while (port_id < RTE_MAX_ETHPORTS &&
> -	       (rte_eth_devices[port_id].state == RTE_ETH_DEV_UNUSED ||
> -	       rte_eth_devices[port_id].data->owner.id != owner_id))
> -		port_id++;
> -
> -	if (port_id >= RTE_MAX_ETHPORTS)
> -		return RTE_MAX_ETHPORTS;
> +			rte_eth_devices[port_id].data->owner.id != owner_id)
> +		port_id = rte_eth_find_next(port_id + 1);
>  
>  	return port_id;
>  }
> @@ -768,9 +770,8 @@ rte_eth_dev_count_total(void)
>  {
>  	uint16_t port, count = 0;
>  
> -	for (port = 0; port < RTE_MAX_ETHPORTS; port++)
> -		if (rte_eth_devices[port].state != RTE_ETH_DEV_UNUSED)
> -			count++;
> +	RTE_ETH_FOREACH_VALID_DEV(port)
> +		count++;
>  
>  	return count;
>  }
> @@ -804,13 +805,11 @@ rte_eth_dev_get_port_by_name(const char *name, uint16_t *port_id)
>  		return -EINVAL;
>  	}
>  
> -	for (pid = 0; pid < RTE_MAX_ETHPORTS; pid++) {
> -		if (rte_eth_devices[pid].state != RTE_ETH_DEV_UNUSED &&
> -		    !strcmp(name, rte_eth_dev_shared_data->data[pid].name)) {
> +	RTE_ETH_FOREACH_VALID_DEV(pid)
> +		if (!strcmp(name, rte_eth_dev_shared_data->data[pid].name)) {
>  			*port_id = pid;
>  			return 0;
>  		}
> -	}
>  
>  	return -ENODEV;
>  }
> 

Re: [dpdk-dev] [PATCH 2/2] ethdev: promote function for port count as stable

[Ferruh Yigit]

On 4/17/2019 11:59 PM, Thomas Monjalon wrote:
> The function rte_eth_dev_count_total() was introduced one year ago
> in the release 18.05. It can be declared non experimental in 19.05.
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>

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

Re: [dpdk-dev] [PATCH 2/2] ethdev: promote function for port count as stable

[Andrew Rybchenko]

On 4/18/19 2:51 PM, Ferruh Yigit wrote:
> On 4/17/2019 11:59 PM, Thomas Monjalon wrote:
>> The function rte_eth_dev_count_total() was introduced one year ago
>> in the release 18.05. It can be declared non experimental in 19.05.
>>
>> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> Acked-by: Ferruh Yigit <ferruh.yigit@intel.com>

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

Re: [dpdk-dev] [PATCH 1/2] ethdev: avoid explicit check of valid port state

[Thomas Monjalon]

18/04/2019 13:50, Ferruh Yigit:
> On 4/17/2019 11:59 PM, Thomas Monjalon wrote:
> > Some port iterations are manually checking against RTE_ETH_DEV_UNUSED
> > instead of using the iterators based on rte_eth_find_next().
> > 
> > A new macro RTE_ETH_FOREACH_VALID_DEV() is introduced, but kept private
> > because there should be no need of iterating over all devices in the API.
> > The public iterators have additional filters for ownership, parent device
> > or sibling ports.
> > 
> > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> > ---
> >  drivers/net/mlx5/mlx5.c        |  9 ++-------
> >  lib/librte_ethdev/rte_ethdev.c | 25 ++++++++++++-------------
> 
> No strong opinion but should we separate patch for driver and the library,
> logically both changes RTE_ETH_DEV_UNUSED check with macros, but there is no
> dependency, I mean they are individual changes, driver patch will be valid on
> its own.

This is the same change. I removed usage of RTE_ETH_DEV_UNUSED.
By chance, it was used only in ethdev and mlx5.
I don't feel the need to split because there are usages in different files.


> > +#define RTE_ETH_FOREACH_VALID_DEV(port_id) \
> > +	for (port_id = rte_eth_find_next(0); \
> > +	     port_id < RTE_MAX_ETHPORTS; \
> > +	     port_id = rte_eth_find_next(port_id + 1))
> > +
> 
> What do you think adding some documentation to the new macro, specially I think
> documenting the difference between "RTE_ETH_FOREACH_DEV" and this one can be
> good otherwise it may confuse people that "RTE_ETH_FOREACH_DEV" iterates on
> invalid devices too?

This one is not part of the API.
I am not sure what I can document more than "iterating all valid ports"?
About RTE_ETH_FOREACH_DEV, it is already documented:
	"Macro to iterate over all enabled and ownerless ethdev ports."

Re: [dpdk-dev] [PATCH 1/2] ethdev: avoid explicit check of valid port state

[Thomas Monjalon]

18/04/2019 14:47, Thomas Monjalon:
> 18/04/2019 13:50, Ferruh Yigit:
> > On 4/17/2019 11:59 PM, Thomas Monjalon wrote:
> > > Some port iterations are manually checking against RTE_ETH_DEV_UNUSED
> > > instead of using the iterators based on rte_eth_find_next().
> > > 
> > > A new macro RTE_ETH_FOREACH_VALID_DEV() is introduced, but kept private
> > > because there should be no need of iterating over all devices in the API.
> > > The public iterators have additional filters for ownership, parent device
> > > or sibling ports.
> > > 
> > > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> > > ---
> > > +#define RTE_ETH_FOREACH_VALID_DEV(port_id) \
> > > +	for (port_id = rte_eth_find_next(0); \
> > > +	     port_id < RTE_MAX_ETHPORTS; \
> > > +	     port_id = rte_eth_find_next(port_id + 1))
> > > +
> > 
> > What do you think adding some documentation to the new macro, specially I think
> > documenting the difference between "RTE_ETH_FOREACH_DEV" and this one can be
> > good otherwise it may confuse people that "RTE_ETH_FOREACH_DEV" iterates on
> > invalid devices too?
> 
> This one is not part of the API.
> I am not sure what I can document more than "iterating all valid ports"?
> About RTE_ETH_FOREACH_DEV, it is already documented:
> 	"Macro to iterate over all enabled and ownerless ethdev ports."

OK, let's add a comment to explain the difference:

/*
 * Macro to iterate over all valid ports for internal usage.
 * Note: RTE_ETH_FOREACH_DEV is different because filtering owned ports.
 */

Re: [dpdk-dev] [PATCH 1/2] ethdev: avoid explicit check of valid port state

[Ferruh Yigit]

On 4/17/2019 11:59 PM, Thomas Monjalon wrote:
> Some port iterations are manually checking against RTE_ETH_DEV_UNUSED
> instead of using the iterators based on rte_eth_find_next().
> 
> A new macro RTE_ETH_FOREACH_VALID_DEV() is introduced, but kept private
> because there should be no need of iterating over all devices in the API.
> The public iterators have additional filters for ownership, parent device
> or sibling ports.
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>

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

(Note for macro added while merging)