[dpdk-dev,v2] net/tap: fix dev name look-up
Checks
Commit Message
Store the device name in dev->data->name, to have symmetrical behavior
between rte_pmd_tap_probe(name) and rte_pmd_tap_remove(name).
The netdevice name (linux interface name) is stored in the name field of
struct pmd_internals.
There's no need to allocate an rte_eth_dev_data, as it is done in
rte_eth_dev_allocate()/rte_eth_dev_data_alloc().
Signed-off-by: Pascal Mazon <pascal.mazon@6wind.com>
---
drivers/net/tap/rte_eth_tap.c | 15 ++++++---------
1 file changed, 6 insertions(+), 9 deletions(-)
Comments
On 3/6/2017 1:51 PM, Pascal Mazon wrote:
> Store the device name in dev->data->name, to have symmetrical behavior
> between rte_pmd_tap_probe(name) and rte_pmd_tap_remove(name).
>
> The netdevice name (linux interface name) is stored in the name field of
> struct pmd_internals.
+1 for this.
>
> There's no need to allocate an rte_eth_dev_data, as it is done in
> rte_eth_dev_allocate()/rte_eth_dev_data_alloc().
Allocating rte_eth_dev_data, although rte_eth_dev_allocate() does
already, common practice for virtual devices, for a reason.
rte_eth_dev_allocate() allocates device data from a shared memory, so
that multiple DPDK processes can access same device data, this may make
sense for physical devices, since there is a single physical resource.
But for virtual devices, each DPDK process may want to have independent
instance of the virtual device, that is why PMD itself allocates data
and overwrites with existing dev->data.
Also this may lead unexpected results for some multi process use cases,
like secondary process virtual device corrupt primary process physical
device data.
I believe it is good to keep this as it is.
>
> Signed-off-by: Pascal Mazon <pascal.mazon@6wind.com>
<...>
On Mon, 6 Mar 2017 14:27:18 +0000
Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> On 3/6/2017 1:51 PM, Pascal Mazon wrote:
> > Store the device name in dev->data->name, to have symmetrical
> > behavior between rte_pmd_tap_probe(name) and
> > rte_pmd_tap_remove(name).
> >
> > The netdevice name (linux interface name) is stored in the name
> > field of struct pmd_internals.
>
> +1 for this.
>
> >
> > There's no need to allocate an rte_eth_dev_data, as it is done in
> > rte_eth_dev_allocate()/rte_eth_dev_data_alloc().
>
> Allocating rte_eth_dev_data, although rte_eth_dev_allocate() does
> already, common practice for virtual devices, for a reason.
>
> rte_eth_dev_allocate() allocates device data from a shared memory, so
> that multiple DPDK processes can access same device data, this may
> make sense for physical devices, since there is a single physical
> resource.
>
> But for virtual devices, each DPDK process may want to have
> independent instance of the virtual device, that is why PMD itself
> allocates data and overwrites with existing dev->data.
>
> Also this may lead unexpected results for some multi process use
> cases, like secondary process virtual device corrupt primary process
> physical device data.
>
> I believe it is good to keep this as it is.
>
I see your point.
I'll send a v3 with just the correct "name" used for allocation.
And I'll move the snprintf(data->name) closer, to insist it must use the
same name as in allocation.
Regards,
Pascal
> >
> > Signed-off-by: Pascal Mazon <pascal.mazon@6wind.com>
> <...>
@@ -651,23 +651,22 @@ eth_dev_tap_create(const char *name, char *tap_name)
RTE_LOG(DEBUG, PMD, " TAP device on numa %u\n", rte_socket_id());
- data = rte_zmalloc_socket(tap_name, sizeof(*data), 0, numa_node);
- if (!data) {
- RTE_LOG(ERR, PMD, "TAP Failed to allocate data\n");
- goto error_exit;
- }
-
pmd = rte_zmalloc_socket(tap_name, sizeof(*pmd), 0, numa_node);
if (!pmd) {
RTE_LOG(ERR, PMD, "TAP Unable to allocate internal struct\n");
goto error_exit;
}
- dev = rte_eth_dev_allocate(tap_name);
+ dev = rte_eth_dev_allocate(name);
if (!dev) {
RTE_LOG(ERR, PMD, "TAP Unable to allocate device struct\n");
goto error_exit;
}
+ data = dev->data;
+ if (!dev->data) {
+ RTE_LOG(ERR, PMD, "TAP Failed to allocate data\n");
+ goto error_exit;
+ }
snprintf(pmd->name, sizeof(pmd->name), "%s", tap_name);
@@ -686,12 +685,10 @@ eth_dev_tap_create(const char *name, char *tap_name)
data->nb_rx_queues = pmd->nb_queues;
data->nb_tx_queues = pmd->nb_queues;
- dev->data = data;
dev->dev_ops = &ops;
dev->driver = NULL;
dev->rx_pkt_burst = pmd_rx_burst;
dev->tx_pkt_burst = pmd_tx_burst;
- snprintf(dev->data->name, sizeof(dev->data->name), "%s", name);
/* Presetup the fds to -1 as being not valid */
for (i = 0; i < RTE_PMD_TAP_MAX_QUEUES; i++) {