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

["Kerlin, MarcinX" <marcinx.kerlin@intel.com>]

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.


> +			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)"?

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.


>  	eth_dev->attached = DEV_ATTACHED;
>  	eth_dev->dev_type = type;
>  	nb_ports++;
> @@ -293,8 +305,10 @@ rte_eth_dev_init(struct rte_pci_driver *pci_drv,
>  			pci_drv->name,
>  			(unsigned) pci_dev->id.vendor_id,
>  			(unsigned) pci_dev->id.device_id);
> -	if (rte_eal_process_type() == RTE_PROC_PRIMARY)
> +	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
>  		rte_free(eth_dev->data->dev_private);
> +		memset(eth_dev->data, 0, sizeof(*rte_eth_dev_data));
> +	}
>  	rte_eth_dev_release_port(eth_dev);
>  	return diag;
>  }
> @@ -330,8 +344,10 @@ rte_eth_dev_uninit(struct rte_pci_device *pci_dev)
>  	/* free ether device */
>  	rte_eth_dev_release_port(eth_dev);
> 
> -	if (rte_eal_process_type() == RTE_PROC_PRIMARY)
> +	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
>  		rte_free(eth_dev->data->dev_private);
> +		memset(eth_dev->data, 0, sizeof(*rte_eth_dev_data));
> +	}
> 
>  	eth_dev->pci_dev = NULL;
>  	eth_dev->driver = NULL;
> --
> 2.8.1

Regards,
Marcin