Re: [dpdk-dev] [PATCH v3 2/7] ethdev: fix used portid allocation

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

Hi Matan,

> 
> Hi Konstantin
> 
> From: Ananyev, Konstantin, Friday, January 19, 2018 2:40 PM
> > > -----Original Message-----
> > > From: Matan Azrad [mailto:matan@mellanox.com]
> > > Sent: Thursday, January 18, 2018 4:35 PM
> > > To: Thomas Monjalon <thomas@monjalon.net>; Gaetan Rivet
> > > <gaetan.rivet@6wind.com>; Wu, Jingjing <jingjing.wu@intel.com>
> > > Cc: dev@dpdk.org; Neil Horman <nhorman@tuxdriver.com>; Richardson,
> > > Bruce <bruce.richardson@intel.com>; Ananyev, Konstantin
> > > <konstantin.ananyev@intel.com>; stable@dpdk.org
> > > Subject: [PATCH v3 2/7] ethdev: fix used portid allocation
> > >
> > > rte_eth_dev_find_free_port() found a free port by state checking.
> > > The state field are in local process memory, so other DPDK processes
> > > may get the same port ID because their local states may be different.
> > >
> > > Replace the state checking by the ethdev port name checking, so, if
> > > the name is an empty string the port ID will be detected as unused.
> > >
> > > Fixes: d948f596fee2 ("ethdev: fix port data mismatched in multiple
> > > process model")
> > > Cc: stable@dpdk.org
> > >
> > > Suggested-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> > > Signed-off-by: Matan Azrad <matan@mellanox.com>
> > > ---
> > >  lib/librte_ether/rte_ethdev.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/lib/librte_ether/rte_ethdev.c
> > > b/lib/librte_ether/rte_ethdev.c index 156231c..5d87f72 100644
> > > --- a/lib/librte_ether/rte_ethdev.c
> > > +++ b/lib/librte_ether/rte_ethdev.c
> > > @@ -164,7 +164,7 @@ struct rte_eth_dev *
> > >  	unsigned i;
> > >
> > >  	for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
> > > -		if (rte_eth_devices[i].state == RTE_ETH_DEV_UNUSED)
> > > +		if (rte_eth_dev_share_data->data[i].name[0] == '\0')
> >
> > I know it is not really necessary, but I'd keep both (just in case):
> > if (rte_eth_devices[i].state == RTE_ETH_DEV_UNUSED) &&
> > rte_eth_dev_share_data->data[i].name[0] == '\0')
> >
> Since, as you, I don't think it is necessary, searched again and didn't find reason to that,
> What's about
> RTE_ASSERT(rte_eth_devices[i].state == RTE_ETH_DEV_UNUSED);
>  Instead?

Sounds ok to me.
Konstantin

> 
> > Aprart from that: Acked-by: Konstantin Ananyev
> > <konstantin.ananyev@intel.com>
> >
> > >  			return i;
> > >  	}
> > >  	return RTE_MAX_ETHPORTS;
> > > --
> > > 1.8.3.1