[PATCH] net/tap: fix dev name look-up

[PATCH] net/tap: fix dev name look-up

[Pascal Mazon]

The call to rte_eth_dev_allocate(tap_name) sets dev->data->name to
tap_name (e.g. "dtap0").

A look-up using tap_name is expected to return this device, not a
look-up using name (e.g. "net_tap0").

Signed-off-by: Pascal Mazon <pascal.mazon@6wind.com>
---
 drivers/net/tap/rte_eth_tap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index 47a706070652..14c345f07afa 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -691,7 +691,7 @@ eth_dev_tap_create(const char *name, char *tap_name)
 	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);
+	snprintf(dev->data->name, sizeof(dev->data->name), "%s", tap_name);
 
 	/* Presetup the fds to -1 as being not valid */
 	for (i = 0; i < RTE_PMD_TAP_MAX_QUEUES; i++) {
-- 
2.8.0.rc0

Re: [PATCH] net/tap: fix dev name look-up

[Wiles, Keith]

> On Mar 3, 2017, at 2:54 AM, Pascal Mazon <pascal.mazon@6wind.com> wrote:
> 
> The call to rte_eth_dev_allocate(tap_name) sets dev->data->name to
> tap_name (e.g. "dtap0").
> 
> A look-up using tap_name is expected to return this device, not a
> look-up using name (e.g. "net_tap0").
> 
> Signed-off-by: Pascal Mazon <pascal.mazon@6wind.com>

Acked-by: Keith Wiles <keith.wiles@intel.com>

> ---
> drivers/net/tap/rte_eth_tap.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> index 47a706070652..14c345f07afa 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -691,7 +691,7 @@ eth_dev_tap_create(const char *name, char *tap_name)
> 	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);
> +	snprintf(dev->data->name, sizeof(dev->data->name), "%s", tap_name);
> 
> 	/* Presetup the fds to -1 as being not valid */
> 	for (i = 0; i < RTE_PMD_TAP_MAX_QUEUES; i++) {
> -- 
> 2.8.0.rc0
> 

Regards,
Keith

Re: [PATCH] net/tap: fix dev name look-up

[Ferruh Yigit]

On 3/3/2017 8:54 AM, Pascal Mazon wrote:
> The call to rte_eth_dev_allocate(tap_name) sets dev->data->name to
> tap_name (e.g. "dtap0").
> 
> A look-up using tap_name is expected to return this device, not a
> look-up using name (e.g. "net_tap0").

This will break rte_pmd_tap_remove(), because it gets device name
(net_tap0) as parameter.

And logically this is wrong too, current eth_dev->data->name is to keep
device name, all other PMDs use this way, not for Linux interface name.

Current tap PMD, first gives "dtap0" to rte_eth_dev_allocate() as
argument, which sets device name to this value. Later with snprintf()
overwrites the device name with correct value, I think better thing to
do is give correct argument to rte_eth_dev_allocate() and remove snprintf()

> 
> Signed-off-by: Pascal Mazon <pascal.mazon@6wind.com>
> ---
>  drivers/net/tap/rte_eth_tap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> index 47a706070652..14c345f07afa 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -691,7 +691,7 @@ eth_dev_tap_create(const char *name, char *tap_name)
>  	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);
> +	snprintf(dev->data->name, sizeof(dev->data->name), "%s", tap_name);
>  
>  	/* Presetup the fds to -1 as being not valid */
>  	for (i = 0; i < RTE_PMD_TAP_MAX_QUEUES; i++) {
> 

[PATCH v2] net/tap: fix dev name look-up

[Pascal Mazon]

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(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index 47a706070652..839c4187a47f 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -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++) {
-- 
2.8.0.rc0

Re: [PATCH] net/tap: fix dev name look-up

[Wiles, Keith]

> On Mar 5, 2017, at 3:35 PM, Yigit, Ferruh <ferruh.yigit@intel.com> wrote:
> 
> On 3/3/2017 8:54 AM, Pascal Mazon wrote:
>> The call to rte_eth_dev_allocate(tap_name) sets dev->data->name to
>> tap_name (e.g. "dtap0").
>> 
>> A look-up using tap_name is expected to return this device, not a
>> look-up using name (e.g. "net_tap0").
> 
> This will break rte_pmd_tap_remove(), because it gets device name
> (net_tap0) as parameter.
> 
> And logically this is wrong too, current eth_dev->data->name is to keep
> device name, all other PMDs use this way, not for Linux interface name.
> 
> Current tap PMD, first gives "dtap0" to rte_eth_dev_allocate() as
> argument, which sets device name to this value. Later with snprintf()
> overwrites the device name with correct value, I think better thing to
> do is give correct argument to rte_eth_dev_allocate() and remove snprintf()

All of these different names for the device is driving me crazy :-)

As long as the name of the device as seen from the ifconfig command or host facing name is dtapX then I am ok. We need to clean this up somehow as it is very confusing.

> 
>> 
>> Signed-off-by: Pascal Mazon <pascal.mazon@6wind.com>
>> ---
>> drivers/net/tap/rte_eth_tap.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
>> index 47a706070652..14c345f07afa 100644
>> --- a/drivers/net/tap/rte_eth_tap.c
>> +++ b/drivers/net/tap/rte_eth_tap.c
>> @@ -691,7 +691,7 @@ eth_dev_tap_create(const char *name, char *tap_name)
>> 	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);
>> +	snprintf(dev->data->name, sizeof(dev->data->name), "%s", tap_name);
>> 
>> 	/* Presetup the fds to -1 as being not valid */
>> 	for (i = 0; i < RTE_PMD_TAP_MAX_QUEUES; i++) {
>> 
> 

Regards,
Keith

Re: [PATCH v2] net/tap: fix dev name look-up

[Ferruh Yigit]

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

Re: [PATCH] net/tap: fix dev name look-up

[Ferruh Yigit]

On 3/6/2017 2:15 PM, Wiles, Keith wrote:
> 
>> On Mar 5, 2017, at 3:35 PM, Yigit, Ferruh <ferruh.yigit@intel.com> wrote:
>>
>> On 3/3/2017 8:54 AM, Pascal Mazon wrote:
>>> The call to rte_eth_dev_allocate(tap_name) sets dev->data->name to
>>> tap_name (e.g. "dtap0").
>>>
>>> A look-up using tap_name is expected to return this device, not a
>>> look-up using name (e.g. "net_tap0").
>>
>> This will break rte_pmd_tap_remove(), because it gets device name
>> (net_tap0) as parameter.
>>
>> And logically this is wrong too, current eth_dev->data->name is to keep
>> device name, all other PMDs use this way, not for Linux interface name.
>>
>> Current tap PMD, first gives "dtap0" to rte_eth_dev_allocate() as
>> argument, which sets device name to this value. Later with snprintf()
>> overwrites the device name with correct value, I think better thing to
>> do is give correct argument to rte_eth_dev_allocate() and remove snprintf()
> 
> All of these different names for the device is driving me crazy :-)
> 
> As long as the name of the device as seen from the ifconfig command or host facing name is dtapX then I am ok. We need to clean this up somehow as it is very confusing.

It is not that bad indeed, from DPDK point of view, there is device and
driver name, and specific to tap, there is also Linux device (interface)
name.

Using Linux device name for DPDK device structures caused confusion for
tap, but I am not sure what to clean up.

> 
>>
>>>
>>> Signed-off-by: Pascal Mazon <pascal.mazon@6wind.com>
>>> ---
>>> drivers/net/tap/rte_eth_tap.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
>>> index 47a706070652..14c345f07afa 100644
>>> --- a/drivers/net/tap/rte_eth_tap.c
>>> +++ b/drivers/net/tap/rte_eth_tap.c
>>> @@ -691,7 +691,7 @@ eth_dev_tap_create(const char *name, char *tap_name)
>>> 	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);
>>> +	snprintf(dev->data->name, sizeof(dev->data->name), "%s", tap_name);
>>>
>>> 	/* Presetup the fds to -1 as being not valid */
>>> 	for (i = 0; i < RTE_PMD_TAP_MAX_QUEUES; i++) {
>>>
>>
> 
> Regards,
> Keith
> 

Re: [PATCH v2] net/tap: fix dev name look-up

[Pascal Mazon]

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

[PATCH v3] net/tap: fix dev name look-up

[Pascal Mazon]

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.

snprintf(data->name) has been moved closer to the rte_ethdev_allocate()
as it should use the same name.

Signed-off-by: Pascal Mazon <pascal.mazon@6wind.com>
---
 drivers/net/tap/rte_eth_tap.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index 47a706070652..ece3a5fcc897 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -663,7 +663,9 @@ eth_dev_tap_create(const char *name, char *tap_name)
 		goto error_exit;
 	}
 
-	dev = rte_eth_dev_allocate(tap_name);
+	/* name in allocation and data->name must be consistent */
+	snprintf(data->name, sizeof(data->name), "%s", name);
+	dev = rte_eth_dev_allocate(name);
 	if (!dev) {
 		RTE_LOG(ERR, PMD, "TAP Unable to allocate device struct\n");
 		goto error_exit;
@@ -691,7 +693,6 @@ eth_dev_tap_create(const char *name, char *tap_name)
 	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++) {
-- 
2.8.0.rc0

Re: [PATCH v3] net/tap: fix dev name look-up

[Wiles, Keith]

> On Mar 6, 2017, at 9:13 AM, Pascal Mazon <pascal.mazon@6wind.com> 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.
> 
> snprintf(data->name) has been moved closer to the rte_ethdev_allocate()
> as it should use the same name.
> 
> Signed-off-by: Pascal Mazon <pascal.mazon@6wind.com>
> ---
> drivers/net/tap/rte_eth_tap.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> index 47a706070652..ece3a5fcc897 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -663,7 +663,9 @@ eth_dev_tap_create(const char *name, char *tap_name)
> 		goto error_exit;
> 	}
> 
> -	dev = rte_eth_dev_allocate(tap_name);
> +	/* name in allocation and data->name must be consistent */
> +	snprintf(data->name, sizeof(data->name), "%s", name);
> +	dev = rte_eth_dev_allocate(name);
> 	if (!dev) {
> 		RTE_LOG(ERR, PMD, "TAP Unable to allocate device struct\n");
> 		goto error_exit;
> @@ -691,7 +693,6 @@ eth_dev_tap_create(const char *name, char *tap_name)
> 	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++) {
> -- 
> 2.8.0.rc0

Looks good to me, but Ferruh seems to be the expect here :-)
> 

Regards,
Keith

Re: [PATCH v3] net/tap: fix dev name look-up

[Ferruh Yigit]

On 3/6/2017 3:13 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.
> 
> snprintf(data->name) has been moved closer to the rte_ethdev_allocate()
> as it should use the same name.
> 
> Signed-off-by: Pascal Mazon <pascal.mazon@6wind.com>
> ---
>  drivers/net/tap/rte_eth_tap.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> index 47a706070652..ece3a5fcc897 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -663,7 +663,9 @@ eth_dev_tap_create(const char *name, char *tap_name)
>  		goto error_exit;
>  	}
>  
> -	dev = rte_eth_dev_allocate(tap_name);
> +	/* name in allocation and data->name must be consistent */
> +	snprintf(data->name, sizeof(data->name), "%s", name);

When you use correct name (name) for rte_eth_dev_allocate(), snprintf()
no more required. rte_eth_dev_allocate() already does it [1].

[1]
http://dpdk.org/browse/dpdk/tree/lib/librte_ether/rte_ethdev.c#n230

> +	dev = rte_eth_dev_allocate(name);
>  	if (!dev) {
>  		RTE_LOG(ERR, PMD, "TAP Unable to allocate device struct\n");
>  		goto error_exit;
> @@ -691,7 +693,6 @@ eth_dev_tap_create(const char *name, char *tap_name)
>  	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++) {
> 

Re: [PATCH v3] net/tap: fix dev name look-up

[Ferruh Yigit]

On 3/6/2017 3:16 PM, Wiles, Keith wrote:
> 
>> On Mar 6, 2017, at 9:13 AM, Pascal Mazon <pascal.mazon@6wind.com> 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.
>>
>> snprintf(data->name) has been moved closer to the rte_ethdev_allocate()
>> as it should use the same name.
>>
>> Signed-off-by: Pascal Mazon <pascal.mazon@6wind.com>
>> ---
>> drivers/net/tap/rte_eth_tap.c | 5 +++--
>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
>> index 47a706070652..ece3a5fcc897 100644
>> --- a/drivers/net/tap/rte_eth_tap.c
>> +++ b/drivers/net/tap/rte_eth_tap.c
>> @@ -663,7 +663,9 @@ eth_dev_tap_create(const char *name, char *tap_name)
>> 		goto error_exit;
>> 	}
>>
>> -	dev = rte_eth_dev_allocate(tap_name);
>> +	/* name in allocation and data->name must be consistent */
>> +	snprintf(data->name, sizeof(data->name), "%s", name);
>> +	dev = rte_eth_dev_allocate(name);
>> 	if (!dev) {
>> 		RTE_LOG(ERR, PMD, "TAP Unable to allocate device struct\n");
>> 		goto error_exit;
>> @@ -691,7 +693,6 @@ eth_dev_tap_create(const char *name, char *tap_name)
>> 	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++) {
>> -- 
>> 2.8.0.rc0
> 
> Looks good to me, but Ferruh seems to be the expect here :-)

One more minor update, and will be fine I guess J

>>
> 
> Regards,
> Keith
> 

Re: [PATCH v3] net/tap: fix dev name look-up

[Pascal Mazon]

On Mon, 6 Mar 2017 15:41:14 +0000
Ferruh Yigit <ferruh.yigit@intel.com> wrote:

> On 3/6/2017 3:13 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.
> > 
> > snprintf(data->name) has been moved closer to the
> > rte_ethdev_allocate() as it should use the same name.
> > 
> > Signed-off-by: Pascal Mazon <pascal.mazon@6wind.com>
> > ---
> >  drivers/net/tap/rte_eth_tap.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/tap/rte_eth_tap.c
> > b/drivers/net/tap/rte_eth_tap.c index 47a706070652..ece3a5fcc897
> > 100644 --- a/drivers/net/tap/rte_eth_tap.c
> > +++ b/drivers/net/tap/rte_eth_tap.c
> > @@ -663,7 +663,9 @@ eth_dev_tap_create(const char *name, char
> > *tap_name) goto error_exit;
> >  	}
> >  
> > -	dev = rte_eth_dev_allocate(tap_name);
> > +	/* name in allocation and data->name must be consistent */
> > +	snprintf(data->name, sizeof(data->name), "%s", name);
> 
> When you use correct name (name) for rte_eth_dev_allocate(),
> snprintf() no more required. rte_eth_dev_allocate() already does it
> [1].
> 
> [1]
> http://dpdk.org/browse/dpdk/tree/lib/librte_ether/rte_ethdev.c#n230
> 

I disagree here. What rte_eth_dev_allocate() changes, is the shared
automatically-allocated data part, as mentioned in your earlier mail.
We didn't yet pass our own manually allocated data to dev.

Do you see what I mean?

Best regards,
Pascal

> > +	dev = rte_eth_dev_allocate(name);
> >  	if (!dev) {
> >  		RTE_LOG(ERR, PMD, "TAP Unable to allocate device
> > struct\n"); goto error_exit;
> > @@ -691,7 +693,6 @@ eth_dev_tap_create(const char *name, char
> > *tap_name) 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++) {
> > 
> 

Re: [PATCH v3] net/tap: fix dev name look-up

[Ferruh Yigit]

On 3/6/2017 3:55 PM, Pascal Mazon wrote:
> On Mon, 6 Mar 2017 15:41:14 +0000
> Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> 
>> On 3/6/2017 3:13 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.
>>>
>>> snprintf(data->name) has been moved closer to the
>>> rte_ethdev_allocate() as it should use the same name.
>>>
>>> Signed-off-by: Pascal Mazon <pascal.mazon@6wind.com>
>>> ---
>>>  drivers/net/tap/rte_eth_tap.c | 5 +++--
>>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/tap/rte_eth_tap.c
>>> b/drivers/net/tap/rte_eth_tap.c index 47a706070652..ece3a5fcc897
>>> 100644 --- a/drivers/net/tap/rte_eth_tap.c
>>> +++ b/drivers/net/tap/rte_eth_tap.c
>>> @@ -663,7 +663,9 @@ eth_dev_tap_create(const char *name, char
>>> *tap_name) goto error_exit;
>>>  	}
>>>  
>>> -	dev = rte_eth_dev_allocate(tap_name);
>>> +	/* name in allocation and data->name must be consistent */
>>> +	snprintf(data->name, sizeof(data->name), "%s", name);
>>
>> When you use correct name (name) for rte_eth_dev_allocate(),
>> snprintf() no more required. rte_eth_dev_allocate() already does it
>> [1].
>>
>> [1]
>> http://dpdk.org/browse/dpdk/tree/lib/librte_ether/rte_ethdev.c#n230
>>
> 
> I disagree here. What rte_eth_dev_allocate() changes, is the shared
> automatically-allocated data part, as mentioned in your earlier mail.
> We didn't yet pass our own manually allocated data to dev.
> 
> Do you see what I mean?

Yes I do, you are right, so this looks good.

> 
> Best regards,
> Pascal
> 
>>> +	dev = rte_eth_dev_allocate(name);
>>>  	if (!dev) {
>>>  		RTE_LOG(ERR, PMD, "TAP Unable to allocate device
>>> struct\n"); goto error_exit;
>>> @@ -691,7 +693,6 @@ eth_dev_tap_create(const char *name, char
>>> *tap_name) 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++) {
>>>
>>
> 

Re: [PATCH v3] net/tap: fix dev name look-up

[Ferruh Yigit]

On 3/6/2017 3:42 PM, Ferruh Yigit wrote:
> On 3/6/2017 3:16 PM, Wiles, Keith wrote:
>>
>>> On Mar 6, 2017, at 9:13 AM, Pascal Mazon <pascal.mazon@6wind.com> 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.
>>>
>>> snprintf(data->name) has been moved closer to the rte_ethdev_allocate()
>>> as it should use the same name.
>>>
>>> Signed-off-by: Pascal Mazon <pascal.mazon@6wind.com>
>>> ---
>>> drivers/net/tap/rte_eth_tap.c | 5 +++--
>>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
>>> index 47a706070652..ece3a5fcc897 100644
>>> --- a/drivers/net/tap/rte_eth_tap.c
>>> +++ b/drivers/net/tap/rte_eth_tap.c
>>> @@ -663,7 +663,9 @@ eth_dev_tap_create(const char *name, char *tap_name)
>>> 		goto error_exit;
>>> 	}
>>>
>>> -	dev = rte_eth_dev_allocate(tap_name);
>>> +	/* name in allocation and data->name must be consistent */
>>> +	snprintf(data->name, sizeof(data->name), "%s", name);
>>> +	dev = rte_eth_dev_allocate(name);
>>> 	if (!dev) {
>>> 		RTE_LOG(ERR, PMD, "TAP Unable to allocate device struct\n");
>>> 		goto error_exit;
>>> @@ -691,7 +693,6 @@ eth_dev_tap_create(const char *name, char *tap_name)
>>> 	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++) {
>>> -- 
>>> 2.8.0.rc0
>>
>> Looks good to me, but Ferruh seems to be the expect here :-)
> 
> One more minor update, and will be fine I guess J

Since no more update required, I am using above as your "Acked-by".

> 
>>>
>>
>> Regards,
>> Keith
>>
> 

Re: [PATCH v3] net/tap: fix dev name look-up

[Wiles, Keith]

> On Mar 6, 2017, at 10:03 AM, Yigit, Ferruh <ferruh.yigit@intel.com> wrote:
> 
> On 3/6/2017 3:42 PM, Ferruh Yigit wrote:
>> On 3/6/2017 3:16 PM, Wiles, Keith wrote:
>>> 
>>>> On Mar 6, 2017, at 9:13 AM, Pascal Mazon <pascal.mazon@6wind.com> 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.
>>>> 
>>>> snprintf(data->name) has been moved closer to the rte_ethdev_allocate()
>>>> as it should use the same name.
>>>> 
>>>> Signed-off-by: Pascal Mazon <pascal.mazon@6wind.com>
>>>> ---
>>>> drivers/net/tap/rte_eth_tap.c | 5 +++--
>>>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>>> 
>>>> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
>>>> index 47a706070652..ece3a5fcc897 100644
>>>> --- a/drivers/net/tap/rte_eth_tap.c
>>>> +++ b/drivers/net/tap/rte_eth_tap.c
>>>> @@ -663,7 +663,9 @@ eth_dev_tap_create(const char *name, char *tap_name)
>>>> 		goto error_exit;
>>>> 	}
>>>> 
>>>> -	dev = rte_eth_dev_allocate(tap_name);
>>>> +	/* name in allocation and data->name must be consistent */
>>>> +	snprintf(data->name, sizeof(data->name), "%s", name);
>>>> +	dev = rte_eth_dev_allocate(name);
>>>> 	if (!dev) {
>>>> 		RTE_LOG(ERR, PMD, "TAP Unable to allocate device struct\n");
>>>> 		goto error_exit;
>>>> @@ -691,7 +693,6 @@ eth_dev_tap_create(const char *name, char *tap_name)
>>>> 	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++) {
>>>> -- 
>>>> 2.8.0.rc0
>>> 
>>> Looks good to me, but Ferruh seems to be the expect here :-)
>> 
>> One more minor update, and will be fine I guess J
> 
> Since no more update required, I am using above as your "Acked-by”.

Acked-by: Keith Wiles <keith.wiles@intel.com>

> 
>> 
>>>> 
>>> 
>>> Regards,
>>> Keith

Regards,
Keith

Re: [PATCH v3] net/tap: fix dev name look-up

[Ferruh Yigit]

On 3/6/2017 4:14 PM, Wiles, Keith wrote:
> 
>> On Mar 6, 2017, at 10:03 AM, Yigit, Ferruh <ferruh.yigit@intel.com> wrote:
>>
>> On 3/6/2017 3:42 PM, Ferruh Yigit wrote:
>>> On 3/6/2017 3:16 PM, Wiles, Keith wrote:
>>>>
>>>>> On Mar 6, 2017, at 9:13 AM, Pascal Mazon <pascal.mazon@6wind.com> 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.
>>>>>
>>>>> snprintf(data->name) has been moved closer to the rte_ethdev_allocate()
>>>>> as it should use the same name.
>>>>>
>>>>> Signed-off-by: Pascal Mazon <pascal.mazon@6wind.com>

> Acked-by: Keith Wiles <keith.wiles@intel.com>

Applied to dpdk-next-net/master, thanks.