Re: [dpdk-dev] [PATCH] ethdev: ensure consistent port id assignment

["Tootoonchian, Amin" <amin.tootoonchian@intel.com>]

Hi Marcin,

Comments inline:

> -----Original Message-----
> From: Kerlin, MarcinX
> Sent: Wednesday, July 20, 2016 1:51 AM
> To: Tootoonchian, Amin <amin.tootoonchian@intel.com>
> Cc: dev@dpdk.org; thomas.monjalon@6wind.com
> Subject: RE: [PATCH] ethdev: ensure consistent port id assignment
> 
> Hi Amin,
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Tootoonchian,
> > Amin
> > Sent: Tuesday, July 12, 2016 4:01 AM
> > To: thomas.monjalon@6wind.com
> > Cc: dev@dpdk.org
> > Subject: [dpdk-dev] [PATCH] ethdev: ensure consistent port id
> > assignment
> >
> > The rte_eth_dev_allocate() code has an implicit assumption that the
> > port id assignment in the secondary process is consistent with that of
> > the primary. The current code breaks if the enumeration of ethdevs in
> > primary and secondary processes are not identical (e.g., when the
> > black/whitelist and vdev args of the primary and secondary do not
> > match, or when the primary dynamically adds/removes ethdevs).
> >
> > To fix this problem, rte_eth_dev_allocate() now looks up port id by
> > name in a secondary process (making it explicit that ethdevs can only
> > be created and initialized by the primary process). Upon releasing a
> > port, the primary process zeros out eth_dev->data to avoid false
> > positives in port id lookup by rte_eth_dev_get_port_by_name().
> >
> > Signed-off-by: Amin Tootoonchian <amin.tootoonchian@intel.com>
> > ---
> >  lib/librte_ether/rte_ethdev.c | 44
> > +++++++++++++++++++++++++++++------------
> > --
> >  1 file changed, 30 insertions(+), 14 deletions(-)
> >
> > diff --git a/lib/librte_ether/rte_ethdev.c
> > b/lib/librte_ether/rte_ethdev.c index
> > 0a6e3f1..1801f57 100644
> > --- a/lib/librte_ether/rte_ethdev.c
> > +++ b/lib/librte_ether/rte_ethdev.c
> > @@ -195,25 +195,37 @@ rte_eth_dev_allocate(const char *name, enum
> > rte_eth_dev_type type)
> >  	uint8_t port_id;
> >  	struct rte_eth_dev *eth_dev;
> >
> > -	port_id = rte_eth_dev_find_free_port();
> > -	if (port_id == RTE_MAX_ETHPORTS) {
> > -		RTE_PMD_DEBUG_TRACE("Reached maximum number of
> > Ethernet ports\n");
> > -		return NULL;
> > -	}
> > -
> >  	if (rte_eth_dev_data == NULL)
> >  		rte_eth_dev_data_alloc();
> >
> > -	if (rte_eth_dev_allocated(name) != NULL) {
> > -		RTE_PMD_DEBUG_TRACE("Ethernet Device with name %s
> > already allocated!\n",
> > -				name);
> > -		return NULL;
> > +	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
> > +		port_id = rte_eth_dev_find_free_port();
> > +		if (port_id == RTE_MAX_ETHPORTS) {
> > +			RTE_PMD_DEBUG_TRACE("Reached maximum number
> > of Ethernet ports\n");
> > +			return NULL;
> > +		}
> > +
> > +		if (rte_eth_dev_allocated(name) != NULL) {
> > +			RTE_PMD_DEBUG_TRACE("Ethernet Device with name
> > %s already allocated!\n",
> > +					name);
> > +			return NULL;
> > +		}
> > +	} else {
> > +		if (rte_eth_dev_get_port_by_name(name, &port_id) != 0) {
> 
> 
> I was working also on this problem but I didn't send patch yet, so I did review of
> your code.
> 
> Condition (rte_eth_dev_get_port_by_name(name, &port_id) != 0) will always
> fail.
> Secondary process enter here and he will be looking for him name but has not
> yet added and the application return NULL here e.g. we will run app with device
> name pcap1 but this device is not on list and function return null.

This is the intended behavior with this patch. Ports are to be created only by the primary process. This is required for correct operation IMO, because if we allow secondary processes to create ports dynamically (and locally use conflicting port ids) without any synchronization mechanism, they're guaranteed to overwrite each other's rte_eth_dev_data.

> > +			RTE_PMD_DEBUG_TRACE("Ethernet Device with name
> > %s could not be found!\n",
> > +					name);
> > +			return NULL;
> > +		}
> >  	}
> >
> >  	eth_dev = &rte_eth_devices[port_id];
> >  	eth_dev->data = &rte_eth_dev_data[port_id];
> > -	snprintf(eth_dev->data->name, sizeof(eth_dev->data->name), "%s",
> > name);
> > -	eth_dev->data->port_id = port_id;
> > +
> > +	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
> > +		snprintf(eth_dev->data->name, sizeof(eth_dev->data->name),
> > "%s", name);
> > +		eth_dev->data->port_id = port_id;
> > +	}
> > +
> 
> 
> 1. rte_eth_dev_data[port_id] -> port id should be shifted because secondary
> process
> overwrite e.g. first position which is common with primary process, so should be
> add at the end
>
> 2. If this condition is true only for primary it means that secondary process can't
> add own name.
> So this excludes with above line: "if (rte_eth_dev_get_port_by_name(name,
> &port_id) != 0)"?

No, with this patch, secondary processes cannot add their own ports, but instead, through some mechanism, should ask the primary process to create the port for them.

> I will send also my patch soon and we can compare and prepare a common
> version. We should
> keep in mind also the hot plugging.

Thanks Marcin! Indeed, I have been using this patch in an environment which we use hotplug very aggressively and I confirm that the patch works flawlessly. Of course, all processes using an about-to-be-removed port need to close and detach. For attaching, the primary attaches first and any secondary can attach next.

Thomas, your thoughts?

Thanks,
Amin