diff mbox series

[1/2] ethdev: avoid explicit check of valid port state

Message ID 20190417225928.8962-1-thomas@monjalon.net (mailing list archive)
State Accepted, archived
Delegated to: Ferruh Yigit
Headers
Series [1/2] ethdev: avoid explicit check of valid port state |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/intel-Performance-Testing success Performance Testing PASS
ci/mellanox-Performance-Testing success Performance Testing PASS

Commit Message

Thomas Monjalon April 17, 2019, 10:59 p.m. UTC 
  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(-)

Comments

Ferruh Yigit April 18, 2019, 11:50 a.m. UTC | #1 
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;
>  }
>
Thomas Monjalon April 18, 2019, 12:47 p.m. UTC | #2 
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."
Thomas Monjalon April 18, 2019, 5:38 p.m. UTC | #3 
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.
 */
Ferruh Yigit April 18, 2019, 6:37 p.m. UTC | #4 
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)
diff mbox series

Patch

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