Hi Amin,
> -----Original Message-----
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Tootoonchian, Amin
> Sent: Tuesday, July 12, 2016 4:01 AM
> To: thomas.monjalon at 6wind.com
> Cc: dev at 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 at 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
