[PATCH 0/2] app/testpmd: improve multiprocess support

[PATCH 0/2] app/testpmd: improve multiprocess support

[Marcin Kerlin]

This patch ensure not overwrite device data in the multiprocess application.

1)Changes in the library introduces continuity in array rte_eth_dev_data[]
shared between all processes. Secondary process adds new entries in free
space instead of overwriting existing entries.

2)Changes in application testpmd allow secondary process to attach the mempool
created by primary process rather than create new and in the case of quit or
force quit to free devices data from shared array rte_eth_dev_data[].

Breaking ABI:
Changes in the library librte_ether causes extending existing structure 
rte_eth_dev_data with a new field lock. The reason is that this structure is
sharing between all the processes so it should be protected against attempting
to write from two different processes.

Tomasz Kulasek sent announce ABI change in librte_ether on 21 July 2016. I would
like to join to this breaking ABI, if it is possible.

Marcin Kerlin (2):
  librte_ether: ensure not overwrite device data in mp app
  app/testpmd: improve handling of multiprocess

 app/test-pmd/testpmd.c                 | 36 +++++++++++++-
 app/test-pmd/testpmd.h                 |  1 +
 lib/librte_ether/rte_ethdev.c          | 90 +++++++++++++++++++++++++++++++---
 lib/librte_ether/rte_ethdev.h          | 24 +++++++++
 lib/librte_ether/rte_ether_version.map |  7 +++
 5 files changed, 148 insertions(+), 10 deletions(-)

-- 
1.9.1

[PATCH 1/2] librte_ether: ensure not overwrite device data in mp app

[Marcin Kerlin]

Added prevention not overwrite device data in array rte_eth_dev_data[].
Secondary process appends in the first free place rather than at the
beginning. This behavior prevents overwriting devices of primary process
by secondary process.

Signed-off-by: Marcin Kerlin <marcinx.kerlin@intel.com>
---
 lib/librte_ether/rte_ethdev.c          | 90 +++++++++++++++++++++++++++++++---
 lib/librte_ether/rte_ethdev.h          | 24 +++++++++
 lib/librte_ether/rte_ether_version.map |  7 +++
 3 files changed, 113 insertions(+), 8 deletions(-)

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index f62a9ec..78e42bf 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -177,6 +177,19 @@ rte_eth_dev_allocated(const char *name)
 	return NULL;
 }
 
+struct rte_eth_dev_data *
+rte_eth_dev_data_allocated(const char *name)
+{
+	unsigned int i;
+
+	for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
+		if (strcmp(rte_eth_dev_data[i].name, name) == 0)
+			return &rte_eth_dev_data[i];
+	}
+
+	return NULL;
+}
+
 static uint8_t
 rte_eth_dev_find_free_port(void)
 {
@@ -189,10 +202,43 @@ rte_eth_dev_find_free_port(void)
 	return RTE_MAX_ETHPORTS;
 }
 
+static uint8_t
+rte_eth_dev_find_free_dev_data_id(void)
+{
+	unsigned int i;
+
+	for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
+		if (!strlen(rte_eth_dev_data[i].name))
+			return i;
+	}
+	return RTE_MAX_ETHPORTS;
+}
+
+int
+rte_eth_dev_release_dev_data(uint8_t port_id)
+{
+	char device[RTE_ETH_NAME_MAX_LEN];
+	struct rte_eth_dev_data *eth_dev_data = NULL;
+
+	/* get device name by port id */
+	if (rte_eth_dev_get_name_by_port(port_id, device))
+		return -EINVAL;
+
+	/* look for an entry in the device data */
+	eth_dev_data = rte_eth_dev_data_allocated(device);
+	if (eth_dev_data == NULL)
+		return -EINVAL;
+
+	/* clear an entry in the device data */
+	memset(eth_dev_data, 0, sizeof(struct rte_eth_dev_data));
+
+	return 0;
+}
+
 struct rte_eth_dev *
 rte_eth_dev_allocate(const char *name, enum rte_eth_dev_type type)
 {
-	uint8_t port_id;
+	uint8_t port_id, dev_data_id;
 	struct rte_eth_dev *eth_dev;
 
 	port_id = rte_eth_dev_find_free_port();
@@ -204,17 +250,35 @@ rte_eth_dev_allocate(const char *name, enum rte_eth_dev_type type)
 	if (rte_eth_dev_data == NULL)
 		rte_eth_dev_data_alloc();
 
+	do {
+		dev_data_id = rte_eth_dev_find_free_dev_data_id();
+	} while (!rte_spinlock_trylock(&rte_eth_dev_data[dev_data_id].lock)
+			&& dev_data_id < RTE_MAX_ETHPORTS);
+
+	if (dev_data_id == RTE_MAX_ETHPORTS) {
+		RTE_PMD_DEBUG_TRACE("Reached maximum number of Ethernet ports by all "
+				"the processes\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;
 	}
 
+	if (rte_eth_dev_data_allocated(name) != NULL) {
+		RTE_PMD_DEBUG_TRACE("Ethernet Device with name %s already allocated by "
+				"another process!\n", name);
+		return NULL;
+	}
+
 	eth_dev = &rte_eth_devices[port_id];
-	eth_dev->data = &rte_eth_dev_data[port_id];
+	eth_dev->data = &rte_eth_dev_data[dev_data_id];
 	snprintf(eth_dev->data->name, sizeof(eth_dev->data->name), "%s", name);
 	eth_dev->data->port_id = port_id;
 	eth_dev->attached = DEV_ATTACHED;
+	rte_spinlock_unlock(&eth_dev->data->lock);
 	eth_dev->dev_type = type;
 	nb_ports++;
 	return eth_dev;
@@ -418,9 +482,7 @@ rte_eth_dev_get_name_by_port(uint8_t port_id, char *name)
 		return -EINVAL;
 	}
 
-	/* shouldn't check 'rte_eth_devices[i].data',
-	 * because it might be overwritten by VDEV PMD */
-	tmp = rte_eth_dev_data[port_id].name;
+	tmp = rte_eth_devices[port_id].data->name;
 	strcpy(name, tmp);
 	return 0;
 }
@@ -439,9 +501,7 @@ rte_eth_dev_get_port_by_name(const char *name, uint8_t *port_id)
 
 	for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
 
-		if (!strncmp(name,
-			rte_eth_dev_data[i].name, strlen(name))) {
-
+		if (!strncmp(name, rte_eth_devices[i].data->name, strlen(name))) {
 			*port_id = i;
 
 			return 0;
@@ -632,6 +692,8 @@ int
 rte_eth_dev_detach(uint8_t port_id, char *name)
 {
 	struct rte_pci_addr addr;
+	struct rte_eth_dev_data *eth_dev_data = NULL;
+	char device[RTE_ETH_NAME_MAX_LEN];
 	int ret = -1;
 
 	if (name == NULL) {
@@ -643,6 +705,15 @@ rte_eth_dev_detach(uint8_t port_id, char *name)
 	if (rte_eth_dev_is_detachable(port_id))
 		goto err;
 
+	/* get device name by port id */
+	if (rte_eth_dev_get_name_by_port(port_id, device))
+		goto err;
+
+	/* look for an entry in the shared device data */
+	eth_dev_data = rte_eth_dev_data_allocated(device);
+	if (eth_dev_data == NULL)
+		goto err;
+
 	if (rte_eth_dev_get_device_type(port_id) == RTE_ETH_DEV_PCI) {
 		ret = rte_eth_dev_get_addr_by_port(port_id, &addr);
 		if (ret < 0)
@@ -662,6 +733,9 @@ rte_eth_dev_detach(uint8_t port_id, char *name)
 			goto err;
 	}
 
+	/* clear an entry in the shared device data */
+	memset(eth_dev_data, 0, sizeof(struct rte_eth_dev_data));
+
 	return 0;
 
 err:
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index b0fe033..c4efb2d 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -1704,6 +1704,7 @@ struct rte_eth_dev_data {
 	enum rte_kernel_driver kdrv;    /**< Kernel driver passthrough */
 	int numa_node;  /**< NUMA node connection */
 	const char *drv_name;   /**< Driver name */
+	rte_spinlock_t lock; /** Lock on allocate eth device */
 };
 
 /** Device supports hotplug detach */
@@ -1748,6 +1749,29 @@ struct rte_eth_dev *rte_eth_dev_allocated(const char *name);
 
 /**
  * @internal
+ * Returns a shared device data slot specified by the unique identifier name.
+ *
+ * @param	name
+ *  The pointer to the Unique identifier name for each shared Ethernet device
+ *  between multiple processes.
+ * @return
+ *   - The pointer to the device data slot, on success. NULL on error
+ */
+struct rte_eth_dev_data *rte_eth_dev_data_allocated(const char *name);
+
+/**
+ * @internal
+ * Release device data kept in shared memory for all processes.
+ *
+ * @param	port_id
+ *   The port identifier of the device to release device data.
+ * @return
+ *   - 0 on success, negative on error
+ */
+int rte_eth_dev_release_dev_data(uint8_t port_id);
+
+/**
+ * @internal
  * Allocates a new ethdev slot for an ethernet device and returns the pointer
  * to that slot for the driver to use.
  *
diff --git a/lib/librte_ether/rte_ether_version.map b/lib/librte_ether/rte_ether_version.map
index 45ddf44..34e1109 100644
--- a/lib/librte_ether/rte_ether_version.map
+++ b/lib/librte_ether/rte_ether_version.map
@@ -139,3 +139,10 @@ DPDK_16.07 {
 	rte_eth_dev_get_port_by_name;
 	rte_eth_xstats_get_names;
 } DPDK_16.04;
+
+DPDK_16.11 {
+	global:
+
+	rte_eth_dev_data_allocated;
+	rte_eth_dev_release_dev_data;
+} DPDK_16.07
-- 
1.9.1

Re: [PATCH 1/2] librte_ether: ensure not overwrite device data in mp app

[Yuanhan Liu]

On Fri, Sep 02, 2016 at 10:58:29AM +0200, Marcin Kerlin wrote:
> Added prevention not overwrite device data in array rte_eth_dev_data[].
> Secondary process appends in the first free place rather than at the
> beginning. This behavior prevents overwriting devices of primary process
> by secondary process.
> 
> Signed-off-by: Marcin Kerlin <marcinx.kerlin@intel.com>
> diff --git a/lib/librte_ether/rte_ether_version.map b/lib/librte_ether/rte_ether_version.map
> index 45ddf44..34e1109 100644
> --- a/lib/librte_ether/rte_ether_version.map
> +++ b/lib/librte_ether/rte_ether_version.map
> @@ -139,3 +139,10 @@ DPDK_16.07 {
>  	rte_eth_dev_get_port_by_name;
>  	rte_eth_xstats_get_names;
>  } DPDK_16.04;
> +
> +DPDK_16.11 {
> +	global:
> +
> +	rte_eth_dev_data_allocated;
> +	rte_eth_dev_release_dev_data;
> +} DPDK_16.07

Hi,

FYI, my testrobot caught some errors when this patch is applied.

        --yliu

---
i686-native-linuxapp-gcc: config-all-yes-shared
===============================================
/usr/bin/ld:/yeti/vm/ubuntu-initrd-16.04-i386-build/dpdk/lib/librte_ether/rte_ether_version.map:149: syntax error in VERSION script
collect2: error: ld returned 1 exit status
make[5]: *** [libethdev.so.4.1] Error 1
make[4]: *** [librte_ether] Error 2
make[3]: *** [lib] Error 2
make[2]: *** [all] Error 2
make[1]: *** [pre_install] Error 2
make: *** [install] Error 2
error: build failed

[PATCH v2 0/2] app/testpmd: improve multiprocess support

[Marcin Kerlin]

This patch ensure not overwrite device data in the multiprocess application.

1)Changes in the library introduces continuity in array rte_eth_dev_data[]
shared between all processes. Secondary process adds new entries in free
space instead of overwriting existing entries.

2)Changes in application testpmd allow secondary process to attach the mempool
created by primary process rather than create new and in the case of quit or
force quit to free devices data from shared array rte_eth_dev_data[].

Breaking ABI:
Changes in the library librte_ether causes extending existing structure
rte_eth_dev_data with a new field lock. The reason is that this structure
is sharing between all the processes so it should be protected against
attempting to write from two different processes.

Tomasz Kulasek sent announce ABI change in librte_ether on 21 July 2016.
I would like to join to this breaking ABI, if it is possible.

v2:
* fix syntax error in version script

Marcin Kerlin (2):
  librte_ether: ensure not overwrite device data in mp app
  app/testpmd: improve handling of multiprocess

 app/test-pmd/testpmd.c                 | 36 +++++++++++++-
 app/test-pmd/testpmd.h                 |  1 +
 lib/librte_ether/rte_ethdev.c          | 90 +++++++++++++++++++++++++++++++---
 lib/librte_ether/rte_ethdev.h          | 24 +++++++++
 lib/librte_ether/rte_ether_version.map |  7 +++
 5 files changed, 148 insertions(+), 10 deletions(-)

-- 
1.9.1

[PATCH v2 0/2] app/testpmd: improve multiprocess support

[Marcin Kerlin]

This patch ensure not overwrite device data in the multiprocess application.

1)Changes in the library introduces continuity in array rte_eth_dev_data[]
shared between all processes. Secondary process adds new entries in free
space instead of overwriting existing entries.

2)Changes in application testpmd allow secondary process to attach the mempool
created by primary process rather than create new and in the case of quit or
force quit to free devices data from shared array rte_eth_dev_data[].

Breaking ABI:
Changes in the library librte_ether causes extending existing structure
rte_eth_dev_data with a new field lock. The reason is that this structure
is sharing between all the processes so it should be protected against
attempting to write from two different processes.

Tomasz Kulasek sent announce ABI change in librte_ether on 21 July 2016.
I would like to join to this breaking ABI, if it is possible.

v2:
* fix syntax error in version script

Marcin Kerlin (2):
  librte_ether: ensure not overwrite device data in mp app
  app/testpmd: improve handling of multiprocess

 app/test-pmd/testpmd.c                 | 36 +++++++++++++-
 app/test-pmd/testpmd.h                 |  1 +
 lib/librte_ether/rte_ethdev.c          | 90 +++++++++++++++++++++++++++++++---
 lib/librte_ether/rte_ethdev.h          | 24 +++++++++
 lib/librte_ether/rte_ether_version.map |  7 +++
 5 files changed, 148 insertions(+), 10 deletions(-)

-- 
1.9.1

[PATCH v2 1/2] librte_ether: ensure not overwrite device data in mp app

[Marcin Kerlin]

Added prevention not overwrite device data in array rte_eth_dev_data[].
Secondary process appends in the first free place rather than at the
beginning. This behavior prevents overwriting devices of primary process
by secondary process.

Signed-off-by: Marcin Kerlin <marcinx.kerlin@intel.com>
---
 lib/librte_ether/rte_ethdev.c          | 90 +++++++++++++++++++++++++++++++---
 lib/librte_ether/rte_ethdev.h          | 24 +++++++++
 lib/librte_ether/rte_ether_version.map |  7 +++
 3 files changed, 113 insertions(+), 8 deletions(-)

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index 382c959..dadd77a 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -176,6 +176,19 @@ rte_eth_dev_allocated(const char *name)
 	return NULL;
 }
 
+struct rte_eth_dev_data *
+rte_eth_dev_data_allocated(const char *name)
+{
+	unsigned int i;
+
+	for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
+		if (strcmp(rte_eth_dev_data[i].name, name) == 0)
+			return &rte_eth_dev_data[i];
+	}
+
+	return NULL;
+}
+
 static uint8_t
 rte_eth_dev_find_free_port(void)
 {
@@ -188,10 +201,43 @@ rte_eth_dev_find_free_port(void)
 	return RTE_MAX_ETHPORTS;
 }
 
+static uint8_t
+rte_eth_dev_find_free_dev_data_id(void)
+{
+	unsigned int i;
+
+	for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
+		if (!strlen(rte_eth_dev_data[i].name))
+			return i;
+	}
+	return RTE_MAX_ETHPORTS;
+}
+
+int
+rte_eth_dev_release_dev_data(uint8_t port_id)
+{
+	char device[RTE_ETH_NAME_MAX_LEN];
+	struct rte_eth_dev_data *eth_dev_data = NULL;
+
+	/* get device name by port id */
+	if (rte_eth_dev_get_name_by_port(port_id, device))
+		return -EINVAL;
+
+	/* look for an entry in the device data */
+	eth_dev_data = rte_eth_dev_data_allocated(device);
+	if (eth_dev_data == NULL)
+		return -EINVAL;
+
+	/* clear an entry in the device data */
+	memset(eth_dev_data, 0, sizeof(struct rte_eth_dev_data));
+
+	return 0;
+}
+
 struct rte_eth_dev *
 rte_eth_dev_allocate(const char *name, enum rte_eth_dev_type type)
 {
-	uint8_t port_id;
+	uint8_t port_id, dev_data_id;
 	struct rte_eth_dev *eth_dev;
 
 	port_id = rte_eth_dev_find_free_port();
@@ -203,17 +249,35 @@ rte_eth_dev_allocate(const char *name, enum rte_eth_dev_type type)
 	if (rte_eth_dev_data == NULL)
 		rte_eth_dev_data_alloc();
 
+	do {
+		dev_data_id = rte_eth_dev_find_free_dev_data_id();
+	} while (!rte_spinlock_trylock(&rte_eth_dev_data[dev_data_id].lock)
+			&& dev_data_id < RTE_MAX_ETHPORTS);
+
+	if (dev_data_id == RTE_MAX_ETHPORTS) {
+		RTE_PMD_DEBUG_TRACE("Reached maximum number of Ethernet ports by all "
+				"the processes\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;
 	}
 
+	if (rte_eth_dev_data_allocated(name) != NULL) {
+		RTE_PMD_DEBUG_TRACE("Ethernet Device with name %s already allocated by "
+				"another process!\n", name);
+		return NULL;
+	}
+
 	eth_dev = &rte_eth_devices[port_id];
-	eth_dev->data = &rte_eth_dev_data[port_id];
+	eth_dev->data = &rte_eth_dev_data[dev_data_id];
 	snprintf(eth_dev->data->name, sizeof(eth_dev->data->name), "%s", name);
 	eth_dev->data->port_id = port_id;
 	eth_dev->attached = DEV_ATTACHED;
+	rte_spinlock_unlock(&eth_dev->data->lock);
 	eth_dev->dev_type = type;
 	nb_ports++;
 	return eth_dev;
@@ -417,9 +481,7 @@ rte_eth_dev_get_name_by_port(uint8_t port_id, char *name)
 		return -EINVAL;
 	}
 
-	/* shouldn't check 'rte_eth_devices[i].data',
-	 * because it might be overwritten by VDEV PMD */
-	tmp = rte_eth_dev_data[port_id].name;
+	tmp = rte_eth_devices[port_id].data->name;
 	strcpy(name, tmp);
 	return 0;
 }
@@ -438,9 +500,7 @@ rte_eth_dev_get_port_by_name(const char *name, uint8_t *port_id)
 
 	for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
 
-		if (!strncmp(name,
-			rte_eth_dev_data[i].name, strlen(name))) {
-
+		if (!strncmp(name, rte_eth_devices[i].data->name, strlen(name))) {
 			*port_id = i;
 
 			return 0;
@@ -631,6 +691,8 @@ int
 rte_eth_dev_detach(uint8_t port_id, char *name)
 {
 	struct rte_pci_addr addr;
+	struct rte_eth_dev_data *eth_dev_data = NULL;
+	char device[RTE_ETH_NAME_MAX_LEN];
 	int ret = -1;
 
 	if (name == NULL) {
@@ -642,6 +704,15 @@ rte_eth_dev_detach(uint8_t port_id, char *name)
 	if (rte_eth_dev_is_detachable(port_id))
 		goto err;
 
+	/* get device name by port id */
+	if (rte_eth_dev_get_name_by_port(port_id, device))
+		goto err;
+
+	/* look for an entry in the shared device data */
+	eth_dev_data = rte_eth_dev_data_allocated(device);
+	if (eth_dev_data == NULL)
+		goto err;
+
 	if (rte_eth_dev_get_device_type(port_id) == RTE_ETH_DEV_PCI) {
 		ret = rte_eth_dev_get_addr_by_port(port_id, &addr);
 		if (ret < 0)
@@ -661,6 +732,9 @@ rte_eth_dev_detach(uint8_t port_id, char *name)
 			goto err;
 	}
 
+	/* clear an entry in the shared device data */
+	memset(eth_dev_data, 0, sizeof(struct rte_eth_dev_data));
+
 	return 0;
 
 err:
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index 96575e8..541e44e 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -1708,6 +1708,7 @@ struct rte_eth_dev_data {
 	enum rte_kernel_driver kdrv;    /**< Kernel driver passthrough */
 	int numa_node;  /**< NUMA node connection */
 	const char *drv_name;   /**< Driver name */
+	rte_spinlock_t lock; /** Lock on allocate eth device */
 };
 
 /** Device supports hotplug detach */
@@ -1752,6 +1753,29 @@ struct rte_eth_dev *rte_eth_dev_allocated(const char *name);
 
 /**
  * @internal
+ * Returns a shared device data slot specified by the unique identifier name.
+ *
+ * @param	name
+ *  The pointer to the Unique identifier name for each shared Ethernet device
+ *  between multiple processes.
+ * @return
+ *   - The pointer to the device data slot, on success. NULL on error
+ */
+struct rte_eth_dev_data *rte_eth_dev_data_allocated(const char *name);
+
+/**
+ * @internal
+ * Release device data kept in shared memory for all processes.
+ *
+ * @param	port_id
+ *   The port identifier of the device to release device data.
+ * @return
+ *   - 0 on success, negative on error
+ */
+int rte_eth_dev_release_dev_data(uint8_t port_id);
+
+/**
+ * @internal
  * Allocates a new ethdev slot for an ethernet device and returns the pointer
  * to that slot for the driver to use.
  *
diff --git a/lib/librte_ether/rte_ether_version.map b/lib/librte_ether/rte_ether_version.map
index 45ddf44..f37a15c 100644
--- a/lib/librte_ether/rte_ether_version.map
+++ b/lib/librte_ether/rte_ether_version.map
@@ -139,3 +139,10 @@ DPDK_16.07 {
 	rte_eth_dev_get_port_by_name;
 	rte_eth_xstats_get_names;
 } DPDK_16.04;
+
+DPDK_16.11 {
+	global:
+
+	rte_eth_dev_data_allocated;
+	rte_eth_dev_release_dev_data;
+} DPDK_16.07;
-- 
1.9.1

Re: [PATCH v2 1/2] librte_ether: ensure not overwrite device data in mp app

[Pattan, Reshma]

Hi Marcin,



>  /**
>   * @internal
> + * Returns a shared device data slot specified by the unique identifier name.
> + *
> + * @param	name
> + *  The pointer to the Unique identifier name for each shared Ethernet
> +device
> + *  between multiple processes.
> + * @return
> + *   - The pointer to the device data slot, on success. NULL on error
> + */
> +struct rte_eth_dev_data *rte_eth_dev_data_allocated(const char *name);

This should be static function in source file rather than public function.
And name can be rte_eth_dev_get_dev_by_name() something like that?

Thanks,
Reshma

Re: [PATCH v2 1/2] librte_ether: ensure not overwrite device data in mp app

[Kerlin, MarcinX]

Hi Reshma,

> -----Original Message-----
> From: Pattan, Reshma
> Sent: Tuesday, September 20, 2016 6:14 PM
> To: Kerlin, MarcinX <marcinx.kerlin@intel.com>; dev@dpdk.org
> Cc: thomas.monjalon@6wind.com; De Lara Guarch, Pablo
> <pablo.de.lara.guarch@intel.com>; Kerlin, MarcinX
> <marcinx.kerlin@intel.com>
> Subject: RE: [dpdk-dev] [PATCH v2 1/2] librte_ether: ensure not overwrite
> device data in mp app
> 
> Hi Marcin,
> 
> 
> 
> >  /**
> >   * @internal
> > + * Returns a shared device data slot specified by the unique identifier name.
> > + *
> > + * @param	name
> > + *  The pointer to the Unique identifier name for each shared
> > +Ethernet device
> > + *  between multiple processes.
> > + * @return
> > + *   - The pointer to the device data slot, on success. NULL on error
> > + */
> > +struct rte_eth_dev_data *rte_eth_dev_data_allocated(const char
> > +*name);
> 
> This should be static function in source file rather than public function.
> And name can be rte_eth_dev_get_dev_by_name() something like that?

1) Yes should be, this function is not using outside lib now, thanks
2) My proposition is rte_eth_dev_get_dev_data_by_name(), because it is related with device data structure.. Do you have any objections Thomas?

I am waiting for still some objections and then prepare v3

Regards,
Marcin

> 
> Thanks,
> Reshma

Re: [PATCH v2 1/2] librte_ether: ensure not overwrite device data in mp app

[Thomas Monjalon]

2016-09-22 14:11, Kerlin, MarcinX:
> Hi Reshma,
> 
> From: Pattan, Reshma
> > 
> > Hi Marcin,
> > 
> > >  /**
> > >   * @internal
> > > + * Returns a shared device data slot specified by the unique identifier name.
> > > + *
> > > + * @param	name
> > > + *  The pointer to the Unique identifier name for each shared
> > > +Ethernet device
> > > + *  between multiple processes.
> > > + * @return
> > > + *   - The pointer to the device data slot, on success. NULL on error
> > > + */
> > > +struct rte_eth_dev_data *rte_eth_dev_data_allocated(const char
> > > +*name);
> > 
> > This should be static function in source file rather than public function.
> > And name can be rte_eth_dev_get_dev_by_name() something like that?
> 
> 1) Yes should be, this function is not using outside lib now, thanks
> 2) My proposition is rte_eth_dev_get_dev_data_by_name(), because it is related with device data structure.. Do you have any objections Thomas?

No objection on the name.
But the whole patch looks strange.

> I am waiting for still some objections and then prepare v3

Please could you better state the problem you want to solve in the
messages of each v3 patch?
I'll try to understand and review the v3.

Thanks

Re: [PATCH v2 1/2] librte_ether: ensure not overwrite device data in mp app

[Kerlin, MarcinX]

Hi Thomas,

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Friday, September 23, 2016 4:13 PM
> To: Kerlin, MarcinX <marcinx.kerlin@intel.com>
> Cc: Pattan, Reshma <reshma.pattan@intel.com>; dev@dpdk.org; De Lara
> Guarch, Pablo <pablo.de.lara.guarch@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v2 1/2] librte_ether: ensure not overwrite
> device data in mp app
> 
> 2016-09-22 14:11, Kerlin, MarcinX:
> > Hi Reshma,
> >
> > From: Pattan, Reshma
> > >
> > > Hi Marcin,
> > >
> > > >  /**
> > > >   * @internal
> > > > + * Returns a shared device data slot specified by the unique identifier
> name.
> > > > + *
> > > > + * @param	name
> > > > + *  The pointer to the Unique identifier name for each shared
> > > > +Ethernet device
> > > > + *  between multiple processes.
> > > > + * @return
> > > > + *   - The pointer to the device data slot, on success. NULL on error
> > > > + */
> > > > +struct rte_eth_dev_data *rte_eth_dev_data_allocated(const char
> > > > +*name);
> > >
> > > This should be static function in source file rather than public function.
> > > And name can be rte_eth_dev_get_dev_by_name() something like that?
> >
> > 1) Yes should be, this function is not using outside lib now, thanks
> > 2) My proposition is rte_eth_dev_get_dev_data_by_name(), because it is
> related with device data structure.. Do you have any objections Thomas?
> 
> No objection on the name.
> But the whole patch looks strange.
> 
> > I am waiting for still some objections and then prepare v3
> 
> Please could you better state the problem you want to solve in the messages of
> each v3 patch?
> I'll try to understand and review the v3.

you're right, description without example is hard to quickly understand.

I added to cover letter how to reproduce the bug, how it affects on applications and 
how it is repaired in patch.  

I hope that it will clarify problem.

Regards,
Marcin
> 
> Thanks

Re: [PATCH v2 1/2] librte_ether: ensure not overwrite device data in mp app

[Pattan, Reshma]

Hi,

> +
> +	if (dev_data_id == RTE_MAX_ETHPORTS) {
> +		RTE_PMD_DEBUG_TRACE("Reached maximum number of
> Ethernet ports by all "
> +				"the processes\n");
> +		return NULL;
> +	}
> +
> 

Can the log message be changed to ("Cannot allocate more than %d number of devices ", RTE_MAX_ETHPORTS).
Instead of mentioning about the processes?

Thanks,
Reshma

Re: [PATCH v2 1/2] librte_ether: ensure not overwrite device data in mp app

[Kerlin, MarcinX]

Hi,

> -----Original Message-----
> From: Pattan, Reshma
> Sent: Tuesday, September 20, 2016 6:48 PM
> To: Kerlin, MarcinX <marcinx.kerlin@intel.com>; dev@dpdk.org
> Cc: thomas.monjalon@6wind.com; De Lara Guarch, Pablo
> <pablo.de.lara.guarch@intel.com>; Kerlin, MarcinX
> <marcinx.kerlin@intel.com>
> Subject: RE: [dpdk-dev] [PATCH v2 1/2] librte_ether: ensure not overwrite
> device data in mp app
> 
> Hi,
> 
> > +
> > +	if (dev_data_id == RTE_MAX_ETHPORTS) {
> > +		RTE_PMD_DEBUG_TRACE("Reached maximum number of
> > Ethernet ports by all "
> > +				"the processes\n");
> > +		return NULL;
> > +	}
> > +
> >
> 
> Can the log message be changed to ("Cannot allocate more than %d number of
> devices ", RTE_MAX_ETHPORTS).
> Instead of mentioning about the processes?

First message announces that it exceeded the limit in one application:
1)
	if (port_id == RTE_MAX_ETHPORTS) {
		RTE_PMD_DEBUG_TRACE("Reached maximum number of Ethernet ports\n");
		return NULL;
	}

Second announces that it exceeded the limit in all applications:
2)
	if (dev_data_id == RTE_MAX_ETHPORTS) {
		RTE_PMD_DEBUG_TRACE("Reached maximum number of Ethernet ports by all "
				"the processes\n");
		return NULL;
	}


"Cannot allocate more than %d number of devices" In my opinion this message is general and 
says nothing other than first (1).

Maybe only leave such a message instead of the above 2, which check only common array as 
common array will be faster filled up than local to each process

	if (dev_data_id == RTE_MAX_ETHPORTS) {
		RTE_PMD_DEBUG_TRACE("Cannot allocate more than %d number of devices \n");
		return NULL;
	}

Regards,
Marcin

> 
> Thanks,
> Reshma

[PATCH v3 0/2] app/testpmd: improve multiprocess support

[Marcin Kerlin]

This patch ensure not overwrite device data in the multiprocess application.

1)Changes in the library introduces continuity in array rte_eth_dev_data[]
shared between all processes. Secondary process adds new entries in free
space instead of overwriting existing entries.

2)Changes in application testpmd allow secondary process to attach the
mempool created by primary process rather than create new and in the case of
quit or force quit to free devices data from shared array rte_eth_dev_data[].

-------------------------
How to reproduce the bug:

1) Run primary process:
-c 0xf -n 4 --socket-mem='512,0' -w 03:00.1 -w 03:00.0 --proc-type=primary 
--file-prefix=xz1 -- -i

(gdb) print rte_eth_devices[0].data.name
$52 = "3:0.1"
(gdb) print rte_eth_devices[1].data.name
$53 = "3:0.0"

2) Run secondary process:
-c 0xf0 --socket-mem='512,0' -n 4 -v -b 03:00.1 -b 03:00.0 -b 01:00.0 -b 01:00.0
--vdev 'eth_pcap0,rx_pcap=/var/log/device1.pcap,tx_pcap=/var/log/device2.pcap'
--proc-type=secondary --file-prefix=xz1 -- -i

(gdb) print rte_eth_devices[0].data.name
$52 = "eth_pcap0"
(gdb) print rte_eth_devices[1].data.name
$53 = "eth_pcap1"

3) Go back to the primary and re-check:
(gdb) print rte_eth_devices[0].data.name
$54 = "eth_pcap0"
(gdb) print rte_eth_devices[1].data.name
$55 = "eth_pcap1"

It means that secondary process overwrite data of primary process.

This patch fix it and now if we go back to the primary and re-check then 
everything is fine:
(gdb) print rte_eth_devices[0].data.name
$56 = "3:0.1"
(gdb) print rte_eth_devices[1].data.name
$57 = "3:0.0"

So after this fix structure rte_eth_dev_data[] will keep all data one 
after the other instead of overwriting:

(gdb) print rte_eth_dev_data[0].name
$52 = "3:0.1"
(gdb) print rte_eth_dev_data[1].name
$53 = "3:0.0"
(gdb) print rte_eth_dev_data[2].name
$54 = "eth_pcap0"
(gdb) print rte_eth_dev_data[3].name
$55 = "eth_pcap1"
and so on will be append in the next indexes

If secondary process will be turned off then also will be deleted from array:
(gdb) print rte_eth_dev_data[0].name
$52 = "3:0.1"
(gdb) print rte_eth_dev_data[1].name
$53 = "3:0.0"
(gdb) print rte_eth_dev_data[2].name
$54 = ""
(gdb) print rte_eth_dev_data[3].name
$55 = ""
this also allows re-use index 2 and 3 for next another process
-------------------------

Breaking ABI:
Changes in the library librte_ether causes extending existing structure
rte_eth_dev_data with a new field lock. The reason is that this structure is
sharing between all the processes so it should be protected against attempting
to write from two different processes.

Tomasz Kulasek sent announce ABI change in librte_ether on 21 July 2016.
I would like to join to this breaking ABI, if it is possible.

v2:
* fix syntax error in version script
v3:
* changed scope of function
* improved description

Marcin Kerlin (2):
  librte_ether: ensure not overwrite device data in mp app
  app/testpmd: improve handling of multiprocess

 app/test-pmd/testpmd.c                 | 36 +++++++++++++-
 app/test-pmd/testpmd.h                 |  1 +
 lib/librte_ether/rte_ethdev.c          | 90 +++++++++++++++++++++++++++++++---
 lib/librte_ether/rte_ethdev.h          | 24 +++++++++
 lib/librte_ether/rte_ether_version.map |  7 +++
 5 files changed, 148 insertions(+), 10 deletions(-)

-- 
1.9.1

[PATCH v3 1/2] librte_ether: ensure not overwrite device data in mp app

[Marcin Kerlin]

Added prevention not overwrite device data in array rte_eth_dev_data[].
Secondary process appends in the first free place rather than at the
beginning. This behavior prevents overwriting devices data of primary process
by secondary process.

Signed-off-by: Marcin Kerlin <marcinx.kerlin@intel.com>
---
 lib/librte_ether/rte_ethdev.c          | 90 +++++++++++++++++++++++++++++++---
 lib/librte_ether/rte_ethdev.h          | 24 +++++++++
 lib/librte_ether/rte_ether_version.map |  7 +++
 3 files changed, 113 insertions(+), 8 deletions(-)

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index f62a9ec..faaa170 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -177,6 +177,19 @@ rte_eth_dev_allocated(const char *name)
 	return NULL;
 }
 
+static struct rte_eth_dev_data *
+rte_eth_dev_get_dev_data_by_name(const char *name)
+{
+	unsigned int i;
+
+	for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
+		if (strcmp(rte_eth_dev_data[i].name, name) == 0)
+			return &rte_eth_dev_data[i];
+	}
+
+	return NULL;
+}
+
 static uint8_t
 rte_eth_dev_find_free_port(void)
 {
@@ -189,10 +202,43 @@ rte_eth_dev_find_free_port(void)
 	return RTE_MAX_ETHPORTS;
 }
 
+static uint8_t
+rte_eth_dev_find_free_dev_data_id(void)
+{
+	unsigned int i;
+
+	for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
+		if (!strlen(rte_eth_dev_data[i].name))
+			return i;
+	}
+	return RTE_MAX_ETHPORTS;
+}
+
+int
+rte_eth_dev_release_dev_data(uint8_t port_id)
+{
+	char device[RTE_ETH_NAME_MAX_LEN];
+	struct rte_eth_dev_data *eth_dev_data = NULL;
+
+	/* get device name by port id */
+	if (rte_eth_dev_get_name_by_port(port_id, device))
+		return -EINVAL;
+
+	/* look for an entry in the device data */
+	eth_dev_data = rte_eth_dev_get_dev_data_by_name(device);
+	if (eth_dev_data == NULL)
+		return -EINVAL;
+
+	/* clear an entry in the device data */
+	memset(eth_dev_data, 0, sizeof(struct rte_eth_dev_data));
+
+	return 0;
+}
+
 struct rte_eth_dev *
 rte_eth_dev_allocate(const char *name, enum rte_eth_dev_type type)
 {
-	uint8_t port_id;
+	uint8_t port_id, dev_data_id;
 	struct rte_eth_dev *eth_dev;
 
 	port_id = rte_eth_dev_find_free_port();
@@ -204,17 +250,35 @@ rte_eth_dev_allocate(const char *name, enum rte_eth_dev_type type)
 	if (rte_eth_dev_data == NULL)
 		rte_eth_dev_data_alloc();
 
+	do {
+		dev_data_id = rte_eth_dev_find_free_dev_data_id();
+	} while (!rte_spinlock_trylock(&rte_eth_dev_data[dev_data_id].lock)
+			&& dev_data_id < RTE_MAX_ETHPORTS);
+
+	if (dev_data_id == RTE_MAX_ETHPORTS) {
+		RTE_PMD_DEBUG_TRACE("Reached maximum number of Ethernet ports by all "
+				"the processes\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;
 	}
 
+	if (rte_eth_dev_get_dev_data_by_name(name) != NULL) {
+		RTE_PMD_DEBUG_TRACE("Ethernet Device with name %s already allocated by "
+				"another process!\n", name);
+		return NULL;
+	}
+
 	eth_dev = &rte_eth_devices[port_id];
-	eth_dev->data = &rte_eth_dev_data[port_id];
+	eth_dev->data = &rte_eth_dev_data[dev_data_id];
 	snprintf(eth_dev->data->name, sizeof(eth_dev->data->name), "%s", name);
 	eth_dev->data->port_id = port_id;
 	eth_dev->attached = DEV_ATTACHED;
+	rte_spinlock_unlock(&eth_dev->data->lock);
 	eth_dev->dev_type = type;
 	nb_ports++;
 	return eth_dev;
@@ -418,9 +482,7 @@ rte_eth_dev_get_name_by_port(uint8_t port_id, char *name)
 		return -EINVAL;
 	}
 
-	/* shouldn't check 'rte_eth_devices[i].data',
-	 * because it might be overwritten by VDEV PMD */
-	tmp = rte_eth_dev_data[port_id].name;
+	tmp = rte_eth_devices[port_id].data->name;
 	strcpy(name, tmp);
 	return 0;
 }
@@ -439,9 +501,7 @@ rte_eth_dev_get_port_by_name(const char *name, uint8_t *port_id)
 
 	for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
 
-		if (!strncmp(name,
-			rte_eth_dev_data[i].name, strlen(name))) {
-
+		if (!strncmp(name, rte_eth_devices[i].data->name, strlen(name))) {
 			*port_id = i;
 
 			return 0;
@@ -632,6 +692,8 @@ int
 rte_eth_dev_detach(uint8_t port_id, char *name)
 {
 	struct rte_pci_addr addr;
+	struct rte_eth_dev_data *eth_dev_data = NULL;
+	char device[RTE_ETH_NAME_MAX_LEN];
 	int ret = -1;
 
 	if (name == NULL) {
@@ -643,6 +705,15 @@ rte_eth_dev_detach(uint8_t port_id, char *name)
 	if (rte_eth_dev_is_detachable(port_id))
 		goto err;
 
+	/* get device name by port id */
+	if (rte_eth_dev_get_name_by_port(port_id, device))
+		goto err;
+
+	/* look for an entry in the shared device data */
+	eth_dev_data = rte_eth_dev_get_dev_data_by_name(device);
+	if (eth_dev_data == NULL)
+		goto err;
+
 	if (rte_eth_dev_get_device_type(port_id) == RTE_ETH_DEV_PCI) {
 		ret = rte_eth_dev_get_addr_by_port(port_id, &addr);
 		if (ret < 0)
@@ -662,6 +733,9 @@ rte_eth_dev_detach(uint8_t port_id, char *name)
 			goto err;
 	}
 
+	/* clear an entry in the shared device data */
+	memset(eth_dev_data, 0, sizeof(struct rte_eth_dev_data));
+
 	return 0;
 
 err:
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index b0fe033..c4efb2d 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -1704,6 +1704,7 @@ struct rte_eth_dev_data {
 	enum rte_kernel_driver kdrv;    /**< Kernel driver passthrough */
 	int numa_node;  /**< NUMA node connection */
 	const char *drv_name;   /**< Driver name */
+	rte_spinlock_t lock; /** Lock on allocate eth device */
 };
 
 /** Device supports hotplug detach */
@@ -1748,6 +1749,29 @@ struct rte_eth_dev *rte_eth_dev_allocated(const char *name);
 
 /**
  * @internal
+ * Returns a shared device data slot specified by the unique identifier name.
+ *
+ * @param	name
+ *  The pointer to the Unique identifier name for each shared Ethernet device
+ *  between multiple processes.
+ * @return
+ *   - The pointer to the shared ethdev slot, on success. NULL on error
+ */
+struct rte_eth_dev_data *rte_eth_dev_data_allocated(const char *name);
+
+/**
+ * @internal
+ * Release device data kept in shared memory for all processes.
+ *
+ * @param	port_id
+ *   The port identifier of the device to release device data.
+ * @return
+ *   - 0 on success, negative on error
+ */
+int rte_eth_dev_release_dev_data(uint8_t port_id);
+
+/**
+ * @internal
  * Allocates a new ethdev slot for an ethernet device and returns the pointer
  * to that slot for the driver to use.
  *
diff --git a/lib/librte_ether/rte_ether_version.map b/lib/librte_ether/rte_ether_version.map
index 45ddf44..34e1109 100644
--- a/lib/librte_ether/rte_ether_version.map
+++ b/lib/librte_ether/rte_ether_version.map
@@ -139,3 +139,10 @@ DPDK_16.07 {
 	rte_eth_dev_get_port_by_name;
 	rte_eth_xstats_get_names;
 } DPDK_16.04;
+
+DPDK_16.11 {
+	global:
+
+	rte_eth_dev_data_allocated;
+	rte_eth_dev_release_dev_data;
+} DPDK_16.07
-- 
1.9.1

Re: [PATCH v3 1/2] librte_ether: ensure not overwrite device data in mp app

[Yuanhan Liu]

On Mon, Sep 26, 2016 at 04:53:05PM +0200, Marcin Kerlin wrote:
> diff --git a/lib/librte_ether/rte_ether_version.map b/lib/librte_ether/rte_ether_version.map
> index 45ddf44..34e1109 100644
> --- a/lib/librte_ether/rte_ether_version.map
> +++ b/lib/librte_ether/rte_ether_version.map
> @@ -139,3 +139,10 @@ DPDK_16.07 {
>  	rte_eth_dev_get_port_by_name;
>  	rte_eth_xstats_get_names;
>  } DPDK_16.04;
> +
> +DPDK_16.11 {
> +	global:
> +
> +	rte_eth_dev_data_allocated;
> +	rte_eth_dev_release_dev_data;
> +} DPDK_16.07

Interestingly, my robot caught an error (missing ';') with your v1,
and you fixed it in v2. Now you broke it again in v3.

	--yliu

Re: [PATCH v3 1/2] librte_ether: ensure not overwrite device data in mp app

[Kerlin, MarcinX]

Hi Liu

> -----Original Message-----
> From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
> Sent: Tuesday, September 27, 2016 5:07 AM
> To: Kerlin, MarcinX <marcinx.kerlin@intel.com>
> Cc: dev@dpdk.org; De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>;
> thomas.monjalon@6wind.com
> Subject: Re: [dpdk-dev] [PATCH v3 1/2] librte_ether: ensure not overwrite
> device data in mp app
> 
> On Mon, Sep 26, 2016 at 04:53:05PM +0200, Marcin Kerlin wrote:
> > diff --git a/lib/librte_ether/rte_ether_version.map
> > b/lib/librte_ether/rte_ether_version.map
> > index 45ddf44..34e1109 100644
> > --- a/lib/librte_ether/rte_ether_version.map
> > +++ b/lib/librte_ether/rte_ether_version.map
> > @@ -139,3 +139,10 @@ DPDK_16.07 {
> >  	rte_eth_dev_get_port_by_name;
> >  	rte_eth_xstats_get_names;
> >  } DPDK_16.04;
> > +
> > +DPDK_16.11 {
> > +	global:
> > +
> > +	rte_eth_dev_data_allocated;
> > +	rte_eth_dev_release_dev_data;
> > +} DPDK_16.07
> 
> Interestingly, my robot caught an error (missing ';') with your v1, and you fixed
> it in v2. Now you broke it again in v3.

I did format-patch on the older branch without fix semicolon.. Thanks, your robot is a great tool.

Regards,
Marcin

> 
> 	--yliu

[PATCH v4 0/2] app/testpmd: improve multiprocess support

[Marcin Kerlin]

This patch ensure not overwrite device data in the multiprocess application.

1)Changes in the library introduces continuity in array rte_eth_dev_data[]
shared between all processes. Secondary process adds new entries in free
space instead of overwriting existing entries.

2)Changes in application testpmd allow secondary process to attach the
mempool created by primary process rather than create new and in the case of
quit or force quit to free devices data from shared array rte_eth_dev_data[].

-------------------------
How to reproduce the bug:

1) Run primary process:
-c 0xf -n 4 --socket-mem='512,0' -w 03:00.1 -w 03:00.0 --proc-type=primary 
--file-prefix=xz1 -- -i

(gdb) print rte_eth_devices[0].data.name
$52 = "3:0.1"
(gdb) print rte_eth_devices[1].data.name
$53 = "3:0.0"

2) Run secondary process:
-c 0xf0 --socket-mem='512,0' -n 4 -v -b 03:00.1 -b 03:00.0 -b 01:00.0 -b 01:00.0
--vdev 'eth_pcap0,rx_pcap=/var/log/device1.pcap,tx_pcap=/var/log/device2.pcap'
--proc-type=secondary --file-prefix=xz1 -- -i

(gdb) print rte_eth_devices[0].data.name
$52 = "eth_pcap0"
(gdb) print rte_eth_devices[1].data.name
$53 = "eth_pcap1"

3) Go back to the primary and re-check:
(gdb) print rte_eth_devices[0].data.name
$54 = "eth_pcap0"
(gdb) print rte_eth_devices[1].data.name
$55 = "eth_pcap1"

It means that secondary process overwrite data of primary process.

This patch fix it and now if we go back to the primary and re-check then 
everything is fine:
(gdb) print rte_eth_devices[0].data.name
$56 = "3:0.1"
(gdb) print rte_eth_devices[1].data.name
$57 = "3:0.0"

So after this fix structure rte_eth_dev_data[] will keep all data one 
after the other instead of overwriting:

(gdb) print rte_eth_dev_data[0].name
$52 = "3:0.1"
(gdb) print rte_eth_dev_data[1].name
$53 = "3:0.0"
(gdb) print rte_eth_dev_data[2].name
$54 = "eth_pcap0"
(gdb) print rte_eth_dev_data[3].name
$55 = "eth_pcap1"
and so on will be append in the next indexes

If secondary process will be turned off then also will be deleted from array:
(gdb) print rte_eth_dev_data[0].name
$52 = "3:0.1"
(gdb) print rte_eth_dev_data[1].name
$53 = "3:0.0"
(gdb) print rte_eth_dev_data[2].name
$54 = ""
(gdb) print rte_eth_dev_data[3].name
$55 = ""
this also allows re-use index 2 and 3 for next another process
-------------------------

Breaking ABI:
Changes in the library librte_ether causes extending existing structure
rte_eth_dev_data with a new field lock. The reason is that this structure is
sharing between all the processes so it should be protected against attempting
to write from two different processes.

Tomasz Kulasek sent announce ABI change in librte_ether on 21 July 2016.
I would like to join to this breaking ABI, if it is possible.

v2:
* fix syntax error in version script
v3:
* changed scope of function
* improved description
v4:
* fix syntax error in version script

Marcin Kerlin (2):
  librte_ether: add protection against overwrite device data
  app/testpmd: improve handling of multiprocess

 app/test-pmd/testpmd.c                 | 36 +++++++++++++-
 app/test-pmd/testpmd.h                 |  1 +
 lib/librte_ether/rte_ethdev.c          | 90 +++++++++++++++++++++++++++++++---
 lib/librte_ether/rte_ethdev.h          | 24 +++++++++
 lib/librte_ether/rte_ether_version.map |  6 +++
 5 files changed, 147 insertions(+), 10 deletions(-)

-- 
1.9.1

[PATCH v4 0/2] app/testpmd: improve multiprocess support

[Marcin Kerlin]

This patch ensure not overwrite device data in the multiprocess application.

1)Changes in the library introduces continuity in array rte_eth_dev_data[]
shared between all processes. Secondary process adds new entries in free
space instead of overwriting existing entries.

2)Changes in application testpmd allow secondary process to attach the
mempool created by primary process rather than create new and in the case of
quit or force quit to free devices data from shared array rte_eth_dev_data[].

-------------------------
How to reproduce the bug:

1) Run primary process:
-c 0xf -n 4 --socket-mem='512,0' -w 03:00.1 -w 03:00.0 --proc-type=primary 
--file-prefix=xz1 -- -i

(gdb) print rte_eth_devices[0].data.name
$52 = "3:0.1"
(gdb) print rte_eth_devices[1].data.name
$53 = "3:0.0"

2) Run secondary process:
-c 0xf0 --socket-mem='512,0' -n 4 -v -b 03:00.1 -b 03:00.0 -b 01:00.0 -b 01:00.0
--vdev 'eth_pcap0,rx_pcap=/var/log/device1.pcap,tx_pcap=/var/log/device2.pcap'
--proc-type=secondary --file-prefix=xz1 -- -i

(gdb) print rte_eth_devices[0].data.name
$52 = "eth_pcap0"
(gdb) print rte_eth_devices[1].data.name
$53 = "eth_pcap1"

3) Go back to the primary and re-check:
(gdb) print rte_eth_devices[0].data.name
$54 = "eth_pcap0"
(gdb) print rte_eth_devices[1].data.name
$55 = "eth_pcap1"

It means that secondary process overwrite data of primary process.

This patch fix it and now if we go back to the primary and re-check then 
everything is fine:
(gdb) print rte_eth_devices[0].data.name
$56 = "3:0.1"
(gdb) print rte_eth_devices[1].data.name
$57 = "3:0.0"

So after this fix structure rte_eth_dev_data[] will keep all data one 
after the other instead of overwriting:

(gdb) print rte_eth_dev_data[0].name
$52 = "3:0.1"
(gdb) print rte_eth_dev_data[1].name
$53 = "3:0.0"
(gdb) print rte_eth_dev_data[2].name
$54 = "eth_pcap0"
(gdb) print rte_eth_dev_data[3].name
$55 = "eth_pcap1"
and so on will be append in the next indexes

If secondary process will be turned off then also will be deleted from array:
(gdb) print rte_eth_dev_data[0].name
$52 = "3:0.1"
(gdb) print rte_eth_dev_data[1].name
$53 = "3:0.0"
(gdb) print rte_eth_dev_data[2].name
$54 = ""
(gdb) print rte_eth_dev_data[3].name
$55 = ""
this also allows re-use index 2 and 3 for next another process
-------------------------

Breaking ABI:
Changes in the library librte_ether causes extending existing structure
rte_eth_dev_data with a new field lock. The reason is that this structure is
sharing between all the processes so it should be protected against attempting
to write from two different processes.

Tomasz Kulasek sent announce ABI change in librte_ether on 21 July 2016.
I would like to join to this breaking ABI, if it is possible.

v2:
* fix syntax error in version script
v3:
* changed scope of function
* improved description
v4:
* fix syntax error in version script

Marcin Kerlin (2):
  librte_ether: add protection against overwrite device data
  app/testpmd: improve handling of multiprocess

 app/test-pmd/testpmd.c                 | 36 +++++++++++++-
 app/test-pmd/testpmd.h                 |  1 +
 lib/librte_ether/rte_ethdev.c          | 90 +++++++++++++++++++++++++++++++---
 lib/librte_ether/rte_ethdev.h          | 24 +++++++++
 lib/librte_ether/rte_ether_version.map |  6 +++
 5 files changed, 147 insertions(+), 10 deletions(-)

-- 
1.9.1

[PATCH v4 1/2] librte_ether: add protection against overwrite device data

[Marcin Kerlin]

Added prevention not overwrite device data in array rte_eth_dev_data[]
for the next secondary applications. Secondary process appends in the
first free place rather than at the beginning. This behavior prevents
overwriting devices data of primary process by secondary process.

Signed-off-by: Marcin Kerlin <marcinx.kerlin@intel.com>
---
 lib/librte_ether/rte_ethdev.c          | 90 +++++++++++++++++++++++++++++++---
 lib/librte_ether/rte_ethdev.h          | 24 +++++++++
 lib/librte_ether/rte_ether_version.map |  6 +++
 3 files changed, 112 insertions(+), 8 deletions(-)

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index 382c959..9060df4 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -176,6 +176,19 @@ rte_eth_dev_allocated(const char *name)
 	return NULL;
 }
 
+static struct rte_eth_dev_data *
+rte_eth_dev_get_dev_data_by_name(const char *name)
+{
+	unsigned int i;
+
+	for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
+		if (strcmp(rte_eth_dev_data[i].name, name) == 0)
+			return &rte_eth_dev_data[i];
+	}
+
+	return NULL;
+}
+
 static uint8_t
 rte_eth_dev_find_free_port(void)
 {
@@ -188,10 +201,43 @@ rte_eth_dev_find_free_port(void)
 	return RTE_MAX_ETHPORTS;
 }
 
+static uint8_t
+rte_eth_dev_find_free_dev_data_id(void)
+{
+	unsigned int i;
+
+	for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
+		if (!strlen(rte_eth_dev_data[i].name))
+			return i;
+	}
+	return RTE_MAX_ETHPORTS;
+}
+
+int
+rte_eth_dev_release_dev_data(uint8_t port_id)
+{
+	char device[RTE_ETH_NAME_MAX_LEN];
+	struct rte_eth_dev_data *eth_dev_data = NULL;
+
+	/* get device name by port id */
+	if (rte_eth_dev_get_name_by_port(port_id, device))
+		return -EINVAL;
+
+	/* look for an entry in the device data */
+	eth_dev_data = rte_eth_dev_get_dev_data_by_name(device);
+	if (eth_dev_data == NULL)
+		return -EINVAL;
+
+	/* clear an entry in the device data */
+	memset(eth_dev_data, 0, sizeof(struct rte_eth_dev_data));
+
+	return 0;
+}
+
 struct rte_eth_dev *
 rte_eth_dev_allocate(const char *name, enum rte_eth_dev_type type)
 {
-	uint8_t port_id;
+	uint8_t port_id, dev_data_id;
 	struct rte_eth_dev *eth_dev;
 
 	port_id = rte_eth_dev_find_free_port();
@@ -203,17 +249,35 @@ rte_eth_dev_allocate(const char *name, enum rte_eth_dev_type type)
 	if (rte_eth_dev_data == NULL)
 		rte_eth_dev_data_alloc();
 
+	do {
+		dev_data_id = rte_eth_dev_find_free_dev_data_id();
+	} while (!rte_spinlock_trylock(&rte_eth_dev_data[dev_data_id].lock)
+			&& dev_data_id < RTE_MAX_ETHPORTS);
+
+	if (dev_data_id == RTE_MAX_ETHPORTS) {
+		RTE_PMD_DEBUG_TRACE("Reached maximum number of Ethernet ports by all "
+				"the processes\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;
 	}
 
+	if (rte_eth_dev_get_dev_data_by_name(name) != NULL) {
+		RTE_PMD_DEBUG_TRACE("Ethernet Device with name %s already allocated by "
+				"another process!\n", name);
+		return NULL;
+	}
+
 	eth_dev = &rte_eth_devices[port_id];
-	eth_dev->data = &rte_eth_dev_data[port_id];
+	eth_dev->data = &rte_eth_dev_data[dev_data_id];
 	snprintf(eth_dev->data->name, sizeof(eth_dev->data->name), "%s", name);
 	eth_dev->data->port_id = port_id;
 	eth_dev->attached = DEV_ATTACHED;
+	rte_spinlock_unlock(&eth_dev->data->lock);
 	eth_dev->dev_type = type;
 	nb_ports++;
 	return eth_dev;
@@ -417,9 +481,7 @@ rte_eth_dev_get_name_by_port(uint8_t port_id, char *name)
 		return -EINVAL;
 	}
 
-	/* shouldn't check 'rte_eth_devices[i].data',
-	 * because it might be overwritten by VDEV PMD */
-	tmp = rte_eth_dev_data[port_id].name;
+	tmp = rte_eth_devices[port_id].data->name;
 	strcpy(name, tmp);
 	return 0;
 }
@@ -438,9 +500,7 @@ rte_eth_dev_get_port_by_name(const char *name, uint8_t *port_id)
 
 	for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
 
-		if (!strncmp(name,
-			rte_eth_dev_data[i].name, strlen(name))) {
-
+		if (!strncmp(name, rte_eth_devices[i].data->name, strlen(name))) {
 			*port_id = i;
 
 			return 0;
@@ -631,6 +691,8 @@ int
 rte_eth_dev_detach(uint8_t port_id, char *name)
 {
 	struct rte_pci_addr addr;
+	struct rte_eth_dev_data *eth_dev_data = NULL;
+	char device[RTE_ETH_NAME_MAX_LEN];
 	int ret = -1;
 
 	if (name == NULL) {
@@ -642,6 +704,15 @@ rte_eth_dev_detach(uint8_t port_id, char *name)
 	if (rte_eth_dev_is_detachable(port_id))
 		goto err;
 
+	/* get device name by port id */
+	if (rte_eth_dev_get_name_by_port(port_id, device))
+		goto err;
+
+	/* look for an entry in the shared device data */
+	eth_dev_data = rte_eth_dev_get_dev_data_by_name(device);
+	if (eth_dev_data == NULL)
+		goto err;
+
 	if (rte_eth_dev_get_device_type(port_id) == RTE_ETH_DEV_PCI) {
 		ret = rte_eth_dev_get_addr_by_port(port_id, &addr);
 		if (ret < 0)
@@ -661,6 +732,9 @@ rte_eth_dev_detach(uint8_t port_id, char *name)
 			goto err;
 	}
 
+	/* clear an entry in the shared device data */
+	memset(eth_dev_data, 0, sizeof(struct rte_eth_dev_data));
+
 	return 0;
 
 err:
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index 96575e8..8d7674e 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -1708,6 +1708,7 @@ struct rte_eth_dev_data {
 	enum rte_kernel_driver kdrv;    /**< Kernel driver passthrough */
 	int numa_node;  /**< NUMA node connection */
 	const char *drv_name;   /**< Driver name */
+	rte_spinlock_t lock; /** Lock on allocate eth device */
 };
 
 /** Device supports hotplug detach */
@@ -1752,6 +1753,29 @@ struct rte_eth_dev *rte_eth_dev_allocated(const char *name);
 
 /**
  * @internal
+ * Returns a shared device data slot specified by the unique identifier name.
+ *
+ * @param	name
+ *  The pointer to the Unique identifier name for each shared Ethernet device
+ *  between multiple processes.
+ * @return
+ *   - The pointer to the shared ethdev slot, on success. NULL on error
+ */
+struct rte_eth_dev_data *rte_eth_dev_data_allocated(const char *name);
+
+/**
+ * @internal
+ * Release device data kept in shared memory for all processes.
+ *
+ * @param	port_id
+ *   The port identifier of the device to release device data.
+ * @return
+ *   - 0 on success, negative on error
+ */
+int rte_eth_dev_release_dev_data(uint8_t port_id);
+
+/**
+ * @internal
  * Allocates a new ethdev slot for an ethernet device and returns the pointer
  * to that slot for the driver to use.
  *
diff --git a/lib/librte_ether/rte_ether_version.map b/lib/librte_ether/rte_ether_version.map
index 45ddf44..c98ecd4 100644
--- a/lib/librte_ether/rte_ether_version.map
+++ b/lib/librte_ether/rte_ether_version.map
@@ -139,3 +139,9 @@ DPDK_16.07 {
 	rte_eth_dev_get_port_by_name;
 	rte_eth_xstats_get_names;
 } DPDK_16.04;
+
+DPDK_16.11 {
+	global:
+
+	rte_eth_dev_release_dev_data;
+} DPDK_16.07;
-- 
1.9.1

Re: [PATCH v4 1/2] librte_ether: add protection against overwrite device data

[Pattan, Reshma]

Hi,


> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Marcin Kerlin
> Sent: Tuesday, September 27, 2016 12:13 PM
> To: dev@dpdk.org
> Cc: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>;
> thomas.monjalon@6wind.com; Kerlin, MarcinX <marcinx.kerlin@intel.com>
>  /**
>   * @internal
> + * Returns a shared device data slot specified by the unique identifier name.
> + *
> + * @param	name
> + *  The pointer to the Unique identifier name for each shared Ethernet
> +device
> + *  between multiple processes.
> + * @return
> + *   - The pointer to the shared ethdev slot, on success. NULL on error
> + */
> +struct rte_eth_dev_data *rte_eth_dev_data_allocated(const char *name);
> +

1) You forgot to remove this function declaration.

2)Check patch reported lines over 80 characters. Please run check patch on next versions of patch before sending.

WARNING:LONG_LINE_STRING: line over 80 characters
#106: FILE: lib/librte_ether/rte_ethdev.c:258:
+               RTE_PMD_DEBUG_TRACE("Reached maximum number of Ethernet ports by all "

WARNING:LONG_LINE_STRING: line over 80 characters
#118: FILE: lib/librte_ether/rte_ethdev.c:270:
+               RTE_PMD_DEBUG_TRACE("Ethernet Device with name %s already allocated by "

WARNING:LONG_LINE: line over 80 characters
#151: FILE: lib/librte_ether/rte_ethdev.c:503:
+               if (!strncmp(name, rte_eth_devices[i].data->name, strlen(name))) {

3)Can you also make sure to rebase  when you send next patch version, as I see some issue while applying the patch 
Applying: librte_ether: add protection against overwrite device data
error: lib/librte_ether/rte_ether_version.map: does not match index
Patch failed at 0001 librte_ether: add protection against overwrite device data

Thanks,
Reshma

Re: [PATCH v4 1/2] librte_ether: add protection against overwrite device data

[Pattan, Reshma]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Marcin Kerlin
> Sent: Tuesday, September 27, 2016 12:13 PM
> To: dev@dpdk.org
> Cc: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>;
> thomas.monjalon@6wind.com; Kerlin, MarcinX <marcinx.kerlin@intel.com>
> Subject: [dpdk-dev] [PATCH v4 1/2] librte_ether: add protection against
> overwrite device data
> 
> +int
> +rte_eth_dev_release_dev_data(uint8_t port_id) {
> +	char device[RTE_ETH_NAME_MAX_LEN];
> +	struct rte_eth_dev_data *eth_dev_data = NULL;
> +
> +
> @@ -631,6 +691,8 @@ int
>  rte_eth_dev_detach(uint8_t port_id, char *name)  {
>  	struct rte_pci_addr addr;
> +	struct rte_eth_dev_data *eth_dev_data = NULL;
> +	char device[RTE_ETH_NAME_MAX_LEN];
>  	int ret = -1;
> 
>  	if (name == NULL) {
> @@ -642,6 +704,15 @@ rte_eth_dev_detach(uint8_t port_id, char *name)
>  	if (rte_eth_dev_is_detachable(port_id))
>  		goto err;
> 
> +	/* get device name by port id */
> +	if (rte_eth_dev_get_name_by_port(port_id, device))
> +		goto err;
> +
> +	/* look for an entry in the shared device data */
> +	eth_dev_data = rte_eth_dev_get_dev_data_by_name(device);
> +	if (eth_dev_data == NULL)
> +		goto err;
> +
>  	if (rte_eth_dev_get_device_type(port_id) == RTE_ETH_DEV_PCI) {
>  		ret = rte_eth_dev_get_addr_by_port(port_id, &addr);
>  		if (ret < 0)
> @@ -661,6 +732,9 @@ rte_eth_dev_detach(uint8_t port_id, char *name)
>  			goto err;
>  	}
> 
> +	/* clear an entry in the shared device data */
> +	memset(eth_dev_data, 0, sizeof(struct rte_eth_dev_data));
> +
>  	return 0;
> 

In this function, the new code chunks  together is nothing but the function " rte_eth_dev_release_dev_data()".
So u can call the function itself rather than a duplicate code.  

Thanks,
Reshma

Re: [PATCH v4 1/2] librte_ether: add protection against overwrite device data

[Kerlin, MarcinX]

Hi Reshma,

> -----Original Message-----
> From: Pattan, Reshma
> Sent: Wednesday, September 28, 2016 4:04 PM
> To: Kerlin, MarcinX <marcinx.kerlin@intel.com>; dev@dpdk.org
> Cc: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>;
> thomas.monjalon@6wind.com; Kerlin, MarcinX <marcinx.kerlin@intel.com>
> Subject: RE: [dpdk-dev] [PATCH v4 1/2] librte_ether: add protection against
> overwrite device data
> 
> 
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Marcin Kerlin
> > Sent: Tuesday, September 27, 2016 12:13 PM
> > To: dev@dpdk.org
> > Cc: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>;
> > thomas.monjalon@6wind.com; Kerlin, MarcinX <marcinx.kerlin@intel.com>
> > Subject: [dpdk-dev] [PATCH v4 1/2] librte_ether: add protection
> > against overwrite device data
> >
> > +int
> > +rte_eth_dev_release_dev_data(uint8_t port_id) {
> > +	char device[RTE_ETH_NAME_MAX_LEN];
> > +	struct rte_eth_dev_data *eth_dev_data = NULL;
> > +
> > +
> > @@ -631,6 +691,8 @@ int
> >  rte_eth_dev_detach(uint8_t port_id, char *name)  {
> >  	struct rte_pci_addr addr;
> > +	struct rte_eth_dev_data *eth_dev_data = NULL;
> > +	char device[RTE_ETH_NAME_MAX_LEN];
> >  	int ret = -1;
> >
> >  	if (name == NULL) {
> > @@ -642,6 +704,15 @@ rte_eth_dev_detach(uint8_t port_id, char *name)
> >  	if (rte_eth_dev_is_detachable(port_id))
> >  		goto err;
> >
> > +	/* get device name by port id */
> > +	if (rte_eth_dev_get_name_by_port(port_id, device))
> > +		goto err;
> > +
> > +	/* look for an entry in the shared device data */
> > +	eth_dev_data = rte_eth_dev_get_dev_data_by_name(device);
> > +	if (eth_dev_data == NULL)
> > +		goto err;
> > +
> >  	if (rte_eth_dev_get_device_type(port_id) == RTE_ETH_DEV_PCI) {
> >  		ret = rte_eth_dev_get_addr_by_port(port_id, &addr);
> >  		if (ret < 0)
> > @@ -661,6 +732,9 @@ rte_eth_dev_detach(uint8_t port_id, char *name)
> >  			goto err;
> >  	}
> >
> > +	/* clear an entry in the shared device data */
> > +	memset(eth_dev_data, 0, sizeof(struct rte_eth_dev_data));
> > +
> >  	return 0;
> >
> 
> In this function, the new code chunks  together is nothing but the function "
> rte_eth_dev_release_dev_data()".
> So u can call the function itself rather than a duplicate code.

It was intentional, reason:

If I call function in place:
(1) beginning: then I lose device name for function below rte_eth_dev_detach_vdev (1.1):
	a) this is important for drivers  that hold name in shared rte_eth_dev_data[]
	b) not important for drivers that prepare own rte_eth_dev_data e.g pcap (rte_eth_pcap.c, line 816)

(2) end: then I lose device name for my function rte_eth_dev_release_dev_data, because in the above function 
rte_eth_dev_detach_vdev (1.1) for e.g pcap is call rte_free(eth_dev->data) which removes me a pointer to the
name (rte_eth_pcap.c, line 1079).


rte_eth_dev_detach (uint8_t port_id, char *name){
...
	(1) rte_eth_dev_release_dev_data(port_id);

	if (rte_eth_dev_get_device_type(port_id) == RTE_ETH_DEV_PCI) {
		ret = rte_eth_dev_get_addr_by_port(port_id, &addr);
		if (ret < 0)
			goto err;

		ret = rte_eth_dev_detach_pdev(port_id, &addr);
		if (ret < 0)
			goto err;

		snprintf(name, RTE_ETH_NAME_MAX_LEN,
			"%04x:%02x:%02x.%d",
			addr.domain, addr.bus,
			addr.devid, addr.function);
	} else {
		(1.1) ret = rte_eth_dev_detach_vdev(port_id, name);
		if (ret < 0)
			goto err;
	}

	(2) rte_eth_dev_release_dev_data(port_id);
...
}

This is reason why I keep name at the beginning but I release the name at the end function after detach.
At this point I do not see how the code directly replace by one function call.

Regards,
Marcin
> 
> Thanks,
> Reshma

[PATCH v5 0/2] app/testpmd: improve multiprocess support

[Marcin Kerlin]

This patch ensure not overwrite device data in the multiprocess application.

1)Changes in the library introduces continuity in array rte_eth_dev_data[]
shared between all processes. Secondary process adds new entries in free
space instead of overwriting existing entries.

2)Changes in application testpmd allow secondary process to attach the
mempool created by primary process rather than create new and in the case of
quit or force quit to free devices data from shared array rte_eth_dev_data[].

-------------------------
How to reproduce the bug:

1) Run primary process:
./testpmd -c 0xf -n 4 --socket-mem='512,0' -w 03:00.1 -w 03:00.0
--proc-type=primary --file-prefix=xz1 -- -i

(gdb) print rte_eth_devices[0].data.name
$52 = "3:0.1"
(gdb) print rte_eth_devices[1].data.name
$53 = "3:0.0"

2) Run secondary process:
./testpmd -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

(gdb) print rte_eth_devices[0].data.name
$52 = "eth_pcap0"
(gdb) print rte_eth_devices[1].data.name
$53 = "eth_pcap1"

3) Go back to the primary and re-check:
(gdb) print rte_eth_devices[0].data.name
$54 = "eth_pcap0"
(gdb) print rte_eth_devices[1].data.name
$55 = "eth_pcap1"

It means that secondary process overwrite data of primary process.

This patch fix it and now if we go back to the primary and re-check then 
everything is fine:
(gdb) print rte_eth_devices[0].data.name
$56 = "3:0.1"
(gdb) print rte_eth_devices[1].data.name
$57 = "3:0.0"

So after this fix structure rte_eth_dev_data[] will keep all data one after
the other instead of overwriting:
(gdb) print rte_eth_dev_data[0].name
$52 = "3:0.1"
(gdb) print rte_eth_dev_data[1].name
$53 = "3:0.0"
(gdb) print rte_eth_dev_data[2].name
$54 = "eth_pcap0"
(gdb) print rte_eth_dev_data[3].name
$55 = "eth_pcap1"
and so on will be append in the next indexes

If secondary process will be turned off then also will be deleted from array:
(gdb) print rte_eth_dev_data[0].name
$52 = "3:0.1"
(gdb) print rte_eth_dev_data[1].name
$53 = "3:0.0"
(gdb) print rte_eth_dev_data[2].name
$54 = ""
(gdb) print rte_eth_dev_data[3].name
$55 = ""
this also allows re-use index 2 and 3 for next another process
-------------------------

Breaking ABI:
Changes in the library librte_ether causes extending existing structure 
rte_eth_dev_data with a new field lock. The reason is that this structure
is sharing between all the processes so it should be protected against
attempting to write from two different processes.

Tomasz Kulasek sent announce ABI change in librte_ether on 21 July 2016.
I would like to join to this breaking ABI, if it is possible.

v2:
* fix syntax error in version script
v3:
* changed scope of function
* improved description
v4:
* fix syntax error in version script
v5:
* fix header file

Marcin Kerlin (2):
  librte_ether: add protection against overwrite device data
  app/testpmd: improve handling of multiprocess

 app/test-pmd/testpmd.c                 | 37 +++++++++++++-
 app/test-pmd/testpmd.h                 |  1 +
 lib/librte_ether/rte_ethdev.c          | 90 +++++++++++++++++++++++++++++++---
 lib/librte_ether/rte_ethdev.h          | 12 +++++
 lib/librte_ether/rte_ether_version.map |  6 +++
 5 files changed, 136 insertions(+), 10 deletions(-)

-- 
1.9.1

[PATCH v5 1/2] librte_ether: add protection against overwrite device data

[Marcin Kerlin]

Added protection against overwrite device data in array rte_eth_dev_data[]
for the next secondary applications. Secondary process appends in the
first free place rather than at the beginning. This behavior prevents
overwriting devices data of primary process by secondary process.

Signed-off-by: Marcin Kerlin <marcinx.kerlin@intel.com>
---
 lib/librte_ether/rte_ethdev.c          | 89 +++++++++++++++++++++++++++++++---
 lib/librte_ether/rte_ethdev.h          | 12 +++++
 lib/librte_ether/rte_ether_version.map |  6 +++
 3 files changed, 100 insertions(+), 7 deletions(-)

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index 382c959..89a382c 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -176,6 +176,19 @@ rte_eth_dev_allocated(const char *name)
 	return NULL;
 }
 
+static struct rte_eth_dev_data *
+rte_eth_dev_get_dev_data_by_name(const char *name)
+{
+	unsigned int i;
+
+	for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
+		if (strcmp(rte_eth_dev_data[i].name, name) == 0)
+			return &rte_eth_dev_data[i];
+	}
+
+	return NULL;
+}
+
 static uint8_t
 rte_eth_dev_find_free_port(void)
 {
@@ -188,10 +201,43 @@ rte_eth_dev_find_free_port(void)
 	return RTE_MAX_ETHPORTS;
 }
 
+static uint8_t
+rte_eth_dev_find_free_dev_data_id(void)
+{
+	unsigned int i;
+
+	for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
+		if (!strlen(rte_eth_dev_data[i].name))
+			return i;
+	}
+	return RTE_MAX_ETHPORTS;
+}
+
+int
+rte_eth_dev_release_dev_data(uint8_t port_id)
+{
+	char device[RTE_ETH_NAME_MAX_LEN];
+	struct rte_eth_dev_data *eth_dev_data = NULL;
+
+	/* get device name by port id */
+	if (rte_eth_dev_get_name_by_port(port_id, device))
+		return -EINVAL;
+
+	/* look for an entry in the device data */
+	eth_dev_data = rte_eth_dev_get_dev_data_by_name(device);
+	if (eth_dev_data == NULL)
+		return -EINVAL;
+
+	/* clear an entry in the device data */
+	memset(eth_dev_data, 0, sizeof(struct rte_eth_dev_data));
+
+	return 0;
+}
+
 struct rte_eth_dev *
 rte_eth_dev_allocate(const char *name, enum rte_eth_dev_type type)
 {
-	uint8_t port_id;
+	uint8_t port_id, dev_data_id;
 	struct rte_eth_dev *eth_dev;
 
 	port_id = rte_eth_dev_find_free_port();
@@ -203,17 +249,35 @@ rte_eth_dev_allocate(const char *name, enum rte_eth_dev_type type)
 	if (rte_eth_dev_data == NULL)
 		rte_eth_dev_data_alloc();
 
+	do {
+		dev_data_id = rte_eth_dev_find_free_dev_data_id();
+	} while (!rte_spinlock_trylock(&rte_eth_dev_data[dev_data_id].lock)
+			&& dev_data_id < RTE_MAX_ETHPORTS);
+
+	if (dev_data_id == RTE_MAX_ETHPORTS) {
+		RTE_PMD_DEBUG_TRACE("Reached maximum number of Ethernet ports "
+				"by all the processes\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;
 	}
 
+	if (rte_eth_dev_get_dev_data_by_name(name) != NULL) {
+		RTE_PMD_DEBUG_TRACE("Ethernet Device with name %s already "
+				"allocated by another process!\n", name);
+		return NULL;
+	}
+
 	eth_dev = &rte_eth_devices[port_id];
-	eth_dev->data = &rte_eth_dev_data[port_id];
+	eth_dev->data = &rte_eth_dev_data[dev_data_id];
 	snprintf(eth_dev->data->name, sizeof(eth_dev->data->name), "%s", name);
 	eth_dev->data->port_id = port_id;
 	eth_dev->attached = DEV_ATTACHED;
+	rte_spinlock_unlock(&eth_dev->data->lock);
 	eth_dev->dev_type = type;
 	nb_ports++;
 	return eth_dev;
@@ -417,9 +481,7 @@ rte_eth_dev_get_name_by_port(uint8_t port_id, char *name)
 		return -EINVAL;
 	}
 
-	/* shouldn't check 'rte_eth_devices[i].data',
-	 * because it might be overwritten by VDEV PMD */
-	tmp = rte_eth_dev_data[port_id].name;
+	tmp = rte_eth_devices[port_id].data->name;
 	strcpy(name, tmp);
 	return 0;
 }
@@ -439,8 +501,7 @@ rte_eth_dev_get_port_by_name(const char *name, uint8_t *port_id)
 	for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
 
 		if (!strncmp(name,
-			rte_eth_dev_data[i].name, strlen(name))) {
-
+			rte_eth_devices[i].data->name, strlen(name))) {
 			*port_id = i;
 
 			return 0;
@@ -631,6 +692,8 @@ int
 rte_eth_dev_detach(uint8_t port_id, char *name)
 {
 	struct rte_pci_addr addr;
+	struct rte_eth_dev_data *eth_dev_data = NULL;
+	char device[RTE_ETH_NAME_MAX_LEN];
 	int ret = -1;
 
 	if (name == NULL) {
@@ -642,6 +705,15 @@ rte_eth_dev_detach(uint8_t port_id, char *name)
 	if (rte_eth_dev_is_detachable(port_id))
 		goto err;
 
+	/* get device name by port id */
+	if (rte_eth_dev_get_name_by_port(port_id, device))
+		goto err;
+
+	/* look for an entry in the shared device data */
+	eth_dev_data = rte_eth_dev_get_dev_data_by_name(device);
+	if (eth_dev_data == NULL)
+		goto err;
+
 	if (rte_eth_dev_get_device_type(port_id) == RTE_ETH_DEV_PCI) {
 		ret = rte_eth_dev_get_addr_by_port(port_id, &addr);
 		if (ret < 0)
@@ -661,6 +733,9 @@ rte_eth_dev_detach(uint8_t port_id, char *name)
 			goto err;
 	}
 
+	/* clear an entry in the shared device data */
+	memset(eth_dev_data, 0, sizeof(struct rte_eth_dev_data));
+
 	return 0;
 
 err:
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index 96575e8..ae22469 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -1708,6 +1708,7 @@ struct rte_eth_dev_data {
 	enum rte_kernel_driver kdrv;    /**< Kernel driver passthrough */
 	int numa_node;  /**< NUMA node connection */
 	const char *drv_name;   /**< Driver name */
+	rte_spinlock_t lock; /** Lock on allocate eth device */
 };
 
 /** Device supports hotplug detach */
@@ -1752,6 +1753,17 @@ struct rte_eth_dev *rte_eth_dev_allocated(const char *name);
 
 /**
  * @internal
+ * Release device data kept in shared memory for all processes.
+ *
+ * @param	port_id
+ *   The port identifier of the device to release device data.
+ * @return
+ *   - 0 on success, negative on error
+ */
+int rte_eth_dev_release_dev_data(uint8_t port_id);
+
+/**
+ * @internal
  * Allocates a new ethdev slot for an ethernet device and returns the pointer
  * to that slot for the driver to use.
  *
diff --git a/lib/librte_ether/rte_ether_version.map b/lib/librte_ether/rte_ether_version.map
index 45ddf44..c98ecd4 100644
--- a/lib/librte_ether/rte_ether_version.map
+++ b/lib/librte_ether/rte_ether_version.map
@@ -139,3 +139,9 @@ DPDK_16.07 {
 	rte_eth_dev_get_port_by_name;
 	rte_eth_xstats_get_names;
 } DPDK_16.04;
+
+DPDK_16.11 {
+	global:
+
+	rte_eth_dev_release_dev_data;
+} DPDK_16.07;
-- 
1.9.1

Re: [PATCH v5 1/2] librte_ether: add protection against overwrite device data

[Pattan, Reshma]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Marcin Kerlin
> Sent: Friday, September 30, 2016 3:01 PM
> To: dev@dpdk.org
> Cc: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>;
> thomas.monjalon@6wind.com; Kerlin, MarcinX <marcinx.kerlin@intel.com>
> Subject: [dpdk-dev] [PATCH v5 1/2] librte_ether: add protection against
> overwrite device data
> 
> Added protection against overwrite device data in array rte_eth_dev_data[] for
> the next secondary applications. Secondary process appends in the first free
> place rather than at the beginning. This behavior prevents overwriting devices
> data of primary process by secondary process.
> 
> Signed-off-by: Marcin Kerlin <marcinx.kerlin@intel.com>

Acked-by: Reshma Pattan <reshma.pattan@intel.com>

Re: [PATCH v5 1/2] librte_ether: add protection against overwrite device data

[Thomas Monjalon]

Hi Marcin,

2016-09-30 16:00, Marcin Kerlin:
> Added protection against overwrite device data in array rte_eth_dev_data[]
> for the next secondary applications. Secondary process appends in the
> first free place rather than at the beginning. This behavior prevents
> overwriting devices data of primary process by secondary process.

I've just realized that you are trying to fix an useless code.
Why not just remove the array rte_eth_dev_data[] at first?
We already have the array rte_eth_devices[] and there is a pointer to
rte_eth_dev_data in rte_eth_dev.

Is it just a workaround to be able to lookup the rte_eth_dev_data memzone
in the secondary process?
So wouldn't it be saner to have rte_eth_devices[] in a memzone?

Sergio, as the multi-process maintainer, I guess you have an idea :)

Re: [PATCH v5 1/2] librte_ether: add protection against overwrite device data

[Kerlin, MarcinX]

Hi Thomas,

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Thursday, October 06, 2016 11:41 AM
> To: Kerlin, MarcinX <marcinx.kerlin@intel.com>
> Cc: dev@dpdk.org; De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>;
> Gonzalez Monroy, Sergio <sergio.gonzalez.monroy@intel.com>
> Subject: Re: [PATCH v5 1/2] librte_ether: add protection against overwrite
> device data
> 
> Hi Marcin,
> 
> 2016-09-30 16:00, Marcin Kerlin:
> > Added protection against overwrite device data in array
> > rte_eth_dev_data[] for the next secondary applications. Secondary
> > process appends in the first free place rather than at the beginning.
> > This behavior prevents overwriting devices data of primary process by
> secondary process.
> 
> I've just realized that you are trying to fix an useless code.
> Why not just remove the array rte_eth_dev_data[] at first?

because pointer to rte_eth_dev_data in rte_eth_devices[] is 
just to array rte_eth_dev_data[].

rte_ethdev.c:214 
eth_dev->data = &rte_eth_dev_data[port_id];

> We already have the array rte_eth_devices[] and there is a pointer to
> rte_eth_dev_data in rte_eth_dev.

As you write above there is a pointer, but after run secondary testpmd this pointer
will indicate data which hold from now data for secondary testpmd.

1) run testpmd [primary]

rte_eth_devices[0].data.name = 3:0.1

2) run testpmd [secondary]
This testpmd overwrite first index in rte_eth_dev_data[]
3:0.1 -> eth_pcap0

3) back to testpmd [primary]
rte_eth_devices[0].data.name = eth_pcap0

from now in primary testpmd our device name is eth_pcap0 but should be 3:0.1

> 
> Is it just a workaround to be able to lookup the rte_eth_dev_data memzone in
> the secondary process?

No it is not workaround, it is protection against overwrite device data.
I think that my cover letter good explain what is wrong. I did there
short debug log. 

> So wouldn't it be saner to have rte_eth_devices[] in a memzone?

So you mean that move rte_eth_devices[] to memzone + remove rte_eth_dev_data[].
What will indicate pointer inside rte_eth_dev  rte_eth_devices[]:
(struct rte_eth_dev_data *data;  /**< Pointer to device data */)

If I did not understand you please clarify more.

Regards,
Marcin
> 
> Sergio, as the multi-process maintainer, I guess you have an idea :)

Re: [PATCH v5 1/2] librte_ether: add protection against overwrite device data

[Thomas Monjalon]

2016-10-06 13:57, Kerlin, MarcinX:
> Hi Thomas,
> 
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > 
> > Hi Marcin,
> > 
> > 2016-09-30 16:00, Marcin Kerlin:
> > > Added protection against overwrite device data in array
> > > rte_eth_dev_data[] for the next secondary applications. Secondary
> > > process appends in the first free place rather than at the beginning.
> > > This behavior prevents overwriting devices data of primary process by
> > secondary process.
> > 
> > I've just realized that you are trying to fix an useless code.
> > Why not just remove the array rte_eth_dev_data[] at first?
> 
> because pointer to rte_eth_dev_data in rte_eth_devices[] is 
> just to array rte_eth_dev_data[].
> 
> rte_ethdev.c:214 
> eth_dev->data = &rte_eth_dev_data[port_id];

This line indicates that the pointer data is to the struct rte_eth_dev_data
of the port_id, not the array of every devices.

> > We already have the array rte_eth_devices[] and there is a pointer to
> > rte_eth_dev_data in rte_eth_dev.
> 
> As you write above there is a pointer, but after run secondary testpmd this pointer
> will indicate data which hold from now data for secondary testpmd.
[...]
I think I've understood the bug.
I'm just saying you are fixing a weird design (rte_eth_dev_data[]).

> > Is it just a workaround to be able to lookup the rte_eth_dev_data memzone in
> > the secondary process?
> 
> No it is not workaround, it is protection against overwrite device data.
> I think that my cover letter good explain what is wrong. I did there
> short debug log.

I'm talking about the initial introduction of rte_eth_dev_data[]
which seems to be a workaround for multi-process without touching
rte_eth_devices[] allocated as a global variable (not a memzone).

> > So wouldn't it be saner to have rte_eth_devices[] in a memzone?
> 
> So you mean that move rte_eth_devices[] to memzone + remove rte_eth_dev_data[].

Yes

> What will indicate pointer inside rte_eth_dev  rte_eth_devices[]:
> (struct rte_eth_dev_data *data;  /**< Pointer to device data */)

After thinking more about it, I realize that rte_eth_devices cannot be
in a shared memzone because of its local pointers.

Sorry for the noise, I'll reconsider your patch.

Re: [PATCH v5 1/2] librte_ether: add protection against overwrite device data

[Thomas Monjalon]

2016-09-30 16:00, Marcin Kerlin:
> Added protection against overwrite device data in array rte_eth_dev_data[]
> for the next secondary applications. Secondary process appends in the
> first free place rather than at the beginning. This behavior prevents
> overwriting devices data of primary process by secondary process.

It would be good to state what is a secondary process.
You are trying to extend its capabilities to be able to initialize devices.
So primary and secondary processes are almost equivalent?
What happens if we do not create any device in the primary?
Answer from code review: "Cannot allocate memzone for ethernet port data\n"

The secondary process is a hack to me.
But it is fine to have such hack for debug or monitoring purpose.
I would like to understand what are the other use cases?

By the way, the code managing the shared data of a device should
be at the EAL level in order to be used by other interfaces like crypto.

> @@ -631,6 +692,8 @@ int
>  rte_eth_dev_detach(uint8_t port_id, char *name)
>  {
>  	struct rte_pci_addr addr;
> +	struct rte_eth_dev_data *eth_dev_data = NULL;
> +	char device[RTE_ETH_NAME_MAX_LEN];
>  	int ret = -1;
>  
>  	if (name == NULL) {
> @@ -642,6 +705,15 @@ rte_eth_dev_detach(uint8_t port_id, char *name)
>  	if (rte_eth_dev_is_detachable(port_id))
>  		goto err;
>  
> +	/* get device name by port id */
> +	if (rte_eth_dev_get_name_by_port(port_id, device))
> +		goto err;
> +
> +	/* look for an entry in the shared device data */
> +	eth_dev_data = rte_eth_dev_get_dev_data_by_name(device);
> +	if (eth_dev_data == NULL)
> +		goto err;

Why not getting eth_dev_data from rte_eth_devices[port_id].data ?

> --- a/lib/librte_ether/rte_ethdev.h
> +++ b/lib/librte_ether/rte_ethdev.h
>  /**
>   * @internal
> + * Release device data kept in shared memory for all processes.
> + *
> + * @param	port_id
> + *   The port identifier of the device to release device data.
> + * @return
> + *   - 0 on success, negative on error
> + */
> +int rte_eth_dev_release_dev_data(uint8_t port_id);

Why this function? It is not used.
You already have done the job in the detach function.

Re: [PATCH v5 1/2] librte_ether: add protection against overwrite device data

[Kerlin, MarcinX]

Hi Thomas,

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Thursday, October 06, 2016 4:53 PM
> To: Kerlin, MarcinX <marcinx.kerlin@intel.com>
> Cc: dev@dpdk.org; De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>
> Subject: Re: [PATCH v5 1/2] librte_ether: add protection against overwrite
> device data
> 
> 2016-09-30 16:00, Marcin Kerlin:
> > Added protection against overwrite device data in array
> > rte_eth_dev_data[] for the next secondary applications. Secondary
> > process appends in the first free place rather than at the beginning.
> > This behavior prevents overwriting devices data of primary process by
> secondary process.
> 
> It would be good to state what is a secondary process.
> You are trying to extend its capabilities to be able to initialize devices.
> So primary and secondary processes are almost equivalent?
> What happens if we do not create any device in the primary?
> Answer from code review: "Cannot allocate memzone for ethernet port data\n"
> 
> The secondary process is a hack to me.
> But it is fine to have such hack for debug or monitoring purpose.
> I would like to understand what are the other use cases?

It's true, it is fine for debug or monitoring but If DPDK allow run secondary app with 
devices then it should be safe or completely not allowed. 

This bug has been observed while running secondary testpmd with virtual devices.

I will adapt to the decision of maintainers regards to design of secondary process.

> 
> By the way, the code managing the shared data of a device should be at the
> EAL level in order to be used by other interfaces like crypto.
> 
> > @@ -631,6 +692,8 @@ int
> >  rte_eth_dev_detach(uint8_t port_id, char *name)  {
> >  	struct rte_pci_addr addr;
> > +	struct rte_eth_dev_data *eth_dev_data = NULL;
> > +	char device[RTE_ETH_NAME_MAX_LEN];
> >  	int ret = -1;
> >
> >  	if (name == NULL) {
> > @@ -642,6 +705,15 @@ rte_eth_dev_detach(uint8_t port_id, char *name)
> >  	if (rte_eth_dev_is_detachable(port_id))
> >  		goto err;
> >
> > +	/* get device name by port id */
> > +	if (rte_eth_dev_get_name_by_port(port_id, device))
> > +		goto err;
> > +
> > +	/* look for an entry in the shared device data */
> > +	eth_dev_data = rte_eth_dev_get_dev_data_by_name(device);
> > +	if (eth_dev_data == NULL)
> > +		goto err;
> 
> Why not getting eth_dev_data from rte_eth_devices[port_id].data ?

because rte_eth_devices[port_id].data for some drivers (mainly virtual devices)
is pointer to local eth_dev_data (e.g rte_eth_pcap.c:816 and also other drivers).
This causes that local eth_dev_data is clearing rather than shared in memzone. 

Naming is unique so if device was added then there (shared memzone) has to be. 

> 
> > --- a/lib/librte_ether/rte_ethdev.h
> > +++ b/lib/librte_ether/rte_ethdev.h
> >  /**
> >   * @internal
> > + * Release device data kept in shared memory for all processes.
> > + *
> > + * @param	port_id
> > + *   The port identifier of the device to release device data.
> > + * @return
> > + *   - 0 on success, negative on error
> > + */
> > +int rte_eth_dev_release_dev_data(uint8_t port_id);
> 
> Why this function? It is not used.
> You already have done the job in the detach function.

This function is using in testpmd.c:1640, basic wrapper for clean up.

Detach function is working only for detachable devices, release_dev_data() 
no matter just clean up shared array before next run secondary e.g testpmd.

Regards,
Marcin

Re: [PATCH v5 1/2] librte_ether: add protection against overwrite device data

[Thomas Monjalon]

2016-10-07 12:23, Kerlin, MarcinX:
> Hi Thomas,
> 
> > -----Original Message-----
> > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > Sent: Thursday, October 06, 2016 4:53 PM
> > To: Kerlin, MarcinX <marcinx.kerlin@intel.com>
> > Cc: dev@dpdk.org; De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>
> > Subject: Re: [PATCH v5 1/2] librte_ether: add protection against overwrite
> > device data
> > 
> > 2016-09-30 16:00, Marcin Kerlin:
> > > Added protection against overwrite device data in array
> > > rte_eth_dev_data[] for the next secondary applications. Secondary
> > > process appends in the first free place rather than at the beginning.
> > > This behavior prevents overwriting devices data of primary process by
> > secondary process.
> > 
> > It would be good to state what is a secondary process.
> > You are trying to extend its capabilities to be able to initialize devices.
> > So primary and secondary processes are almost equivalent?
> > What happens if we do not create any device in the primary?
> > Answer from code review: "Cannot allocate memzone for ethernet port data\n"
> > 
> > The secondary process is a hack to me.
> > But it is fine to have such hack for debug or monitoring purpose.
> > I would like to understand what are the other use cases?
> 
> It's true, it is fine for debug or monitoring but If DPDK allow run secondary app with 
> devices then it should be safe or completely not allowed. 
> 
> This bug has been observed while running secondary testpmd with virtual devices.
> 
> I will adapt to the decision of maintainers regards to design of secondary process.
> 
> > 
> > By the way, the code managing the shared data of a device should be at the
> > EAL level in order to be used by other interfaces like crypto.
> > 
> > > @@ -631,6 +692,8 @@ int
> > >  rte_eth_dev_detach(uint8_t port_id, char *name)  {
> > >  	struct rte_pci_addr addr;
> > > +	struct rte_eth_dev_data *eth_dev_data = NULL;
> > > +	char device[RTE_ETH_NAME_MAX_LEN];
> > >  	int ret = -1;
> > >
> > >  	if (name == NULL) {
> > > @@ -642,6 +705,15 @@ rte_eth_dev_detach(uint8_t port_id, char *name)
> > >  	if (rte_eth_dev_is_detachable(port_id))
> > >  		goto err;
> > >
> > > +	/* get device name by port id */
> > > +	if (rte_eth_dev_get_name_by_port(port_id, device))
> > > +		goto err;
> > > +
> > > +	/* look for an entry in the shared device data */
> > > +	eth_dev_data = rte_eth_dev_get_dev_data_by_name(device);
> > > +	if (eth_dev_data == NULL)
> > > +		goto err;
> > 
> > Why not getting eth_dev_data from rte_eth_devices[port_id].data ?
> 
> because rte_eth_devices[port_id].data for some drivers (mainly virtual devices)
> is pointer to local eth_dev_data (e.g rte_eth_pcap.c:816 and also other drivers).
> This causes that local eth_dev_data is clearing rather than shared in memzone. 
> 
> Naming is unique so if device was added then there (shared memzone) has to be. 

Not sure to understand. Isn't it a bug to have local eth_dev_data?
It means these devices are not shared with secondary process?

> > > --- a/lib/librte_ether/rte_ethdev.h
> > > +++ b/lib/librte_ether/rte_ethdev.h
> > >  /**
> > >   * @internal
> > > + * Release device data kept in shared memory for all processes.
> > > + *
> > > + * @param	port_id
> > > + *   The port identifier of the device to release device data.
> > > + * @return
> > > + *   - 0 on success, negative on error
> > > + */
> > > +int rte_eth_dev_release_dev_data(uint8_t port_id);
> > 
> > Why this function? It is not used.
> > You already have done the job in the detach function.
> 
> This function is using in testpmd.c:1640, basic wrapper for clean up.

Please explain the need for cleaning on testpmd exit.
Is it cleaning every devices even those used by the primary process?
I feel it is very weak to not clearly define which process owns a device.

> Detach function is working only for detachable devices, release_dev_data() 
> no matter just clean up shared array before next run secondary e.g testpmd.

Yes freeing device resources is for detachable devices.

[PATCH v5 2/2] app/testpmd: improve handling of multiprocess

[Marcin Kerlin]

Added lookup for pool name because secondary process should attach to
mempool created by primary process rather than create new one.

Added function free_shared_dev_data() used at the exit of the testpmd.
This causes detach devices data from array rte_eth_dev_data[] shared
between all processes. This allows to have a up-to-date list of devices
data and run again secondary application with the same name.

Signed-off-by: Marcin Kerlin <marcinx.kerlin@intel.com>
---
 app/test-pmd/testpmd.c | 37 +++++++++++++++++++++++++++++++++++--
 app/test-pmd/testpmd.h |  1 +
 2 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index e2403c3..78b3a39 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -452,8 +452,11 @@ mbuf_pool_create(uint16_t mbuf_seg_size, unsigned nb_mbuf,
 			rte_mempool_obj_iter(rte_mp, rte_pktmbuf_init, NULL);
 		} else {
 			/* wrapper to rte_mempool_create() */
-			rte_mp = rte_pktmbuf_pool_create(pool_name, nb_mbuf,
-				mb_mempool_cache, 0, mbuf_seg_size, socket_id);
+			rte_mp = rte_mempool_lookup(pool_name);
+			if (rte_mp == NULL)
+				rte_mp = rte_pktmbuf_pool_create(pool_name,
+						nb_mbuf, mb_mempool_cache, 0,
+						mbuf_seg_size, socket_id);
 		}
 	}
 
@@ -1611,6 +1614,35 @@ detach_port(uint8_t port_id)
 	return;
 }
 
+void free_shared_dev_data(portid_t pid)
+{
+	portid_t pi;
+
+	if (port_id_is_invalid(pid, ENABLED_WARN))
+		return;
+
+	/* free data only if the secondary process exits */
+	if (rte_eal_process_type() != RTE_PROC_SECONDARY)
+		return;
+
+	printf("Cleaning device data...\n");
+
+	FOREACH_PORT(pi, ports)
+	{
+		if (pid != pi && pid != (portid_t) RTE_PORT_ALL)
+			continue;
+
+		if (!port_is_closed(pi)) {
+			printf("Port %d is not closed now\n", pi);
+			continue;
+		}
+
+		if (rte_eth_dev_release_dev_data(pi) < 0)
+			return;
+	}
+	printf("Done\n");
+}
+
 void
 pmd_test_exit(void)
 {
@@ -1626,6 +1658,7 @@ pmd_test_exit(void)
 			fflush(stdout);
 			stop_port(pt_id);
 			close_port(pt_id);
+			free_shared_dev_data(pt_id);
 		}
 	}
 	printf("\nBye...\n");
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index 2b281cc..3915a06 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -553,6 +553,7 @@ void attach_port(char *identifier);
 void detach_port(uint8_t port_id);
 int all_ports_stopped(void);
 int port_is_started(portid_t port_id);
+void free_shared_dev_data(portid_t pid);
 void pmd_test_exit(void);
 void fdir_get_infos(portid_t port_id);
 void fdir_set_flex_mask(portid_t port_id,
-- 
1.9.1

Re: [PATCH v5 2/2] app/testpmd: improve handling of multiprocess

[Pattan, Reshma]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Marcin Kerlin
> Sent: Friday, September 30, 2016 3:24 PM
> To: dev@dpdk.org
> Cc: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>;
> thomas.monjalon@6wind.com; Kerlin, MarcinX <marcinx.kerlin@intel.com>
> Subject: [dpdk-dev] [PATCH v5 2/2] app/testpmd: improve handling of
> multiprocess
> 
> Added lookup for pool name because secondary process should attach to
> mempool created by primary process rather than create new one.
> 
> Added function free_shared_dev_data() used at the exit of the testpmd.
> This causes detach devices data from array rte_eth_dev_data[] shared between
> all processes. This allows to have a up-to-date list of devices data and run again
> secondary application with the same name.
> 
> Signed-off-by: Marcin Kerlin <marcinx.kerlin@intel.com>

Acked-by: Reshma Pattan <reshma.pattan@intel.com>

Re: [PATCH v5 0/2] app/testpmd: improve multiprocess support

[Pattan, Reshma]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Marcin Kerlin
> Sent: Friday, September 30, 2016 3:01 PM
> To: dev@dpdk.org
> Cc: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>;
> thomas.monjalon@6wind.com; Kerlin, MarcinX <marcinx.kerlin@intel.com>
> Subject: [dpdk-dev] [PATCH v5 0/2] app/testpmd: improve multiprocess support
> 
> This patch ensure not overwrite device data in the multiprocess application.
> 
> 1)Changes in the library introduces continuity in array rte_eth_dev_data[] shared
> between all processes. Secondary process adds new entries in free space instead
> of overwriting existing entries.
> 
> 2)Changes in application testpmd allow secondary process to attach the
> mempool created by primary process rather than create new and in the case of
> quit or force quit to free devices data from shared array rte_eth_dev_data[].
> 
> -------------------------
> How to reproduce the bug:
> 
> 1) Run primary process:
> ./testpmd -c 0xf -n 4 --socket-mem='512,0' -w 03:00.1 -w 03:00.0 --proc-
> type=primary --file-prefix=xz1 -- -i
> 
> (gdb) print rte_eth_devices[0].data.name
> $52 = "3:0.1"
> (gdb) print rte_eth_devices[1].data.name
> $53 = "3:0.0"
> 
> 2) Run secondary process:
> ./testpmd -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
> 
> (gdb) print rte_eth_devices[0].data.name
> $52 = "eth_pcap0"
> (gdb) print rte_eth_devices[1].data.name
> $53 = "eth_pcap1"
> 
> 3) Go back to the primary and re-check:
> (gdb) print rte_eth_devices[0].data.name
> $54 = "eth_pcap0"
> (gdb) print rte_eth_devices[1].data.name
> $55 = "eth_pcap1"
> 
> It means that secondary process overwrite data of primary process.
> 
> This patch fix it and now if we go back to the primary and re-check then
> everything is fine:
> (gdb) print rte_eth_devices[0].data.name
> $56 = "3:0.1"
> (gdb) print rte_eth_devices[1].data.name
> $57 = "3:0.0"
> 
> So after this fix structure rte_eth_dev_data[] will keep all data one after the
> other instead of overwriting:
> (gdb) print rte_eth_dev_data[0].name
> $52 = "3:0.1"
> (gdb) print rte_eth_dev_data[1].name
> $53 = "3:0.0"
> (gdb) print rte_eth_dev_data[2].name
> $54 = "eth_pcap0"
> (gdb) print rte_eth_dev_data[3].name
> $55 = "eth_pcap1"
> and so on will be append in the next indexes
> 
> If secondary process will be turned off then also will be deleted from array:
> (gdb) print rte_eth_dev_data[0].name
> $52 = "3:0.1"
> (gdb) print rte_eth_dev_data[1].name
> $53 = "3:0.0"
> (gdb) print rte_eth_dev_data[2].name
> $54 = ""
> (gdb) print rte_eth_dev_data[3].name
> $55 = ""
> this also allows re-use index 2 and 3 for next another process
> -------------------------
> 
> Breaking ABI:
> Changes in the library librte_ether causes extending existing structure
> rte_eth_dev_data with a new field lock. The reason is that this structure is
> sharing between all the processes so it should be protected against attempting
> to write from two different processes.
> 
> Tomasz Kulasek sent announce ABI change in librte_ether on 21 July 2016.
> I would like to join to this breaking ABI, if it is possible.
> 
> v2:
> * fix syntax error in version script
> v3:
> * changed scope of function
> * improved description
> v4:
> * fix syntax error in version script
> v5:
> * fix header file
> 
> Marcin Kerlin (2):
>   librte_ether: add protection against overwrite device data
>   app/testpmd: improve handling of multiprocess
> 
>  app/test-pmd/testpmd.c                 | 37 +++++++++++++-
>  app/test-pmd/testpmd.h                 |  1 +
>  lib/librte_ether/rte_ethdev.c          | 90 +++++++++++++++++++++++++++++++---
>  lib/librte_ether/rte_ethdev.h          | 12 +++++
>  lib/librte_ether/rte_ether_version.map |  6 +++
>  5 files changed, 136 insertions(+), 10 deletions(-)
> 
> --
> 1.9.1

Acked-by: Reshma Pattan <reshma.pattan@intel.com>

Re: [PATCH v5 0/2] app/testpmd: improve multiprocess support

[Sergio Gonzalez Monroy]

On 30/09/2016 15:00, Marcin Kerlin wrote:
> This patch ensure not overwrite device data in the multiprocess application.
>
> 1)Changes in the library introduces continuity in array rte_eth_dev_data[]
> shared between all processes. Secondary process adds new entries in free
> space instead of overwriting existing entries.
>
> 2)Changes in application testpmd allow secondary process to attach the
> mempool created by primary process rather than create new and in the case of
> quit or force quit to free devices data from shared array rte_eth_dev_data[].
>
> -------------------------
> How to reproduce the bug:
>
> 1) Run primary process:
> ./testpmd -c 0xf -n 4 --socket-mem='512,0' -w 03:00.1 -w 03:00.0
> --proc-type=primary --file-prefix=xz1 -- -i
>
> (gdb) print rte_eth_devices[0].data.name
> $52 = "3:0.1"
> (gdb) print rte_eth_devices[1].data.name
> $53 = "3:0.0"
>
> 2) Run secondary process:
> ./testpmd -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
>
> (gdb) print rte_eth_devices[0].data.name
> $52 = "eth_pcap0"
> (gdb) print rte_eth_devices[1].data.name
> $53 = "eth_pcap1"
>
> 3) Go back to the primary and re-check:
> (gdb) print rte_eth_devices[0].data.name
> $54 = "eth_pcap0"
> (gdb) print rte_eth_devices[1].data.name
> $55 = "eth_pcap1"
>
> It means that secondary process overwrite data of primary process.
>
> This patch fix it and now if we go back to the primary and re-check then
> everything is fine:
> (gdb) print rte_eth_devices[0].data.name
> $56 = "3:0.1"
> (gdb) print rte_eth_devices[1].data.name
> $57 = "3:0.0"
>
> So after this fix structure rte_eth_dev_data[] will keep all data one after
> the other instead of overwriting:
> (gdb) print rte_eth_dev_data[0].name
> $52 = "3:0.1"
> (gdb) print rte_eth_dev_data[1].name
> $53 = "3:0.0"
> (gdb) print rte_eth_dev_data[2].name
> $54 = "eth_pcap0"
> (gdb) print rte_eth_dev_data[3].name
> $55 = "eth_pcap1"
> and so on will be append in the next indexes
>
> If secondary process will be turned off then also will be deleted from array:
> (gdb) print rte_eth_dev_data[0].name
> $52 = "3:0.1"
> (gdb) print rte_eth_dev_data[1].name
> $53 = "3:0.0"
> (gdb) print rte_eth_dev_data[2].name
> $54 = ""
> (gdb) print rte_eth_dev_data[3].name
> $55 = ""
> this also allows re-use index 2 and 3 for next another process
> -------------------------
>
> Breaking ABI:
> Changes in the library librte_ether causes extending existing structure
> rte_eth_dev_data with a new field lock. The reason is that this structure
> is sharing between all the processes so it should be protected against
> attempting to write from two different processes.
>
> Tomasz Kulasek sent announce ABI change in librte_ether on 21 July 2016.
> I would like to join to this breaking ABI, if it is possible.
>
> v2:
> * fix syntax error in version script
> v3:
> * changed scope of function
> * improved description
> v4:
> * fix syntax error in version script
> v5:
> * fix header file
>
> Marcin Kerlin (2):
>    librte_ether: add protection against overwrite device data
>    app/testpmd: improve handling of multiprocess
>
>   app/test-pmd/testpmd.c                 | 37 +++++++++++++-
>   app/test-pmd/testpmd.h                 |  1 +
>   lib/librte_ether/rte_ethdev.c          | 90 +++++++++++++++++++++++++++++++---
>   lib/librte_ether/rte_ethdev.h          | 12 +++++
>   lib/librte_ether/rte_ether_version.map |  6 +++
>   5 files changed, 136 insertions(+), 10 deletions(-)
>

NACK series for 16.11

The patch would break the use case where primary and secondary share 
same PCI device.

Overall, I think Thomas has already mentioned it, we need further 
discussion on the use cases
and scope of DPDK multi-process.
This could be a good topic for the upcoming DPDK Userspace event.

Sergio

[PATCH v4 2/2] app/testpmd: improve handling of multiprocess

[Marcin Kerlin]

Added lookup for pool name because secondary process should attach to
mempool created by primary process rather than create new one.

Added function free_shared_dev_data() used at the exit of the testpmd.
This causes detach devices data from array rte_eth_dev_data[] shared
between all processes. This allows to have a up-to-date list of devices
data and run again secondary application with the same name.

Signed-off-by: Marcin Kerlin <marcinx.kerlin@intel.com>
---
 app/test-pmd/testpmd.c | 36 ++++++++++++++++++++++++++++++++++--
 app/test-pmd/testpmd.h |  1 +
 2 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index e2403c3..6418cf2 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -452,8 +452,10 @@ mbuf_pool_create(uint16_t mbuf_seg_size, unsigned nb_mbuf,
 			rte_mempool_obj_iter(rte_mp, rte_pktmbuf_init, NULL);
 		} else {
 			/* wrapper to rte_mempool_create() */
-			rte_mp = rte_pktmbuf_pool_create(pool_name, nb_mbuf,
-				mb_mempool_cache, 0, mbuf_seg_size, socket_id);
+			rte_mp = rte_mempool_lookup(pool_name);
+			if (rte_mp == NULL)
+				rte_mp = rte_pktmbuf_pool_create(pool_name, nb_mbuf,
+						mb_mempool_cache, 0, mbuf_seg_size, socket_id);
 		}
 	}
 
@@ -1611,6 +1613,35 @@ detach_port(uint8_t port_id)
 	return;
 }
 
+void free_shared_dev_data(portid_t pid)
+{
+	portid_t pi;
+
+	if (port_id_is_invalid(pid, ENABLED_WARN))
+		return;
+
+	/* free data only if the secondary process exits */
+	if (rte_eal_process_type() != RTE_PROC_SECONDARY)
+		return;
+
+	printf("Cleaning device data...\n");
+
+	FOREACH_PORT(pi, ports)
+	{
+		if (pid != pi && pid != (portid_t) RTE_PORT_ALL)
+			continue;
+
+		if (!port_is_closed(pi)) {
+			printf("Port %d is not closed now\n", pi);
+			continue;
+		}
+
+		if (rte_eth_dev_release_dev_data(pi) < 0)
+			return;
+	}
+	printf("Done\n");
+}
+
 void
 pmd_test_exit(void)
 {
@@ -1626,6 +1657,7 @@ pmd_test_exit(void)
 			fflush(stdout);
 			stop_port(pt_id);
 			close_port(pt_id);
+			free_shared_dev_data(pt_id);
 		}
 	}
 	printf("\nBye...\n");
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index 2b281cc..3915a06 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -553,6 +553,7 @@ void attach_port(char *identifier);
 void detach_port(uint8_t port_id);
 int all_ports_stopped(void);
 int port_is_started(portid_t port_id);
+void free_shared_dev_data(portid_t pid);
 void pmd_test_exit(void);
 void fdir_get_infos(portid_t port_id);
 void fdir_set_flex_mask(portid_t port_id,
-- 
1.9.1

Re: [PATCH v4 2/2] app/testpmd: improve handling of multiprocess

[Pattan, Reshma]

Hi,

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Marcin Kerlin
> Sent: Tuesday, September 27, 2016 12:13 PM
> To: dev@dpdk.org
> Cc: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>;
> thomas.monjalon@6wind.com; Kerlin, MarcinX <marcinx.kerlin@intel.com>
> Subject: [dpdk-dev] [PATCH v4 2/2] app/testpmd: improve handling of
> multiprocess
> 

Check patch issues for lines over 80. Please run check patch for next versions

WARNING:LONG_LINE: line over 80 characters
#40: FILE: app/test-pmd/testpmd.c:457:
+                               rte_mp = rte_pktmbuf_pool_create(pool_name, nb_mbuf,

WARNING:LONG_LINE: line over 80 characters
#41: FILE: app/test-pmd/testpmd.c:458:
+                                               mb_mempool_cache, 0, mbuf_seg_size, socket_id);

total: 0 errors, 2 warnings, 61 lines checked

Thanks,
Reshma

Re: [PATCH v4 2/2] app/testpmd: improve handling of multiprocess

[Kerlin, MarcinX]

Hi

> -----Original Message-----
> From: Pattan, Reshma
> Sent: Wednesday, September 28, 2016 12:58 PM
> To: Kerlin, MarcinX <marcinx.kerlin@intel.com>; dev@dpdk.org
> Cc: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>;
> thomas.monjalon@6wind.com; Kerlin, MarcinX <marcinx.kerlin@intel.com>
> Subject: RE: [dpdk-dev] [PATCH v4 2/2] app/testpmd: improve handling of
> multiprocess
> 
> Hi,
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Marcin Kerlin
> > Sent: Tuesday, September 27, 2016 12:13 PM
> > To: dev@dpdk.org
> > Cc: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>;
> > thomas.monjalon@6wind.com; Kerlin, MarcinX <marcinx.kerlin@intel.com>
> > Subject: [dpdk-dev] [PATCH v4 2/2] app/testpmd: improve handling of
> > multiprocess
> >
> 
> Check patch issues for lines over 80. Please run check patch for next versions
> 
> WARNING:LONG_LINE: line over 80 characters
> #40: FILE: app/test-pmd/testpmd.c:457:
> +                               rte_mp =
> + rte_pktmbuf_pool_create(pool_name, nb_mbuf,
> 
> WARNING:LONG_LINE: line over 80 characters
> #41: FILE: app/test-pmd/testpmd.c:458:
> +                                               mb_mempool_cache, 0,
> + mbuf_seg_size, socket_id);
> 
> total: 0 errors, 2 warnings, 61 lines checked

I did checkpatch test before and everything is fine if you look to source code. 

Checkpatch interprets tab as 8 chars but should as 4. Everything is in the 80 characters.

Regards,
Marcin
> 
> Thanks,
> Reshma

Re: [PATCH v4 2/2] app/testpmd: improve handling of multiprocess

[Pattan, Reshma]

Hi,

> -----Original Message-----
> From: Kerlin, MarcinX
> Sent: Wednesday, September 28, 2016 12:35 PM
> To: Pattan, Reshma <reshma.pattan@intel.com>; dev@dpdk.org
> Cc: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>;
> thomas.monjalon@6wind.com
> Subject: RE: [dpdk-dev] [PATCH v4 2/2] app/testpmd: improve handling of
> multiprocess
> 
> Hi
> I did checkpatch test before and everything is fine if you look to source code.
> 
> Checkpatch interprets tab as 8 chars but should as 4. Everything is in the 80
> characters.

Tab  should  be 8 characters wide as per DDK coding guidelines.
"Line length is recommended to be not more than 80 characters, including comments. [Tab stop size should be assumed to be 8-characters wide"
http://dpdk.org/doc/guides/contributing/coding_style.html

Thanks,
Reshma

[PATCH v3 2/2] app/testpmd: improve handling of multiprocess

[Marcin Kerlin]

Added lookup for pool name because secondary process should attach to
mempool created by primary process rather than create new one.

Added function free_shared_dev_data() used at the exit of the testpmd.
This causes detach devices data from array rte_eth_dev_data[] shared
between all processes. This allows to have a up-to-date list of devices
data and run again secondary application with the same name.

Signed-off-by: Marcin Kerlin <marcinx.kerlin@intel.com>
---
 app/test-pmd/testpmd.c | 36 ++++++++++++++++++++++++++++++++++--
 app/test-pmd/testpmd.h |  1 +
 2 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 1428974..b02f4eb 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -453,8 +453,10 @@ mbuf_pool_create(uint16_t mbuf_seg_size, unsigned nb_mbuf,
 			rte_mempool_obj_iter(rte_mp, rte_pktmbuf_init, NULL);
 		} else {
 			/* wrapper to rte_mempool_create() */
-			rte_mp = rte_pktmbuf_pool_create(pool_name, nb_mbuf,
-				mb_mempool_cache, 0, mbuf_seg_size, socket_id);
+			rte_mp = rte_mempool_lookup(pool_name);
+			if (rte_mp == NULL)
+				rte_mp = rte_pktmbuf_pool_create(pool_name, nb_mbuf,
+						mb_mempool_cache, 0, mbuf_seg_size, socket_id);
 		}
 	}
 
@@ -1610,6 +1612,35 @@ detach_port(uint8_t port_id)
 	return;
 }
 
+void free_shared_dev_data(portid_t pid)
+{
+	portid_t pi;
+
+	if (port_id_is_invalid(pid, ENABLED_WARN))
+		return;
+
+	/* free data only if the secondary process exits */
+	if (rte_eal_process_type() != RTE_PROC_SECONDARY)
+		return;
+
+	printf("Cleaning device data...\n");
+
+	FOREACH_PORT(pi, ports)
+	{
+		if (pid != pi && pid != (portid_t) RTE_PORT_ALL)
+			continue;
+
+		if (!port_is_closed(pi)) {
+			printf("Port %d is not closed now\n", pi);
+			continue;
+		}
+
+		if (rte_eth_dev_release_dev_data(pi) < 0)
+			return;
+	}
+	printf("Done\n");
+}
+
 void
 pmd_test_exit(void)
 {
@@ -1625,6 +1656,7 @@ pmd_test_exit(void)
 			fflush(stdout);
 			stop_port(pt_id);
 			close_port(pt_id);
+			free_shared_dev_data(pt_id);
 		}
 	}
 	printf("\nBye...\n");
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index 2b281cc..3915a06 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -553,6 +553,7 @@ void attach_port(char *identifier);
 void detach_port(uint8_t port_id);
 int all_ports_stopped(void);
 int port_is_started(portid_t port_id);
+void free_shared_dev_data(portid_t pid);
 void pmd_test_exit(void);
 void fdir_get_infos(portid_t port_id);
 void fdir_set_flex_mask(portid_t port_id,
-- 
1.9.1

[PATCH v2 2/2] app/testpmd: improve handling of multiprocess

[Marcin Kerlin]

Added lookup for pool name because secondary process should attach to
mempool created by primary process rather than create new one.

Added function free_shared_dev_data() used at the exit of the testpmd.
This causes detach devices data from array rte_eth_dev_data[] shared
between all processes. This allows to have a up-to-date list of devices
data and run again secondary application with the same name.

Signed-off-by: Marcin Kerlin <marcinx.kerlin@intel.com>
---
 app/test-pmd/testpmd.c | 36 ++++++++++++++++++++++++++++++++++--
 app/test-pmd/testpmd.h |  1 +
 2 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 30749a4..51f6aee 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -452,8 +452,10 @@ mbuf_pool_create(uint16_t mbuf_seg_size, unsigned nb_mbuf,
 			rte_mempool_obj_iter(rte_mp, rte_pktmbuf_init, NULL);
 		} else {
 			/* wrapper to rte_mempool_create() */
-			rte_mp = rte_pktmbuf_pool_create(pool_name, nb_mbuf,
-				mb_mempool_cache, 0, mbuf_seg_size, socket_id);
+			rte_mp = rte_mempool_lookup(pool_name);
+			if (rte_mp == NULL)
+				rte_mp = rte_pktmbuf_pool_create(pool_name, nb_mbuf,
+						mb_mempool_cache, 0, mbuf_seg_size, socket_id);
 		}
 	}
 
@@ -1609,6 +1611,35 @@ detach_port(uint8_t port_id)
 	return;
 }
 
+void free_shared_dev_data(portid_t pid)
+{
+	portid_t pi;
+
+	if (port_id_is_invalid(pid, ENABLED_WARN))
+		return;
+
+	/* free data only if the secondary process exits */
+	if (rte_eal_process_type() != RTE_PROC_SECONDARY)
+		return;
+
+	printf("Cleaning device data...\n");
+
+	FOREACH_PORT(pi, ports)
+	{
+		if (pid != pi && pid != (portid_t) RTE_PORT_ALL)
+			continue;
+
+		if (!port_is_closed(pi)) {
+			printf("Port %d is not closed now\n", pi);
+			continue;
+		}
+
+		if (rte_eth_dev_release_dev_data(pi) < 0)
+			return;
+	}
+	printf("Done\n");
+}
+
 void
 pmd_test_exit(void)
 {
@@ -1624,6 +1655,7 @@ pmd_test_exit(void)
 			fflush(stdout);
 			stop_port(pt_id);
 			close_port(pt_id);
+			free_shared_dev_data(pt_id);
 		}
 	}
 	printf("\nBye...\n");
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index 2b281cc..3915a06 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -553,6 +553,7 @@ void attach_port(char *identifier);
 void detach_port(uint8_t port_id);
 int all_ports_stopped(void);
 int port_is_started(portid_t port_id);
+void free_shared_dev_data(portid_t pid);
 void pmd_test_exit(void);
 void fdir_get_infos(portid_t port_id);
 void fdir_set_flex_mask(portid_t port_id,
-- 
1.9.1

[PATCH 2/2] app/testpmd: improve handling of multiprocess

[Marcin Kerlin]

Added lookup for pool name because secondary process should attach to
mempool created by primary process rather than create new one.

Added function free_shared_dev_data() used at the exit of the testpmd.
This causes detach devices data from array rte_eth_dev_data[] shared
between all processes. This allows to have a up-to-date list of devices
data and run again secondary application with the same name.

Signed-off-by: Marcin Kerlin <marcinx.kerlin@intel.com>
---
 app/test-pmd/testpmd.c | 36 ++++++++++++++++++++++++++++++++++--
 app/test-pmd/testpmd.h |  1 +
 2 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 1428974..b02f4eb 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -453,8 +453,10 @@ mbuf_pool_create(uint16_t mbuf_seg_size, unsigned nb_mbuf,
 			rte_mempool_obj_iter(rte_mp, rte_pktmbuf_init, NULL);
 		} else {
 			/* wrapper to rte_mempool_create() */
-			rte_mp = rte_pktmbuf_pool_create(pool_name, nb_mbuf,
-				mb_mempool_cache, 0, mbuf_seg_size, socket_id);
+			rte_mp = rte_mempool_lookup(pool_name);
+			if (rte_mp == NULL)
+				rte_mp = rte_pktmbuf_pool_create(pool_name, nb_mbuf,
+						mb_mempool_cache, 0, mbuf_seg_size, socket_id);
 		}
 	}
 
@@ -1610,6 +1612,35 @@ detach_port(uint8_t port_id)
 	return;
 }
 
+void free_shared_dev_data(portid_t pid)
+{
+	portid_t pi;
+
+	if (port_id_is_invalid(pid, ENABLED_WARN))
+		return;
+
+	/* free data only if the secondary process exits */
+	if (rte_eal_process_type() != RTE_PROC_SECONDARY)
+		return;
+
+	printf("Cleaning device data...\n");
+
+	FOREACH_PORT(pi, ports)
+	{
+		if (pid != pi && pid != (portid_t) RTE_PORT_ALL)
+			continue;
+
+		if (!port_is_closed(pi)) {
+			printf("Port %d is not closed now\n", pi);
+			continue;
+		}
+
+		if (rte_eth_dev_release_dev_data(pi) < 0)
+			return;
+	}
+	printf("Done\n");
+}
+
 void
 pmd_test_exit(void)
 {
@@ -1625,6 +1656,7 @@ pmd_test_exit(void)
 			fflush(stdout);
 			stop_port(pt_id);
 			close_port(pt_id);
+			free_shared_dev_data(pt_id);
 		}
 	}
 	printf("\nBye...\n");
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index 2b281cc..3915a06 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -553,6 +553,7 @@ void attach_port(char *identifier);
 void detach_port(uint8_t port_id);
 int all_ports_stopped(void);
 int port_is_started(portid_t port_id);
+void free_shared_dev_data(portid_t pid);
 void pmd_test_exit(void);
 void fdir_get_infos(portid_t port_id);
 void fdir_set_flex_mask(portid_t port_id,
-- 
1.9.1