diff mbox series

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

Message ID 20190401022700.1570-2-thomas@monjalon.net (mailing list archive)
State Accepted, archived
Delegated to: Ferruh Yigit
Headers
Series ethdev iterators for multi-ports device |

Checks

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

Commit Message

Thomas Monjalon April 1, 2019, 2:26 a.m. UTC 
  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(-)

Comments

Stephen Hemminger April 1, 2019, 2:58 p.m. UTC | #1 
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)?
Thomas Monjalon April 1, 2019, 3:17 p.m. UTC | #2 
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?
Stephen Hemminger April 1, 2019, 4:07 p.m. UTC | #3 
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.
Slava Ovsiienko April 3, 2019, 3:03 p.m. UTC | #4 
> -----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
diff mbox series

Patch

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