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

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

[Tootoonchian, Amin]

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) {
+			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;
+	}
+
 	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

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

[Kerlin, MarcinX]

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

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

[Tootoonchian, Amin]

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

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

[Thomas Monjalon]

Hi,

2016-07-20 15:07, Tootoonchian, Amin:
> Thomas, your thoughts?

I have 2 thoughts:
- it is too big for 16.07
- it is related to multi-process mechanism, maintained by Sergio ;)

Sorry I won't look at it shortly.

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

[Tootoonchian, Amin]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Wednesday, July 20, 2016 8:12 AM
> To: Tootoonchian, Amin <amin.tootoonchian@intel.com>
> Cc: dev@dpdk.org; Kerlin, MarcinX <marcinx.kerlin@intel.com>
> Subject: Re: [dpdk-dev] [PATCH] ethdev: ensure consistent port id assignment
> 
> Hi,
> 
> 2016-07-20 15:07, Tootoonchian, Amin:
> > Thomas, your thoughts?
> 
> I have 2 thoughts:
> - it is too big for 16.07
> - it is related to multi-process mechanism, maintained by Sergio ;)
> 
> Sorry I won't look at it shortly.

[Adding Sergio to the thread for review.]

The actual change of this patch is quite small (see below) but I understand that it may be too late for 16.07:

* The main addition is getting the port id using rte_eth_dev_get_port_by_name for secondary processes in rte_eth_dev_allocate. Also zeroing out rte_eth_dev_data after freeing a port to avoid false positives in rte_eth_dev_get_port_by_name.

* The rest of the patch moves parts of rte_eth_dev_allocate to be executed only by the primary.

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

[Kerlin, MarcinX]

Hi Amin,

> -----Original Message-----
> From: Tootoonchian, Amin
> Sent: Wednesday, July 20, 2016 5:08 PM
> To: Kerlin, MarcinX <marcinx.kerlin@intel.com>
> Cc: dev@dpdk.org; thomas.monjalon@6wind.com
> Subject: RE: [PATCH] ethdev: ensure consistent port id assignment
> 
> 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.

Thanks Amin for clarification,
I had another approach, that rte_eth_devices and rte_eth_dev_data should have 
different offset of port_id and secondary process can also add devices.

as I now understand with this patch we will not be able do something like:
Primary:
./test-pmd -c 0xf  -n 4 --socket-mem='512,0'  -w 03:00.1 -w 03:00.0  
				--proc-type=primary --file-prefix=xz1 -- -i
Secondary: 
./test-pmd -c 0xf0 --socket-mem='512,0' -n 4 -v -b 03:00.1 -b 03:00.0 
--vdev 'eth_pcap0,rx_pcap=/var/log/device1.pcap,tx_pcap=/var/log/device2.pcap'
--proc-type=secondary --file-prefix=xz1 -- -i 

Because secondary processes "Ports are to be created only by the primary process"?
I tried run and secondary process failed here:
else {
		if (rte_eth_dev_get_port_by_name(name, &port_id) != 0) {
			RTE_PMD_DEBUG_TRACE("Ethernet Device with name %s could not be found!\n",
					name);
			return NULL;
		}
	}
because he did not find the name "eth_pcap0"

+ Sergio, just like you added also in the next mail

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

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

[Tootoonchian, Amin]

Inline:

> > 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.
> 
> Thanks Amin for clarification,
> I had another approach, that rte_eth_devices and rte_eth_dev_data should have
> different offset of port_id and secondary process can also add devices.

That wouldn't work without some rather intrusive changes. As of now, rte_eth_dev_data includes port_id and therefore should be consistent across processes. 

> as I now understand with this patch we will not be able do something like:
> Primary:
> ./test-pmd -c 0xf  -n 4 --socket-mem='512,0'  -w 03:00.1 -w 03:00.0
> 				--proc-type=primary --file-prefix=xz1 -- -i
> Secondary:
> ./test-pmd -c 0xf0 --socket-mem='512,0' -n 4 -v -b 03:00.1 -b 03:00.0 --vdev
> 'eth_pcap0,rx_pcap=/var/log/device1.pcap,tx_pcap=/var/log/device2.pcap'
> --proc-type=secondary --file-prefix=xz1 -- -i
> 
> Because secondary processes "Ports are to be created only by the primary
> process"?

Right, that wouldn't work.

Amin

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

[Tootoonchian, Amin]

Sergio, could you please review this patch?

Thanks,
Amin

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Wednesday, July 20, 2016 8:12 AM
> To: Tootoonchian, Amin <amin.tootoonchian@intel.com>
> Cc: dev@dpdk.org; Kerlin, MarcinX <marcinx.kerlin@intel.com>
> Subject: Re: [dpdk-dev] [PATCH] ethdev: ensure consistent port id assignment
> 
> Hi,
> 
> 2016-07-20 15:07, Tootoonchian, Amin:
> > Thomas, your thoughts?
> 
> I have 2 thoughts:
> - it is too big for 16.07
> - it is related to multi-process mechanism, maintained by Sergio ;)
> 
> Sorry I won't look at it shortly.

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

[Sergio Gonzalez Monroy]

Hi,

On 24/08/2016 23:17, amin.tootoonchian at intel.com (Tootoonchian, Amin) 
wrote:
> Sergio, could you please review this patch?
>
> Thanks,
> Amin

The patch status should be updated to 'Not Applicable' since similar 
functionality has been implemented (commit d948f596).
Only Primary processes are allowed to call rte_eth_dev_allocate(), while 
Secondary processes should call rte_eth_dev_attach_secondary()

Thanks,
Sergio

>> -----Original Message-----
>> From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com]
>> Sent: Wednesday, July 20, 2016 8:12 AM
>> To: Tootoonchian, Amin <amin.tootoonchian at intel.com>
>> Cc: dev at dpdk.org; Kerlin, MarcinX <marcinx.kerlin at intel.com>
>> Subject: Re: [dpdk-dev] [PATCH] ethdev: ensure consistent port id assignment
>>
>> Hi,
>>
>> 2016-07-20 15:07, Tootoonchian, Amin:
>>> Thomas, your thoughts?
>> I have 2 thoughts:
>> - it is too big for 16.07
>> - it is related to multi-process mechanism, maintained by Sergio ;)
>>
>> Sorry I won't look at it shortly.

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

[Ferruh Yigit]

On 9/4/2017 3:53 PM, sergio.gonzalez.monroy at intel.com (Sergio Gonzalez
Monroy) wrote:
> Hi,
> 
> On 24/08/2016 23:17, amin.tootoonchian at intel.com (Tootoonchian, Amin) 
> wrote:
>> Sergio, could you please review this patch?
>>
>> Thanks,
>> Amin
> 
> The patch status should be updated to 'Not Applicable' since similar 
> functionality has been implemented (commit d948f596).
> Only Primary processes are allowed to call rte_eth_dev_allocate(), while 
> Secondary processes should call rte_eth_dev_attach_secondary()

Agreed with comment, and even after that comment many thing changed in multi
process support. Patch status updated as 'Not Applicable'.

> 
> Thanks,
> Sergio
> 
>>> -----Original Message-----
>>> From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com]
>>> Sent: Wednesday, July 20, 2016 8:12 AM
>>> To: Tootoonchian, Amin <amin.tootoonchian at intel.com>
>>> Cc: dev at dpdk.org; Kerlin, MarcinX <marcinx.kerlin at intel.com>
>>> Subject: Re: [dpdk-dev] [PATCH] ethdev: ensure consistent port id assignment
>>>
>>> Hi,
>>>
>>> 2016-07-20 15:07, Tootoonchian, Amin:
>>>> Thomas, your thoughts?
>>> I have 2 thoughts:
>>> - it is too big for 16.07
>>> - it is related to multi-process mechanism, maintained by Sergio ;)
>>>
>>> Sorry I won't look at it shortly.
> 
> 
>