[dpdk-dev,v2,1/2] librte_ether: ensure not overwrite device data in mp app
Commit Message
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(-)
Comments
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
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
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
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
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
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(-)
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
@@ -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(ð_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:
@@ -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.
*
@@ -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;