[dpdk-dev] [PATCH 1/6] net/tap: use correct tap name

[dpdk-dev] [PATCH 1/6] net/tap: use correct tap name

[Pascal Mazon]

dev->data->name contains "net_tap", the device driver name.
dev->data->dev_private->name contains the actual iface name,
e.g. "dtap0".

In tun_alloc() especially, we want to use the latter. Otherwise the
netdevice would be wrongly named "net_tap". Furthermore, creating
several tap vdev would point to the same netdevice.

In any case, it must to be consistent with the tun_alloc() call in
eth_dev_tap_create().

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

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index 91f63f5468b2..8faf08551b9e 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -410,6 +410,7 @@ tap_setup_queue(struct rte_eth_dev *dev,
 		struct pmd_internals *internals,
 		uint16_t qid)
 {
+	struct pmd_internals *pmd = dev->data->dev_private;
 	struct rx_queue *rx = &internals->rxq[qid];
 	struct tx_queue *tx = &internals->txq[qid];
 	int fd;
@@ -419,11 +420,10 @@ tap_setup_queue(struct rte_eth_dev *dev,
 		fd = tx->fd;
 		if (fd < 0) {
 			RTE_LOG(INFO, PMD, "Add queue to TAP %s for qid %d\n",
-				dev->data->name, qid);
-			fd = tun_alloc(dev->data->name);
+				pmd->name, qid);
+			fd = tun_alloc(pmd->name);
 			if (fd < 0) {
-				RTE_LOG(ERR, PMD, "tun_alloc(%s) failed\n",
-					dev->data->name);
+				RTE_LOG(ERR, PMD, "tun_alloc(%s) failed\n", pmd->name);
 				return -1;
 			}
 		}
@@ -493,7 +493,7 @@ tap_rx_queue_setup(struct rte_eth_dev *dev,
 
 	internals->fds[rx_queue_id] = fd;
 	RTE_LOG(INFO, PMD, "RX TAP device name %s, qid %d on fd %d\n",
-		dev->data->name, rx_queue_id, internals->rxq[rx_queue_id].fd);
+		internals->name, rx_queue_id, internals->rxq[rx_queue_id].fd);
 
 	return 0;
 }
@@ -516,7 +516,7 @@ tap_tx_queue_setup(struct rte_eth_dev *dev,
 		return -1;
 
 	RTE_LOG(INFO, PMD, "TX TAP device name %s, qid %d on fd %d\n",
-		dev->data->name, tx_queue_id, internals->txq[tx_queue_id].fd);
+		internals->name, tx_queue_id, internals->txq[tx_queue_id].fd);
 
 	return 0;
 }
-- 
2.8.0.rc0

[dpdk-dev] [PATCH 2/6] net/tap: use correct channel for error logs

[Pascal Mazon]

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

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index 8faf08551b9e..4cc1767da5e8 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -594,20 +594,20 @@ eth_dev_tap_create(const char *name, char *tap_name)
 
 	data = rte_zmalloc_socket(tap_name, sizeof(*data), 0, numa_node);
 	if (!data) {
-		RTE_LOG(INFO, PMD, "Failed to allocate data\n");
+		RTE_LOG(ERR, PMD, "Failed to allocate data\n");
 		goto error_exit;
 	}
 
 	pmd = rte_zmalloc_socket(tap_name, sizeof(*pmd), 0, numa_node);
 	if (!pmd) {
-		RTE_LOG(INFO, PMD, "Unable to allocate internal struct\n");
+		RTE_LOG(ERR, PMD, "Unable to allocate internal struct\n");
 		goto error_exit;
 	}
 
 	/* Use the name and not the tap_name */
 	dev = rte_eth_dev_allocate(tap_name);
 	if (!dev) {
-		RTE_LOG(INFO, PMD, "Unable to allocate device struct\n");
+		RTE_LOG(ERR, PMD, "Unable to allocate device struct\n");
 		goto error_exit;
 	}
 
@@ -638,7 +638,7 @@ eth_dev_tap_create(const char *name, char *tap_name)
 	/* Create the first Tap device */
 	fd = tun_alloc(tap_name);
 	if (fd < 0) {
-		RTE_LOG(INFO, PMD, "tun_alloc() failed\n");
+		RTE_LOG(ERR, PMD, "tun_alloc() failed\n");
 		goto error_exit;
 	}
 
@@ -655,7 +655,7 @@ eth_dev_tap_create(const char *name, char *tap_name)
 	pmd->fds[0] = fd;
 
 	if (pmd_mac_address(fd, dev, &pmd->eth_addr) < 0) {
-		RTE_LOG(INFO, PMD, "Unable to get MAC address\n");
+		RTE_LOG(ERR, PMD, "Unable to get MAC address\n");
 		goto error_exit;
 	}
 
-- 
2.8.0.rc0

[dpdk-dev] [PATCH 3/6] net/tap: don't set fd value overwritten just below

[Pascal Mazon]

pmd->fds[0], pmd->rxq[0] and pmd->txq[0] are set a couple of lines after
the for loop that initializes them to -1.

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 4cc1767da5e8..3d15031008c6 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -643,7 +643,7 @@ eth_dev_tap_create(const char *name, char *tap_name)
 	}
 
 	/* Presetup the fds to -1 as being not working */
-	for (i = 0; i < RTE_PMD_TAP_MAX_QUEUES; i++) {
+	for (i = 1; i < RTE_PMD_TAP_MAX_QUEUES; i++) {
 		pmd->fds[i] = -1;
 		pmd->rxq[i].fd = -1;
 		pmd->txq[i].fd = -1;
-- 
2.8.0.rc0

[dpdk-dev] [PATCH 4/6] net/tap: keep kernel-assigned MAC address

[Pascal Mazon]

There's no point in having a different internal MAC address than the one
provided by the kernel when creating the netdevice.

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

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index 3d15031008c6..f8b07b4a8fa1 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -552,28 +552,6 @@ pmd_mac_address(int fd, struct rte_eth_dev *dev, struct ether_addr *addr)
 		return -1;
 	}
 
-	/* Set the host based MAC address to this special MAC format */
-	ifr.ifr_hwaddr.sa_data[0] = 'T';
-	ifr.ifr_hwaddr.sa_data[1] = 'a';
-	ifr.ifr_hwaddr.sa_data[2] = 'p';
-	ifr.ifr_hwaddr.sa_data[3] = '-';
-	ifr.ifr_hwaddr.sa_data[4] = dev->data->port_id;
-	ifr.ifr_hwaddr.sa_data[5] = dev->data->numa_node;
-	if (ioctl(fd, SIOCSIFHWADDR, &ifr) == -1) {
-		RTE_LOG(ERR, PMD, "%s: ioctl failed (SIOCSIFHWADDR) (%s)\n",
-			dev->data->name, ifr.ifr_name);
-		return -1;
-	}
-
-	/* Set the local application MAC address, needs to be different then
-	 * the host based MAC address.
-	 */
-	ifr.ifr_hwaddr.sa_data[0] = 'd';
-	ifr.ifr_hwaddr.sa_data[1] = 'n';
-	ifr.ifr_hwaddr.sa_data[2] = 'e';
-	ifr.ifr_hwaddr.sa_data[3] = 't';
-	ifr.ifr_hwaddr.sa_data[4] = dev->data->port_id;
-	ifr.ifr_hwaddr.sa_data[5] = dev->data->numa_node;
 	rte_memcpy(addr, ifr.ifr_hwaddr.sa_data, ETH_ALEN);
 
 	return 0;
-- 
2.8.0.rc0

[dpdk-dev] [PATCH 5/6] net/tap: display tap name after parsing

[Pascal Mazon]

The probe parses for user-defined iface name. Let's use that value.

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

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index f8b07b4a8fa1..734e3a579219 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -690,9 +690,6 @@ rte_pmd_tap_probe(const char *name, const char *params)
 	snprintf(tap_name, sizeof(tap_name), "%s%d",
 		 DEFAULT_TAP_NAME, tap_unit++);
 
-	RTE_LOG(INFO, PMD, "Initializing pmd_tap for %s as %s\n",
-		name, tap_name);
-
 	if (params && (params[0] != '\0')) {
 		RTE_LOG(INFO, PMD, "paramaters (%s)\n", params);
 
@@ -719,6 +716,9 @@ rte_pmd_tap_probe(const char *name, const char *params)
 	}
 	pmd_link.link_speed = speed;
 
+	RTE_LOG(INFO, PMD, "Initializing pmd_tap for %s as %s\n",
+		name, tap_name);
+
 	ret = eth_dev_tap_create(name, tap_name);
 
 leave:
-- 
2.8.0.rc0

[dpdk-dev] [PATCH 6/6] net/tap: implement link up and down callbacks

[Pascal Mazon]

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

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index 734e3a579219..9b6bbff5fd81 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -405,6 +405,63 @@ tap_link_update(struct rte_eth_dev *dev __rte_unused,
 	return 0;
 }
 
+static int tap_link_set(struct pmd_internals *pmd, int state)
+{
+	struct ifreq ifr;
+	int err, s;
+
+	/*
+	 * An AF_INET/DGRAM socket is needed for
+	 * SIOCGIFFLAGS/SIOCSIFFLAGS, using fd won't work.
+	 */
+	s = socket(AF_INET, SOCK_DGRAM, 0);
+	if (s < 0) {
+		RTE_LOG(ERR, PMD,
+			"Unable to get a socket to set flags: %s\n",
+			strerror(errno));
+		return -1;
+	}
+	memset(&ifr, 0, sizeof(ifr));
+	strncpy(ifr.ifr_name, pmd->name, IFNAMSIZ);
+	err = ioctl(s, SIOCGIFFLAGS, &ifr);
+	if (err < 0) {
+		RTE_LOG(ERR, PMD, "Unable to get tap netdevice flags: %s\n",
+			strerror(errno));
+		close(s);
+		return -1;
+	}
+	if (state == ETH_LINK_UP)
+		ifr.ifr_flags |= IFF_UP | IFF_NOARP;
+	else
+		ifr.ifr_flags &= ~(IFF_UP | IFF_NOARP);
+	err = ioctl(s, SIOCSIFFLAGS, &ifr);
+	if (err < 0) {
+		RTE_LOG(ERR, PMD, "Unable to set flags %s: %s\n",
+			state == ETH_LINK_UP ? "UP" : "DOWN", strerror(errno));
+		close(s);
+		return -1;
+	}
+	close(s);
+
+	return 0;
+}
+
+static int
+tap_link_set_down(struct rte_eth_dev *dev)
+{
+	struct pmd_internals *pmd = dev->data->dev_private;
+
+	return tap_link_set(pmd, ETH_LINK_DOWN);
+}
+
+static int
+tap_link_set_up(struct rte_eth_dev *dev)
+{
+	struct pmd_internals *pmd = dev->data->dev_private;
+
+	return tap_link_set(pmd, ETH_LINK_UP);
+}
+
 static int
 tap_setup_queue(struct rte_eth_dev *dev,
 		struct pmd_internals *internals,
@@ -532,6 +589,8 @@ static const struct eth_dev_ops ops = {
 	.rx_queue_release       = tap_rx_queue_release,
 	.tx_queue_release       = tap_tx_queue_release,
 	.link_update            = tap_link_update,
+	.dev_set_link_up        = tap_link_set_up,
+	.dev_set_link_down      = tap_link_set_down,
 	.stats_get              = tap_stats_get,
 	.stats_reset            = tap_stats_reset,
 };
-- 
2.8.0.rc0

Re: [dpdk-dev] [PATCH 1/6] net/tap: use correct tap name

[Ferruh Yigit]

On 1/31/2017 9:42 AM, Pascal Mazon wrote:
> dev->data->name contains "net_tap", the device driver name.

I see what patch does, just as a note to commit log:

AFAIK, "dev->data->name" is device name, and for this case it is
"net_tap#", like "net_tap0", "net_tap1" ...

"dev->data_drv_name" is the driver name which is "net_tap"

> dev->data->dev_private->name contains the actual iface name,
> e.g. "dtap0".

Right, I agree this is correct comparing "dev->data->name"

But the problem is pmd->name is per eth_dev.

If I read code correct, for multiple queue support, each queue pair will
create a tap device, so each needs a different name.

So can't just use pmd->name. Need to create a name per queue pair, it
can be combination of pmd->name + "_" + queue_id? Or can keep a name per
queue pair, instead of eth_dev.

What do you think?

> 
> In tun_alloc() especially, we want to use the latter. Otherwise the
> netdevice would be wrongly named "net_tap". Furthermore, creating
> several tap vdev would point to the same netdevice.
> 
> In any case, it must to be consistent with the tun_alloc() call in
> eth_dev_tap_create().
> 
> Signed-off-by: Pascal Mazon <pascal.mazon@6wind.com>
<...>

Re: [dpdk-dev] [PATCH 2/6] net/tap: use correct channel for error logs

[Ferruh Yigit]

On 1/31/2017 9:42 AM, Pascal Mazon wrote:
> Signed-off-by: Pascal Mazon <pascal.mazon@6wind.com>
> ---

looks good to me.

Re: [dpdk-dev] [PATCH 4/6] net/tap: keep kernel-assigned MAC address

[Ferruh Yigit]

On 1/31/2017 9:42 AM, Pascal Mazon wrote:
> There's no point in having a different internal MAC address than the one
> provided by the kernel when creating the netdevice.

Agree that this is not required, but also not a defect, worth learning
author's intention before deciding.

> 
> Signed-off-by: Pascal Mazon <pascal.mazon@6wind.com>
> ---
<...>

Re: [dpdk-dev] [PATCH 5/6] net/tap: display tap name after parsing

[Ferruh Yigit]

On 1/31/2017 9:42 AM, Pascal Mazon wrote:
> The probe parses for user-defined iface name. Let's use that value.
> 
> Signed-off-by: Pascal Mazon <pascal.mazon@6wind.com>

looks good to me.

Re: [dpdk-dev] [PATCH 6/6] net/tap: implement link up and down callbacks

[Ferruh Yigit]

On 1/31/2017 9:42 AM, Pascal Mazon wrote:
> Signed-off-by: Pascal Mazon <pascal.mazon@6wind.com>
> ---
>  drivers/net/tap/rte_eth_tap.c | 59 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 59 insertions(+)
> 
> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> index 734e3a579219..9b6bbff5fd81 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -405,6 +405,63 @@ tap_link_update(struct rte_eth_dev *dev __rte_unused,
>  	return 0;
>  }
>  
> +static int tap_link_set(struct pmd_internals *pmd, int state)
> +{
> +	struct ifreq ifr;
> +	int err, s;
> +
> +	/*
> +	 * An AF_INET/DGRAM socket is needed for
> +	 * SIOCGIFFLAGS/SIOCSIFFLAGS, using fd won't work.
> +	 */
> +	s = socket(AF_INET, SOCK_DGRAM, 0);
> +	if (s < 0) {
> +		RTE_LOG(ERR, PMD,
> +			"Unable to get a socket to set flags: %s\n",
> +			strerror(errno));
> +		return -1;
> +	}
> +	memset(&ifr, 0, sizeof(ifr));
> +	strncpy(ifr.ifr_name, pmd->name, IFNAMSIZ);

Again how this will behave for multiple queue setup.

Rest looks good.

> +	err = ioctl(s, SIOCGIFFLAGS, &ifr);
> +	if (err < 0) {
> +		RTE_LOG(ERR, PMD, "Unable to get tap netdevice flags: %s\n",
> +			strerror(errno));
> +		close(s);
> +		return -1;
> +	}
<...>

Re: [dpdk-dev] [PATCH 1/6] net/tap: use correct tap name

[Pascal Mazon]

On 01/31/2017 02:06 PM, Ferruh Yigit wrote:> On 1/31/2017 9:42 AM, 
Pascal Mazon wrote:
>> dev->data->name contains "net_tap", the device driver name.
>
> I see what patch does, just as a note to commit log:
>
> AFAIK, "dev->data->name" is device name, and for this case it is
> "net_tap#", like "net_tap0", "net_tap1" ...
>
> "dev->data_drv_name" is the driver name which is "net_tap"

Indeed, dev->data->name is the device name, looking like "net_tap#",
with number increasing for each vdev.
I'll put the following commit log line if that's ok:

     dev->data->name contains the device name, e.g. "net_tap0".

>
>> dev->data->dev_private->name contains the actual iface name,
>> e.g. "dtap0".
>
> Right, I agree this is correct comparing "dev->data->name"
>
> But the problem is pmd->name is per eth_dev.
>
> If I read code correct, for multiple queue support, each queue pair will
> create a tap device, so each needs a different name.
>
> So can't just use pmd->name. Need to create a name per queue pair, it
> can be combination of pmd->name + "_" + queue_id? Or can keep a name per
> queue pair, instead of eth_dev.
>
> What do you think?

Actually that's not exactly how it goes.
Adding a queue to a netdevice requires to open("/dev/net/tun") and setting
TUNSETIFF (through ioctl) on the resulting fd.
That's the important part: queues for a given tap device must set TUNSETIFF
with a common ifreq.ifr_name (in our case, pmd->name).

This is best explained in the kernel doc, there:

[1] 
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/networking/tuntap.txt#n108

>
>>
>> In tun_alloc() especially, we want to use the latter. Otherwise the
>> netdevice would be wrongly named "net_tap". Furthermore, creating
>> several tap vdev would point to the same netdevice.
>>
>> In any case, it must to be consistent with the tun_alloc() call in
>> eth_dev_tap_create().
>>
>> Signed-off-by: Pascal Mazon <pascal.mazon@6wind.com>
> <...>
>


--
Pascal Mazon
www.6wind.com

Re: [dpdk-dev] [PATCH 6/6] net/tap: implement link up and down callbacks

[Pascal Mazon]

On 01/31/2017 02:21 PM, Ferruh Yigit wrote:
> On 1/31/2017 9:42 AM, Pascal Mazon wrote:
>> Signed-off-by: Pascal Mazon <pascal.mazon@6wind.com>
>> ---
>>  drivers/net/tap/rte_eth_tap.c | 59 +++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 59 insertions(+)
>>
>> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
>> index 734e3a579219..9b6bbff5fd81 100644
>> --- a/drivers/net/tap/rte_eth_tap.c
>> +++ b/drivers/net/tap/rte_eth_tap.c
>> @@ -405,6 +405,63 @@ tap_link_update(struct rte_eth_dev *dev __rte_unused,
>>  	return 0;
>>  }
>>
>> +static int tap_link_set(struct pmd_internals *pmd, int state)
>> +{
>> +	struct ifreq ifr;
>> +	int err, s;
>> +
>> +	/*
>> +	 * An AF_INET/DGRAM socket is needed for
>> +	 * SIOCGIFFLAGS/SIOCSIFFLAGS, using fd won't work.
>> +	 */
>> +	s = socket(AF_INET, SOCK_DGRAM, 0);
>> +	if (s < 0) {
>> +		RTE_LOG(ERR, PMD,
>> +			"Unable to get a socket to set flags: %s\n",
>> +			strerror(errno));
>> +		return -1;
>> +	}
>> +	memset(&ifr, 0, sizeof(ifr));
>> +	strncpy(ifr.ifr_name, pmd->name, IFNAMSIZ);
>
> Again how this will behave for multiple queue setup.

The link status is not related to the number of queues.
As long as there is at least one queue configured (if the last queue is
closed, then the netdevice is effectively deleted entirely), it is
possible to set the link up or down.
With the link down, no queues will be able to receive or send packets.

>
> Rest looks good.
>
>> +	err = ioctl(s, SIOCGIFFLAGS, &ifr);
>> +	if (err < 0) {
>> +		RTE_LOG(ERR, PMD, "Unable to get tap netdevice flags: %s\n",
>> +			strerror(errno));
>> +		close(s);
>> +		return -1;
>> +	}
> <...>
>


-- 
Pascal Mazon
www.6wind.com

Re: [dpdk-dev] [PATCH 1/6] net/tap: use correct tap name

[Wiles, Keith]

> On Jan 31, 2017, at 3:42 AM, Pascal Mazon <pascal.mazon@6wind.com> wrote:
> 
> dev->data->name contains "net_tap", the device driver name.
> dev->data->dev_private->name contains the actual iface name,
> e.g. "dtap0".
> 
> In tun_alloc() especially, we want to use the latter. Otherwise the
> netdevice would be wrongly named "net_tap". Furthermore, creating
> several tap vdev would point to the same netdevice.
> 
> In any case, it must to be consistent with the tun_alloc() call in
> eth_dev_tap_create().
> 
> Signed-off-by: Pascal Mazon <pascal.mazon@6wind.com>
> ---
> drivers/net/tap/rte_eth_tap.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> index 91f63f5468b2..8faf08551b9e 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -410,6 +410,7 @@ tap_setup_queue(struct rte_eth_dev *dev,
> 		struct pmd_internals *internals,
> 		uint16_t qid)
> {
> +	struct pmd_internals *pmd = dev->data->dev_private;
> 	struct rx_queue *rx = &internals->rxq[qid];
> 	struct tx_queue *tx = &internals->txq[qid];
> 	int fd;
> @@ -419,11 +420,10 @@ tap_setup_queue(struct rte_eth_dev *dev,
> 		fd = tx->fd;
> 		if (fd < 0) {
> 			RTE_LOG(INFO, PMD, "Add queue to TAP %s for qid %d\n",
> -				dev->data->name, qid);
> -			fd = tun_alloc(dev->data->name);
> +				pmd->name, qid);
> +			fd = tun_alloc(pmd->name);
> 			if (fd < 0) {
> -				RTE_LOG(ERR, PMD, "tun_alloc(%s) failed\n",
> -					dev->data->name);
> +				RTE_LOG(ERR, PMD, "tun_alloc(%s) failed\n", pmd->name);
> 				return -1;
> 			}
> 		}
> @@ -493,7 +493,7 @@ tap_rx_queue_setup(struct rte_eth_dev *dev,
> 
> 	internals->fds[rx_queue_id] = fd;
> 	RTE_LOG(INFO, PMD, "RX TAP device name %s, qid %d on fd %d\n",
> -		dev->data->name, rx_queue_id, internals->rxq[rx_queue_id].fd);
> +		internals->name, rx_queue_id, internals->rxq[rx_queue_id].fd);
> 
> 	return 0;
> }
> @@ -516,7 +516,7 @@ tap_tx_queue_setup(struct rte_eth_dev *dev,
> 		return -1;
> 
> 	RTE_LOG(INFO, PMD, "TX TAP device name %s, qid %d on fd %d\n",
> -		dev->data->name, tx_queue_id, internals->txq[tx_queue_id].fd);
> +		internals->name, tx_queue_id, internals->txq[tx_queue_id].fd);
> 
> 	return 0;
> }
> -- 
> 2.8.0.rc0

I have not looked at the code completely yet, but it seems reasonable. The only problem is I did have a cleanup patch for the TAP PMD, but Ferruh suggested it was way too many changes at this time for RC2. Are we still under that restriction or when would you suggest this be applied?

> 

Regards,
Keith

Re: [dpdk-dev] [PATCH 1/6] net/tap: use correct tap name

[Ferruh Yigit]

On 1/31/2017 2:52 PM, Wiles, Keith wrote:
> 
>> On Jan 31, 2017, at 3:42 AM, Pascal Mazon <pascal.mazon@6wind.com> wrote:
>>
>> dev->data->name contains "net_tap", the device driver name.
>> dev->data->dev_private->name contains the actual iface name,
>> e.g. "dtap0".
>>
>> In tun_alloc() especially, we want to use the latter. Otherwise the
>> netdevice would be wrongly named "net_tap". Furthermore, creating
>> several tap vdev would point to the same netdevice.
>>
>> In any case, it must to be consistent with the tun_alloc() call in
>> eth_dev_tap_create().
>>
>> Signed-off-by: Pascal Mazon <pascal.mazon@6wind.com>
>> ---
>> drivers/net/tap/rte_eth_tap.c | 12 ++++++------
>> 1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
>> index 91f63f5468b2..8faf08551b9e 100644
>> --- a/drivers/net/tap/rte_eth_tap.c
>> +++ b/drivers/net/tap/rte_eth_tap.c
>> @@ -410,6 +410,7 @@ tap_setup_queue(struct rte_eth_dev *dev,
>> 		struct pmd_internals *internals,
>> 		uint16_t qid)
>> {
>> +	struct pmd_internals *pmd = dev->data->dev_private;
>> 	struct rx_queue *rx = &internals->rxq[qid];
>> 	struct tx_queue *tx = &internals->txq[qid];
>> 	int fd;
>> @@ -419,11 +420,10 @@ tap_setup_queue(struct rte_eth_dev *dev,
>> 		fd = tx->fd;
>> 		if (fd < 0) {
>> 			RTE_LOG(INFO, PMD, "Add queue to TAP %s for qid %d\n",
>> -				dev->data->name, qid);
>> -			fd = tun_alloc(dev->data->name);
>> +				pmd->name, qid);
>> +			fd = tun_alloc(pmd->name);
>> 			if (fd < 0) {
>> -				RTE_LOG(ERR, PMD, "tun_alloc(%s) failed\n",
>> -					dev->data->name);
>> +				RTE_LOG(ERR, PMD, "tun_alloc(%s) failed\n", pmd->name);
>> 				return -1;
>> 			}
>> 		}
>> @@ -493,7 +493,7 @@ tap_rx_queue_setup(struct rte_eth_dev *dev,
>>
>> 	internals->fds[rx_queue_id] = fd;
>> 	RTE_LOG(INFO, PMD, "RX TAP device name %s, qid %d on fd %d\n",
>> -		dev->data->name, rx_queue_id, internals->rxq[rx_queue_id].fd);
>> +		internals->name, rx_queue_id, internals->rxq[rx_queue_id].fd);
>>
>> 	return 0;
>> }
>> @@ -516,7 +516,7 @@ tap_tx_queue_setup(struct rte_eth_dev *dev,
>> 		return -1;
>>
>> 	RTE_LOG(INFO, PMD, "TX TAP device name %s, qid %d on fd %d\n",
>> -		dev->data->name, tx_queue_id, internals->txq[tx_queue_id].fd);
>> +		internals->name, tx_queue_id, internals->txq[tx_queue_id].fd);
>>
>> 	return 0;
>> }
>> -- 
>> 2.8.0.rc0
> 
> I have not looked at the code completely yet, but it seems reasonable. The only problem is I did have a cleanup patch for the TAP PMD, but Ferruh suggested it was way too many changes at this time for RC2. Are we still under that restriction or when would you suggest this be applied?

RC2 is out now, I was aiming that tap PMD works with testpmd with RC2.
Now there is some time for RC3 and some fixes can go in, no new features
but fixes.

> 
>>
> 
> Regards,
> Keith
> 

Re: [dpdk-dev] [PATCH 1/6] net/tap: use correct tap name

[Wiles, Keith]

> On Jan 31, 2017, at 9:14 AM, Yigit, Ferruh <ferruh.yigit@intel.com> wrote:
> 
> On 1/31/2017 2:52 PM, Wiles, Keith wrote:
>> 
>>> On Jan 31, 2017, at 3:42 AM, Pascal Mazon <pascal.mazon@6wind.com> wrote:
>>> 
>>> dev->data->name contains "net_tap", the device driver name.
>>> dev->data->dev_private->name contains the actual iface name,
>>> e.g. "dtap0".
>>> 
>>> In tun_alloc() especially, we want to use the latter. Otherwise the
>>> netdevice would be wrongly named "net_tap". Furthermore, creating
>>> several tap vdev would point to the same netdevice.
>>> 
>>> In any case, it must to be consistent with the tun_alloc() call in
>>> eth_dev_tap_create().
>>> 
>>> Signed-off-by: Pascal Mazon <pascal.mazon@6wind.com>
>>> ---
>>> drivers/net/tap/rte_eth_tap.c | 12 ++++++------
>>> 1 file changed, 6 insertions(+), 6 deletions(-)
>>> 
>>> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
>>> index 91f63f5468b2..8faf08551b9e 100644
>>> --- a/drivers/net/tap/rte_eth_tap.c
>>> +++ b/drivers/net/tap/rte_eth_tap.c
>>> @@ -410,6 +410,7 @@ tap_setup_queue(struct rte_eth_dev *dev,
>>> 		struct pmd_internals *internals,
>>> 		uint16_t qid)
>>> {
>>> +	struct pmd_internals *pmd = dev->data->dev_private;
>>> 	struct rx_queue *rx = &internals->rxq[qid];
>>> 	struct tx_queue *tx = &internals->txq[qid];
>>> 	int fd;
>>> @@ -419,11 +420,10 @@ tap_setup_queue(struct rte_eth_dev *dev,
>>> 		fd = tx->fd;
>>> 		if (fd < 0) {
>>> 			RTE_LOG(INFO, PMD, "Add queue to TAP %s for qid %d\n",
>>> -				dev->data->name, qid);
>>> -			fd = tun_alloc(dev->data->name);
>>> +				pmd->name, qid);
>>> +			fd = tun_alloc(pmd->name);
>>> 			if (fd < 0) {
>>> -				RTE_LOG(ERR, PMD, "tun_alloc(%s) failed\n",
>>> -					dev->data->name);
>>> +				RTE_LOG(ERR, PMD, "tun_alloc(%s) failed\n", pmd->name);
>>> 				return -1;
>>> 			}
>>> 		}
>>> @@ -493,7 +493,7 @@ tap_rx_queue_setup(struct rte_eth_dev *dev,
>>> 
>>> 	internals->fds[rx_queue_id] = fd;
>>> 	RTE_LOG(INFO, PMD, "RX TAP device name %s, qid %d on fd %d\n",
>>> -		dev->data->name, rx_queue_id, internals->rxq[rx_queue_id].fd);
>>> +		internals->name, rx_queue_id, internals->rxq[rx_queue_id].fd);
>>> 
>>> 	return 0;
>>> }
>>> @@ -516,7 +516,7 @@ tap_tx_queue_setup(struct rte_eth_dev *dev,
>>> 		return -1;
>>> 
>>> 	RTE_LOG(INFO, PMD, "TX TAP device name %s, qid %d on fd %d\n",
>>> -		dev->data->name, tx_queue_id, internals->txq[tx_queue_id].fd);
>>> +		internals->name, tx_queue_id, internals->txq[tx_queue_id].fd);
>>> 
>>> 	return 0;
>>> }
>>> -- 
>>> 2.8.0.rc0
>> 
>> I have not looked at the code completely yet, but it seems reasonable. The only problem is I did have a cleanup patch for the TAP PMD, but Ferruh suggested it was way too many changes at this time for RC2. Are we still under that restriction or when would you suggest this be applied?
> 
> RC2 is out now, I was aiming that tap PMD works with testpmd with RC2.
> Now there is some time for RC3 and some fixes can go in, no new features
> but fixes.

OK, I see. Let me integrate the changes suggested and see how they work with my changes. I may send a patch with a combination of the changes and see what everyone thinks.

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

Regards,
Keith

Re: [dpdk-dev] [PATCH 1/6] net/tap: use correct tap name

[Ferruh Yigit]

On 1/31/2017 2:23 PM, Pascal Mazon wrote:
> On 01/31/2017 02:06 PM, Ferruh Yigit wrote:> On 1/31/2017 9:42 AM, 
> Pascal Mazon wrote:
>>> dev->data->name contains "net_tap", the device driver name.
>>
>> I see what patch does, just as a note to commit log:
>>
>> AFAIK, "dev->data->name" is device name, and for this case it is
>> "net_tap#", like "net_tap0", "net_tap1" ...
>>
>> "dev->data_drv_name" is the driver name which is "net_tap"
> 
> Indeed, dev->data->name is the device name, looking like "net_tap#",
> with number increasing for each vdev.
> I'll put the following commit log line if that's ok:
> 
>      dev->data->name contains the device name, e.g. "net_tap0".
> 
>>
>>> dev->data->dev_private->name contains the actual iface name,
>>> e.g. "dtap0".
>>
>> Right, I agree this is correct comparing "dev->data->name"
>>
>> But the problem is pmd->name is per eth_dev.
>>
>> If I read code correct, for multiple queue support, each queue pair will
>> create a tap device, so each needs a different name.
>>
>> So can't just use pmd->name. Need to create a name per queue pair, it
>> can be combination of pmd->name + "_" + queue_id? Or can keep a name per
>> queue pair, instead of eth_dev.
>>
>> What do you think?
> 
> Actually that's not exactly how it goes.
> Adding a queue to a netdevice requires to open("/dev/net/tun") and setting
> TUNSETIFF (through ioctl) on the resulting fd.
> That's the important part: queues for a given tap device must set TUNSETIFF
> with a common ifreq.ifr_name (in our case, pmd->name).
> 
> This is best explained in the kernel doc, there:

Thank you for the clarification.
If so, why PMD keeps a fd per queue?

Overall, patch looks good except mentioned detail in commit log.

I suggest waiting Keith's patch, and rebase this set on top of it.

Thanks,
ferruh

> 
> [1] 
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/networking/tuntap.txt#n108
> 
<...>

Re: [dpdk-dev] [PATCH 1/6] net/tap: use correct tap name

[Pascal Mazon]

On 01/31/2017 04:28 PM, Ferruh Yigit wrote:
> On 1/31/2017 2:23 PM, Pascal Mazon wrote:
>> On 01/31/2017 02:06 PM, Ferruh Yigit wrote:> On 1/31/2017 9:42 AM,
>> Pascal Mazon wrote:
>>>> dev->data->name contains "net_tap", the device driver name.
>>>
>>> I see what patch does, just as a note to commit log:
>>>
>>> AFAIK, "dev->data->name" is device name, and for this case it is
>>> "net_tap#", like "net_tap0", "net_tap1" ...
>>>
>>> "dev->data_drv_name" is the driver name which is "net_tap"
>>
>> Indeed, dev->data->name is the device name, looking like "net_tap#",
>> with number increasing for each vdev.
>> I'll put the following commit log line if that's ok:
>>
>>      dev->data->name contains the device name, e.g. "net_tap0".
>>
>>>
>>>> dev->data->dev_private->name contains the actual iface name,
>>>> e.g. "dtap0".
>>>
>>> Right, I agree this is correct comparing "dev->data->name"
>>>
>>> But the problem is pmd->name is per eth_dev.
>>>
>>> If I read code correct, for multiple queue support, each queue pair will
>>> create a tap device, so each needs a different name.
>>>
>>> So can't just use pmd->name. Need to create a name per queue pair, it
>>> can be combination of pmd->name + "_" + queue_id? Or can keep a name per
>>> queue pair, instead of eth_dev.
>>>
>>> What do you think?
>>
>> Actually that's not exactly how it goes.
>> Adding a queue to a netdevice requires to open("/dev/net/tun") and setting
>> TUNSETIFF (through ioctl) on the resulting fd.
>> That's the important part: queues for a given tap device must set TUNSETIFF
>> with a common ifreq.ifr_name (in our case, pmd->name).
>>
>> This is best explained in the kernel doc, there:
>
> Thank you for the clarification.
> If so, why PMD keeps a fd per queue?
>
> Overall, patch looks good except mentioned detail in commit log.
>
> I suggest waiting Keith's patch, and rebase this set on top of it.
>
> Thanks,
> ferruh
>
>>
>> [1]
>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/networking/tuntap.txt#n108
>>
> <...>
>

I would say it is because dev->{r/t}x_pkt_burst() is done per queue.
In these functions, we need to make our read() and write() accesses on the
right fd considering the queue we're required to process.

I'll wait for Keith's patch, then.

Best regards,

-- 
Pascal Mazon
www.6wind.com

Re: [dpdk-dev] [PATCH 1/6] net/tap: use correct tap name

[Ferruh Yigit]

On 1/31/2017 3:30 PM, Pascal Mazon wrote:
> On 01/31/2017 04:28 PM, Ferruh Yigit wrote:
>> On 1/31/2017 2:23 PM, Pascal Mazon wrote:
>>> On 01/31/2017 02:06 PM, Ferruh Yigit wrote:> On 1/31/2017 9:42 AM,
>>> Pascal Mazon wrote:
>>>>> dev->data->name contains "net_tap", the device driver name.
>>>>
>>>> I see what patch does, just as a note to commit log:
>>>>
>>>> AFAIK, "dev->data->name" is device name, and for this case it is
>>>> "net_tap#", like "net_tap0", "net_tap1" ...
>>>>
>>>> "dev->data_drv_name" is the driver name which is "net_tap"
>>>
>>> Indeed, dev->data->name is the device name, looking like "net_tap#",
>>> with number increasing for each vdev.
>>> I'll put the following commit log line if that's ok:
>>>
>>>      dev->data->name contains the device name, e.g. "net_tap0".
>>>
>>>>
>>>>> dev->data->dev_private->name contains the actual iface name,
>>>>> e.g. "dtap0".
>>>>
>>>> Right, I agree this is correct comparing "dev->data->name"
>>>>
>>>> But the problem is pmd->name is per eth_dev.
>>>>
>>>> If I read code correct, for multiple queue support, each queue pair will
>>>> create a tap device, so each needs a different name.
>>>>
>>>> So can't just use pmd->name. Need to create a name per queue pair, it
>>>> can be combination of pmd->name + "_" + queue_id? Or can keep a name per
>>>> queue pair, instead of eth_dev.
>>>>
>>>> What do you think?
>>>
>>> Actually that's not exactly how it goes.
>>> Adding a queue to a netdevice requires to open("/dev/net/tun") and setting
>>> TUNSETIFF (through ioctl) on the resulting fd.
>>> That's the important part: queues for a given tap device must set TUNSETIFF
>>> with a common ifreq.ifr_name (in our case, pmd->name).
>>>
>>> This is best explained in the kernel doc, there:
>>
>> Thank you for the clarification.
>> If so, why PMD keeps a fd per queue?
>>
>> Overall, patch looks good except mentioned detail in commit log.
>>
>> I suggest waiting Keith's patch, and rebase this set on top of it.
>>
>> Thanks,
>> ferruh
>>
>>>
>>> [1]
>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/networking/tuntap.txt#n108
>>>
>> <...>
>>
> 
> I would say it is because dev->{r/t}x_pkt_burst() is done per queue.
> In these functions, we need to make our read() and write() accesses on the
> right fd considering the queue we're required to process.

If fd is same for all queues, it is possible to keep one instance in pmd
private_data, and access it from queues. I think it is confusing to have
multiple copy of it, but I see your point.

> 
> I'll wait for Keith's patch, then.

Thanks again.

> 
> Best regards,
> 

Re: [dpdk-dev] [PATCH 1/6] net/tap: use correct tap name

[Wiles, Keith]

> On Jan 31, 2017, at 9:38 AM, Yigit, Ferruh <ferruh.yigit@intel.com> wrote:
> 
> On 1/31/2017 3:30 PM, Pascal Mazon wrote:
>> On 01/31/2017 04:28 PM, Ferruh Yigit wrote:
>>> On 1/31/2017 2:23 PM, Pascal Mazon wrote:
>>>> On 01/31/2017 02:06 PM, Ferruh Yigit wrote:> On 1/31/2017 9:42 AM,
>>>> Pascal Mazon wrote:
>>>>>> dev->data->name contains "net_tap", the device driver name.
>>>>> 
>>>>> I see what patch does, just as a note to commit log:
>>>>> 
>>>>> AFAIK, "dev->data->name" is device name, and for this case it is
>>>>> "net_tap#", like "net_tap0", "net_tap1" ...
>>>>> 
>>>>> "dev->data_drv_name" is the driver name which is "net_tap"
>>>> 
>>>> Indeed, dev->data->name is the device name, looking like "net_tap#",
>>>> with number increasing for each vdev.
>>>> I'll put the following commit log line if that's ok:
>>>> 
>>>>     dev->data->name contains the device name, e.g. "net_tap0".
>>>> 
>>>>> 
>>>>>> dev->data->dev_private->name contains the actual iface name,
>>>>>> e.g. "dtap0".
>>>>> 
>>>>> Right, I agree this is correct comparing "dev->data->name"
>>>>> 
>>>>> But the problem is pmd->name is per eth_dev.
>>>>> 
>>>>> If I read code correct, for multiple queue support, each queue pair will
>>>>> create a tap device, so each needs a different name.
>>>>> 
>>>>> So can't just use pmd->name. Need to create a name per queue pair, it
>>>>> can be combination of pmd->name + "_" + queue_id? Or can keep a name per
>>>>> queue pair, instead of eth_dev.
>>>>> 
>>>>> What do you think?
>>>> 
>>>> Actually that's not exactly how it goes.
>>>> Adding a queue to a netdevice requires to open("/dev/net/tun") and setting
>>>> TUNSETIFF (through ioctl) on the resulting fd.
>>>> That's the important part: queues for a given tap device must set TUNSETIFF
>>>> with a common ifreq.ifr_name (in our case, pmd->name).
>>>> 
>>>> This is best explained in the kernel doc, there:
>>> 
>>> Thank you for the clarification.
>>> If so, why PMD keeps a fd per queue?
>>> 
>>> Overall, patch looks good except mentioned detail in commit log.
>>> 
>>> I suggest waiting Keith's patch, and rebase this set on top of it.
>>> 
>>> Thanks,
>>> ferruh
>>> 
>>>> 
>>>> [1]
>>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/networking/tuntap.txt#n108
>>>> 
>>> <...>
>>> 
>> 
>> I would say it is because dev->{r/t}x_pkt_burst() is done per queue.
>> In these functions, we need to make our read() and write() accesses on the
>> right fd considering the queue we're required to process.
> 
> If fd is same for all queues, it is possible to keep one instance in pmd
> private_data, and access it from queues. I think it is confusing to have
> multiple copy of it, but I see your point.

In my changes I removed the fds[] array as it was not required only the rx/tx_queue has an fd variable.

> 
>> 
>> I'll wait for Keith's patch, then.
> 
> Thanks again.
> 
>> 
>> Best regards,

Regards,
Keith

Re: [dpdk-dev] [PATCH 1/6] net/tap: use correct tap name

[Pascal Mazon]

On 01/31/2017 04:44 PM, Wiles, Keith wrote:
>
>> On Jan 31, 2017, at 9:38 AM, Yigit, Ferruh <ferruh.yigit@intel.com> wrote:
>>
>> On 1/31/2017 3:30 PM, Pascal Mazon wrote:
>>> On 01/31/2017 04:28 PM, Ferruh Yigit wrote:
>>>> On 1/31/2017 2:23 PM, Pascal Mazon wrote:
>>>>> On 01/31/2017 02:06 PM, Ferruh Yigit wrote:> On 1/31/2017 9:42 AM,
>>>>> Pascal Mazon wrote:
>>>>>>> dev->data->name contains "net_tap", the device driver name.
>>>>>>
>>>>>> I see what patch does, just as a note to commit log:
>>>>>>
>>>>>> AFAIK, "dev->data->name" is device name, and for this case it is
>>>>>> "net_tap#", like "net_tap0", "net_tap1" ...
>>>>>>
>>>>>> "dev->data_drv_name" is the driver name which is "net_tap"
>>>>>
>>>>> Indeed, dev->data->name is the device name, looking like "net_tap#",
>>>>> with number increasing for each vdev.
>>>>> I'll put the following commit log line if that's ok:
>>>>>
>>>>>     dev->data->name contains the device name, e.g. "net_tap0".
>>>>>
>>>>>>
>>>>>>> dev->data->dev_private->name contains the actual iface name,
>>>>>>> e.g. "dtap0".
>>>>>>
>>>>>> Right, I agree this is correct comparing "dev->data->name"
>>>>>>
>>>>>> But the problem is pmd->name is per eth_dev.
>>>>>>
>>>>>> If I read code correct, for multiple queue support, each queue pair will
>>>>>> create a tap device, so each needs a different name.
>>>>>>
>>>>>> So can't just use pmd->name. Need to create a name per queue pair, it
>>>>>> can be combination of pmd->name + "_" + queue_id? Or can keep a name per
>>>>>> queue pair, instead of eth_dev.
>>>>>>
>>>>>> What do you think?
>>>>>
>>>>> Actually that's not exactly how it goes.
>>>>> Adding a queue to a netdevice requires to open("/dev/net/tun") and setting
>>>>> TUNSETIFF (through ioctl) on the resulting fd.
>>>>> That's the important part: queues for a given tap device must set TUNSETIFF
>>>>> with a common ifreq.ifr_name (in our case, pmd->name).
>>>>>
>>>>> This is best explained in the kernel doc, there:
>>>>
>>>> Thank you for the clarification.
>>>> If so, why PMD keeps a fd per queue?
>>>>
>>>> Overall, patch looks good except mentioned detail in commit log.
>>>>
>>>> I suggest waiting Keith's patch, and rebase this set on top of it.
>>>>
>>>> Thanks,
>>>> ferruh
>>>>
>>>>>
>>>>> [1]
>>>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/networking/tuntap.txt#n108
>>>>>
>>>> <...>
>>>>
>>>
>>> I would say it is because dev->{r/t}x_pkt_burst() is done per queue.
>>> In these functions, we need to make our read() and write() accesses on the
>>> right fd considering the queue we're required to process.
>>
>> If fd is same for all queues, it is possible to keep one instance in pmd
>> private_data, and access it from queues. I think it is confusing to have
>> multiple copy of it, but I see your point.
>
> In my changes I removed the fds[] array as it was not required only the rx/tx_queue has an fd variable.

Sounds good to me.

Ferruh, just to make sure we're on the same page, in multiqueue, there 
is definitely a different fd for each queue.

>
>>
>>>
>>> I'll wait for Keith's patch, then.
>>
>> Thanks again.
>>
>>>
>>> Best regards,
>
> Regards,
> Keith
>


-- 
Pascal Mazon
www.6wind.com

Re: [dpdk-dev] [PATCH 1/6] net/tap: use correct tap name

[Wiles, Keith]

> On Jan 31, 2017, at 9:44 AM, Pascal Mazon <pascal.mazon@6wind.com> wrote:
> 
> On 01/31/2017 04:44 PM, Wiles, Keith wrote:
>> 
>>> On Jan 31, 2017, at 9:38 AM, Yigit, Ferruh <ferruh.yigit@intel.com> wrote:
>>> 
>>> On 1/31/2017 3:30 PM, Pascal Mazon wrote:
>>>> On 01/31/2017 04:28 PM, Ferruh Yigit wrote:
>>>>> On 1/31/2017 2:23 PM, Pascal Mazon wrote:
>>>>>> On 01/31/2017 02:06 PM, Ferruh Yigit wrote:> On 1/31/2017 9:42 AM,
>>>>>> Pascal Mazon wrote:
>>>>>>>> dev->data->name contains "net_tap", the device driver name.
>>>>>>> 
>>>>>>> I see what patch does, just as a note to commit log:
>>>>>>> 
>>>>>>> AFAIK, "dev->data->name" is device name, and for this case it is
>>>>>>> "net_tap#", like "net_tap0", "net_tap1" ...
>>>>>>> 
>>>>>>> "dev->data_drv_name" is the driver name which is "net_tap"
>>>>>> 
>>>>>> Indeed, dev->data->name is the device name, looking like "net_tap#",
>>>>>> with number increasing for each vdev.
>>>>>> I'll put the following commit log line if that's ok:
>>>>>> 
>>>>>>    dev->data->name contains the device name, e.g. "net_tap0".
>>>>>> 
>>>>>>> 
>>>>>>>> dev->data->dev_private->name contains the actual iface name,
>>>>>>>> e.g. "dtap0".
>>>>>>> 
>>>>>>> Right, I agree this is correct comparing "dev->data->name"
>>>>>>> 
>>>>>>> But the problem is pmd->name is per eth_dev.
>>>>>>> 
>>>>>>> If I read code correct, for multiple queue support, each queue pair will
>>>>>>> create a tap device, so each needs a different name.
>>>>>>> 
>>>>>>> So can't just use pmd->name. Need to create a name per queue pair, it
>>>>>>> can be combination of pmd->name + "_" + queue_id? Or can keep a name per
>>>>>>> queue pair, instead of eth_dev.
>>>>>>> 
>>>>>>> What do you think?
>>>>>> 
>>>>>> Actually that's not exactly how it goes.
>>>>>> Adding a queue to a netdevice requires to open("/dev/net/tun") and setting
>>>>>> TUNSETIFF (through ioctl) on the resulting fd.
>>>>>> That's the important part: queues for a given tap device must set TUNSETIFF
>>>>>> with a common ifreq.ifr_name (in our case, pmd->name).

Looking at the changes to set the link up/down and the adding the two functions. I noticed in the stop/start routines I was set the link in DPDK and not adjusting the interface link. Should the stop/start routine also do the same thing?

>>>>>> 
>>>>>> This is best explained in the kernel doc, there:
>>>>> 
>>>>> Thank you for the clarification.
>>>>> If so, why PMD keeps a fd per queue?
>>>>> 
>>>>> Overall, patch looks good except mentioned detail in commit log.
>>>>> 
>>>>> I suggest waiting Keith's patch, and rebase this set on top of it.
>>>>> 
>>>>> Thanks,
>>>>> ferruh
>>>>> 
>>>>>> 
>>>>>> [1]
>>>>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/networking/tuntap.txt#n108
>>>>>> 
>>>>> <...>
>>>>> 
>>>> 
>>>> I would say it is because dev->{r/t}x_pkt_burst() is done per queue.
>>>> In these functions, we need to make our read() and write() accesses on the
>>>> right fd considering the queue we're required to process.
>>> 
>>> If fd is same for all queues, it is possible to keep one instance in pmd
>>> private_data, and access it from queues. I think it is confusing to have
>>> multiple copy of it, but I see your point.
>> 
>> In my changes I removed the fds[] array as it was not required only the rx/tx_queue has an fd variable.
> 
> Sounds good to me.
> 
> Ferruh, just to make sure we're on the same page, in multiqueue, there is definitely a different fd for each queue.
> 
>> 
>>> 
>>>> 
>>>> I'll wait for Keith's patch, then.
>>> 
>>> Thanks again.
>>> 
>>>> 
>>>> Best regards,
>> 
>> Regards,
>> Keith
>> 
> 
> 
> -- 
> Pascal Mazon
> www.6wind.com

Regards,
Keith

Re: [dpdk-dev] [PATCH 1/6] net/tap: use correct tap name

[Pascal Mazon]

On 01/31/2017 05:06 PM, Wiles, Keith wrote:>
> Looking at the changes to set the link up/down and the adding the two functions. I noticed in the stop/start routines I was set the link in DPDK and not adjusting the interface link. Should the stop/start routine also do the same thing?
>

Well, my patch is probably wrong.
The best option would probably be to set dev->data->dev_link.link_status
appropriately inside tap_link_set() only.

I'm not sure it's compulsory to actually set the link UP in tap_dev_start()
(respectively DOWN in tap_dev_stop()).
If it is, however, it would be best done using tap_link_set() in those
functions.

--
Pascal Mazon
www.6wind.com

Re: [dpdk-dev] [PATCH 2/6] net/tap: use correct channel for error logs

[Stephen Hemminger]

On Tue, 31 Jan 2017 13:07:37 +0000
Ferruh Yigit <ferruh.yigit@intel.com> wrote:

> On 1/31/2017 9:42 AM, Pascal Mazon wrote:
> > Signed-off-by: Pascal Mazon <pascal.mazon@6wind.com>
> > ---  
> 
> looks good to me.
> 

Agreed, almost all uses of INFO as log level are wrong!
Either the message is reporting a problem should be NOTICE, WARN, ERR
or the message is often just developer checking the code and should be DEBUG.

Re: [dpdk-dev] [PATCH 2/6] net/tap: use correct channel for error logs

[Wiles, Keith]

> On Jan 31, 2017, at 10:58 AM, Stephen Hemminger <stephen@networkplumber.org> wrote:
> 
> On Tue, 31 Jan 2017 13:07:37 +0000
> Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> 
>> On 1/31/2017 9:42 AM, Pascal Mazon wrote:
>>> Signed-off-by: Pascal Mazon <pascal.mazon@6wind.com>
>>> ---  
>> 
>> looks good to me.
>> 
> 
> Agreed, almost all uses of INFO as log level are wrong!
> Either the message is reporting a problem should be NOTICE, WARN, ERR
> or the message is often just developer checking the code and should be DEBUG.

I had already updated most of the logs even before Pascal sent he patch.

Regards,
Keith

Re: [dpdk-dev] [PATCH 1/6] net/tap: use correct tap name

[Wiles, Keith]

> On Jan 31, 2017, at 10:39 AM, Pascal Mazon <pascal.mazon@6wind.com> wrote:
> 
> On 01/31/2017 05:06 PM, Wiles, Keith wrote:>
>> Looking at the changes to set the link up/down and the adding the two functions. I noticed in the stop/start routines I was set the link in DPDK and not adjusting the interface link. Should the stop/start routine also do the same thing?
>> 
> 
> Well, my patch is probably wrong.
> The best option would probably be to set dev->data->dev_link.link_status
> appropriately inside tap_link_set() only.
> 
> I'm not sure it's compulsory to actually set the link UP in tap_dev_start()
> (respectively DOWN in tap_dev_stop()).
> If it is, however, it would be best done using tap_link_set() in those
> functions.

I was setting the link up/down in both places in the old code. The gotta is link up/down came later (I guess) and applications only call start/stop. In the other drivers like ring the like they tend to set link in start/stop and in link up/down, which is what I patterned my driver on.

I looked around and the only applications calling link up/down was testpmd and ip_pipeline, but all of the apps call start/stop. Even the docs to not suggest that link up/down be used it appears start/stop must set the Link state and the developer can call link up/down APIs if needed for others reasons.

I assume the link up/down only effects the link state and the start/stop is creating/destroying resources.

My only solution I guess is to add the link up/down code to the start/stop API.

> 
> --
> Pascal Mazon
> www.6wind.com

Regards,
Keith

Re: [dpdk-dev] [PATCH 1/6] net/tap: use correct tap name

[Pascal Mazon]

On 02/01/2017 12:29 AM, Wiles, Keith wrote:
>
>> On Jan 31, 2017, at 10:39 AM, Pascal Mazon <pascal.mazon@6wind.com> wrote:
>>
>> On 01/31/2017 05:06 PM, Wiles, Keith wrote:>
>>> Looking at the changes to set the link up/down and the adding the two functions. I noticed in the stop/start routines I was set the link in DPDK and not adjusting the interface link. Should the stop/start routine also do the same thing?
>>>
>>
>> Well, my patch is probably wrong.
>> The best option would probably be to set dev->data->dev_link.link_status
>> appropriately inside tap_link_set() only.
>>
>> I'm not sure it's compulsory to actually set the link UP in tap_dev_start()
>> (respectively DOWN in tap_dev_stop()).
>> If it is, however, it would be best done using tap_link_set() in those
>> functions.
>
> I was setting the link up/down in both places in the old code. The gotta is link up/down came later (I guess) and applications only call start/stop. In the other drivers like ring the like they tend to set link in start/stop and in link up/down, which is what I patterned my driver on.
>
> I looked around and the only applications calling link up/down was testpmd and ip_pipeline, but all of the apps call start/stop. Even the docs to not suggest that link up/down be used it appears start/stop must set the Link state and the developer can call link up/down APIs if needed for others reasons.
>
> I assume the link up/down only effects the link state and the start/stop is creating/destroying resources.
>
> My only solution I guess is to add the link up/down code to the start/stop API.

I'm not sure I understand your conclusion.
If the apps usually call start/stop only, then definitely those 
functions should set the link state appropriately.
To that effect, I think it best to just call tap_link_set() in 
tap_dev_start() (and similar for stopping).
Apps with just start/stop functions would get the expected behavior, and 
the tap PMD would also support setting the link up/down independently, 
for testpmd and ip_pipeline for example.

Does that sound fine?


>
>>
>> --
>> Pascal Mazon
>> www.6wind.com
>
> Regards,
> Keith
>


-- 
Pascal Mazon
www.6wind.com

Re: [dpdk-dev] [PATCH 1/6] net/tap: use correct tap name

[Wiles, Keith]

> On Feb 1, 2017, at 2:11 AM, Pascal Mazon <pascal.mazon@6wind.com> wrote:
> 
> On 02/01/2017 12:29 AM, Wiles, Keith wrote:
>> 
>>> On Jan 31, 2017, at 10:39 AM, Pascal Mazon <pascal.mazon@6wind.com> wrote:
>>> 
>>> On 01/31/2017 05:06 PM, Wiles, Keith wrote:>
>>>> Looking at the changes to set the link up/down and the adding the two functions. I noticed in the stop/start routines I was set the link in DPDK and not adjusting the interface link. Should the stop/start routine also do the same thing?
>>>> 
>>> 
>>> Well, my patch is probably wrong.
>>> The best option would probably be to set dev->data->dev_link.link_status
>>> appropriately inside tap_link_set() only.
>>> 
>>> I'm not sure it's compulsory to actually set the link UP in tap_dev_start()
>>> (respectively DOWN in tap_dev_stop()).
>>> If it is, however, it would be best done using tap_link_set() in those
>>> functions.
>> 
>> I was setting the link up/down in both places in the old code. The gotta is link up/down came later (I guess) and applications only call start/stop. In the other drivers like ring the like they tend to set link in start/stop and in link up/down, which is what I patterned my driver on.
>> 
>> I looked around and the only applications calling link up/down was testpmd and ip_pipeline, but all of the apps call start/stop. Even the docs to not suggest that link up/down be used it appears start/stop must set the Link state and the developer can call link up/down APIs if needed for others reasons.
>> 
>> I assume the link up/down only effects the link state and the start/stop is creating/destroying resources.
>> 
>> My only solution I guess is to add the link up/down code to the start/stop API.
> 
> I'm not sure I understand your conclusion.
> If the apps usually call start/stop only, then definitely those functions should set the link state appropriately.
> To that effect, I think it best to just call tap_link_set() in tap_dev_start() (and similar for stopping).
> Apps with just start/stop functions would get the expected behavior, and the tap PMD would also support setting the link up/down independently, for testpmd and ip_pipeline for example.
> 
> Does that sound fine?

Yes, this was what I was trying to say and calling tap_link_set() in tap_dev_start() is the solution.

> 
> 
>> 
>>> 
>>> --
>>> Pascal Mazon
>>> www.6wind.com
>> 
>> Regards,
>> Keith
>> 
> 
> 
> -- 
> Pascal Mazon
> www.6wind.com

Regards,
Keith

Re: [dpdk-dev] [PATCH 1/6] net/tap: use correct tap name

[Pascal Mazon]

On 02/01/2017 04:25 PM, Wiles, Keith wrote:
>
>> On Feb 1, 2017, at 2:11 AM, Pascal Mazon <pascal.mazon@6wind.com> wrote:
>>
>> On 02/01/2017 12:29 AM, Wiles, Keith wrote:
>>>
>>>> On Jan 31, 2017, at 10:39 AM, Pascal Mazon <pascal.mazon@6wind.com> wrote:
>>>>
>>>> On 01/31/2017 05:06 PM, Wiles, Keith wrote:>
>>>>> Looking at the changes to set the link up/down and the adding the two functions. I noticed in the stop/start routines I was set the link in DPDK and not adjusting the interface link. Should the stop/start routine also do the same thing?
>>>>>
>>>>
>>>> Well, my patch is probably wrong.
>>>> The best option would probably be to set dev->data->dev_link.link_status
>>>> appropriately inside tap_link_set() only.
>>>>
>>>> I'm not sure it's compulsory to actually set the link UP in tap_dev_start()
>>>> (respectively DOWN in tap_dev_stop()).
>>>> If it is, however, it would be best done using tap_link_set() in those
>>>> functions.
>>>
>>> I was setting the link up/down in both places in the old code. The gotta is link up/down came later (I guess) and applications only call start/stop. In the other drivers like ring the like they tend to set link in start/stop and in link up/down, which is what I patterned my driver on.
>>>
>>> I looked around and the only applications calling link up/down was testpmd and ip_pipeline, but all of the apps call start/stop. Even the docs to not suggest that link up/down be used it appears start/stop must set the Link state and the developer can call link up/down APIs if needed for others reasons.
>>>
>>> I assume the link up/down only effects the link state and the start/stop is creating/destroying resources.
>>>
>>> My only solution I guess is to add the link up/down code to the start/stop API.
>>
>> I'm not sure I understand your conclusion.
>> If the apps usually call start/stop only, then definitely those functions should set the link state appropriately.
>> To that effect, I think it best to just call tap_link_set() in tap_dev_start() (and similar for stopping).
>> Apps with just start/stop functions would get the expected behavior, and the tap PMD would also support setting the link up/down independently, for testpmd and ip_pipeline for example.
>>
>> Does that sound fine?
>
> Yes, this was what I was trying to say and calling tap_link_set() in tap_dev_start() is the solution.

Great, that looks good to me!

Regards,
Pascal

Re: [dpdk-dev] [PATCH 1/6] net/tap: use correct tap name

[Wiles, Keith]

> On Feb 1, 2017, at 9:40 AM, Pascal Mazon <pascal.mazon@6wind.com> wrote:
> 
> On 02/01/2017 04:25 PM, Wiles, Keith wrote:
>> 
>>> On Feb 1, 2017, at 2:11 AM, Pascal Mazon <pascal.mazon@6wind.com> wrote:
>>> 
>>> On 02/01/2017 12:29 AM, Wiles, Keith wrote:
>>>> 
>>>>> On Jan 31, 2017, at 10:39 AM, Pascal Mazon <pascal.mazon@6wind.com> wrote:
>>>>> 
>>>>> On 01/31/2017 05:06 PM, Wiles, Keith wrote:>
>>>>>> Looking at the changes to set the link up/down and the adding the two functions. I noticed in the stop/start routines I was set the link in DPDK and not adjusting the interface link. Should the stop/start routine also do the same thing?
>>>>>> 
>>>>> 
>>>>> Well, my patch is probably wrong.
>>>>> The best option would probably be to set dev->data->dev_link.link_status
>>>>> appropriately inside tap_link_set() only.
>>>>> 
>>>>> I'm not sure it's compulsory to actually set the link UP in tap_dev_start()
>>>>> (respectively DOWN in tap_dev_stop()).
>>>>> If it is, however, it would be best done using tap_link_set() in those
>>>>> functions.
>>>> 
>>>> I was setting the link up/down in both places in the old code. The gotta is link up/down came later (I guess) and applications only call start/stop. In the other drivers like ring the like they tend to set link in start/stop and in link up/down, which is what I patterned my driver on.
>>>> 
>>>> I looked around and the only applications calling link up/down was testpmd and ip_pipeline, but all of the apps call start/stop. Even the docs to not suggest that link up/down be used it appears start/stop must set the Link state and the developer can call link up/down APIs if needed for others reasons.
>>>> 
>>>> I assume the link up/down only effects the link state and the start/stop is creating/destroying resources.
>>>> 
>>>> My only solution I guess is to add the link up/down code to the start/stop API.
>>> 
>>> I'm not sure I understand your conclusion.
>>> If the apps usually call start/stop only, then definitely those functions should set the link state appropriately.
>>> To that effect, I think it best to just call tap_link_set() in tap_dev_start() (and similar for stopping).
>>> Apps with just start/stop functions would get the expected behavior, and the tap PMD would also support setting the link up/down independently, for testpmd and ip_pipeline for example.
>>> 
>>> Does that sound fine?
>> 
>> Yes, this was what I was trying to say and calling tap_link_set() in tap_dev_start() is the solution.
> 
> Great, that looks good to me!

Ferruh, Please apply Pascal’s 6 patches and I will based my changes on top of those changes. Does that sound reasonable?

> 
> Regards,
> Pascal

Regards,
Keith

Re: [dpdk-dev] [PATCH 1/6] net/tap: use correct tap name

[Ferruh Yigit]

On 2/1/2017 3:55 PM, Wiles, Keith wrote:
> 
>> On Feb 1, 2017, at 9:40 AM, Pascal Mazon <pascal.mazon@6wind.com> wrote:
>>
>> On 02/01/2017 04:25 PM, Wiles, Keith wrote:
>>>
>>>> On Feb 1, 2017, at 2:11 AM, Pascal Mazon <pascal.mazon@6wind.com> wrote:
>>>>
>>>> On 02/01/2017 12:29 AM, Wiles, Keith wrote:
>>>>>
>>>>>> On Jan 31, 2017, at 10:39 AM, Pascal Mazon <pascal.mazon@6wind.com> wrote:
>>>>>>
>>>>>> On 01/31/2017 05:06 PM, Wiles, Keith wrote:>
>>>>>>> Looking at the changes to set the link up/down and the adding the two functions. I noticed in the stop/start routines I was set the link in DPDK and not adjusting the interface link. Should the stop/start routine also do the same thing?
>>>>>>>
>>>>>>
>>>>>> Well, my patch is probably wrong.
>>>>>> The best option would probably be to set dev->data->dev_link.link_status
>>>>>> appropriately inside tap_link_set() only.
>>>>>>
>>>>>> I'm not sure it's compulsory to actually set the link UP in tap_dev_start()
>>>>>> (respectively DOWN in tap_dev_stop()).
>>>>>> If it is, however, it would be best done using tap_link_set() in those
>>>>>> functions.
>>>>>
>>>>> I was setting the link up/down in both places in the old code. The gotta is link up/down came later (I guess) and applications only call start/stop. In the other drivers like ring the like they tend to set link in start/stop and in link up/down, which is what I patterned my driver on.
>>>>>
>>>>> I looked around and the only applications calling link up/down was testpmd and ip_pipeline, but all of the apps call start/stop. Even the docs to not suggest that link up/down be used it appears start/stop must set the Link state and the developer can call link up/down APIs if needed for others reasons.
>>>>>
>>>>> I assume the link up/down only effects the link state and the start/stop is creating/destroying resources.
>>>>>
>>>>> My only solution I guess is to add the link up/down code to the start/stop API.
>>>>
>>>> I'm not sure I understand your conclusion.
>>>> If the apps usually call start/stop only, then definitely those functions should set the link state appropriately.
>>>> To that effect, I think it best to just call tap_link_set() in tap_dev_start() (and similar for stopping).
>>>> Apps with just start/stop functions would get the expected behavior, and the tap PMD would also support setting the link up/down independently, for testpmd and ip_pipeline for example.
>>>>
>>>> Does that sound fine?
>>>
>>> Yes, this was what I was trying to say and calling tap_link_set() in tap_dev_start() is the solution.
>>
>> Great, that looks good to me!
> 
> Ferruh, Please apply Pascal’s 6 patches and I will based my changes on top of those changes. Does that sound reasonable?

That is good, I will.

Pascal,

Only patch 1/6 commit log needs reworking, rest looks good, (although I
still will do one more round of basic tests). For commit log update, do
you want to send a v2 or prefer me do the update?

Thanks,
ferruh

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

Re: [dpdk-dev] [PATCH 1/6] net/tap: use correct tap name

[Pascal Mazon]

On 02/01/2017 06:50 PM, Ferruh Yigit wrote:
> On 2/1/2017 3:55 PM, Wiles, Keith wrote:
>>
>>> On Feb 1, 2017, at 9:40 AM, Pascal Mazon <pascal.mazon@6wind.com> wrote:
>>>
>>> On 02/01/2017 04:25 PM, Wiles, Keith wrote:
>>>>
>>>>> On Feb 1, 2017, at 2:11 AM, Pascal Mazon <pascal.mazon@6wind.com> wrote:
>>>>>
>>>>> On 02/01/2017 12:29 AM, Wiles, Keith wrote:
>>>>>>
>>>>>>> On Jan 31, 2017, at 10:39 AM, Pascal Mazon <pascal.mazon@6wind.com> wrote:
>>>>>>>
>>>>>>> On 01/31/2017 05:06 PM, Wiles, Keith wrote:>
>>>>>>>> Looking at the changes to set the link up/down and the adding the two functions. I noticed in the stop/start routines I was set the link in DPDK and not adjusting the interface link. Should the stop/start routine also do the same thing?
>>>>>>>>
>>>>>>>
>>>>>>> Well, my patch is probably wrong.
>>>>>>> The best option would probably be to set dev->data->dev_link.link_status
>>>>>>> appropriately inside tap_link_set() only.
>>>>>>>
>>>>>>> I'm not sure it's compulsory to actually set the link UP in tap_dev_start()
>>>>>>> (respectively DOWN in tap_dev_stop()).
>>>>>>> If it is, however, it would be best done using tap_link_set() in those
>>>>>>> functions.
>>>>>>
>>>>>> I was setting the link up/down in both places in the old code. The gotta is link up/down came later (I guess) and applications only call start/stop. In the other drivers like ring the like they tend to set link in start/stop and in link up/down, which is what I patterned my driver on.
>>>>>>
>>>>>> I looked around and the only applications calling link up/down was testpmd and ip_pipeline, but all of the apps call start/stop. Even the docs to not suggest that link up/down be used it appears start/stop must set the Link state and the developer can call link up/down APIs if needed for others reasons.
>>>>>>
>>>>>> I assume the link up/down only effects the link state and the start/stop is creating/destroying resources.
>>>>>>
>>>>>> My only solution I guess is to add the link up/down code to the start/stop API.
>>>>>
>>>>> I'm not sure I understand your conclusion.
>>>>> If the apps usually call start/stop only, then definitely those functions should set the link state appropriately.
>>>>> To that effect, I think it best to just call tap_link_set() in tap_dev_start() (and similar for stopping).
>>>>> Apps with just start/stop functions would get the expected behavior, and the tap PMD would also support setting the link up/down independently, for testpmd and ip_pipeline for example.
>>>>>
>>>>> Does that sound fine?
>>>>
>>>> Yes, this was what I was trying to say and calling tap_link_set() in tap_dev_start() is the solution.
>>>
>>> Great, that looks good to me!
>>
>> Ferruh, Please apply Pascal’s 6 patches and I will based my changes on top of those changes. Does that sound reasonable?
>
> That is good, I will.
>
> Pascal,
>
> Only patch 1/6 commit log needs reworking, rest looks good, (although I
> still will do one more round of basic tests). For commit log update, do
> you want to send a v2 or prefer me do the update?

I'd prefer if you did the update. Thank you!

Pascal

>
> Thanks,
> ferruh
>
>>
>>>
>>> Regards,
>>> Pascal
>>
>> Regards,
>> Keith
>>
>

Re: [dpdk-dev] [PATCH 1/6] net/tap: use correct tap name

[Pascal Mazon]

On Thu, Feb 2, 2017 at 9:05 AM, Pascal Mazon <pascal.mazon@6wind.com> wrote:

> On 02/01/2017 06:50 PM, Ferruh Yigit wrote:
>
>> On 2/1/2017 3:55 PM, Wiles, Keith wrote:
>>
>>>
>>> On Feb 1, 2017, at 9:40 AM, Pascal Mazon <pascal.mazon@6wind.com> wrote:
>>>>
>>>> On 02/01/2017 04:25 PM, Wiles, Keith wrote:
>>>>
>>>>>
>>>>> On Feb 1, 2017, at 2:11 AM, Pascal Mazon <pascal.mazon@6wind.com>
>>>>>> wrote:
>>>>>>
>>>>>> On 02/01/2017 12:29 AM, Wiles, Keith wrote:
>>>>>>
>>>>>>>
>>>>>>> On Jan 31, 2017, at 10:39 AM, Pascal Mazon <pascal.mazon@6wind.com>
>>>>>>>> wrote:
>>>>>>>>
>>>>>>>> On 01/31/2017 05:06 PM, Wiles, Keith wrote:>
>>>>>>>>
>>>>>>>>> Looking at the changes to set the link up/down and the adding the
>>>>>>>>> two functions. I noticed in the stop/start routines I was set the link in
>>>>>>>>> DPDK and not adjusting the interface link. Should the stop/start routine
>>>>>>>>> also do the same thing?
>>>>>>>>>
>>>>>>>>>
>>>>>>>> Well, my patch is probably wrong.
>>>>>>>> The best option would probably be to set
>>>>>>>> dev->data->dev_link.link_status
>>>>>>>> appropriately inside tap_link_set() only.
>>>>>>>>
>>>>>>>> I'm not sure it's compulsory to actually set the link UP in
>>>>>>>> tap_dev_start()
>>>>>>>> (respectively DOWN in tap_dev_stop()).
>>>>>>>> If it is, however, it would be best done using tap_link_set() in
>>>>>>>> those
>>>>>>>> functions.
>>>>>>>>
>>>>>>>
>>>>>>> I was setting the link up/down in both places in the old code. The
>>>>>>> gotta is link up/down came later (I guess) and applications only call
>>>>>>> start/stop. In the other drivers like ring the like they tend to set link
>>>>>>> in start/stop and in link up/down, which is what I patterned my driver on.
>>>>>>>
>>>>>>> I looked around and the only applications calling link up/down was
>>>>>>> testpmd and ip_pipeline, but all of the apps call start/stop. Even the docs
>>>>>>> to not suggest that link up/down be used it appears start/stop must set the
>>>>>>> Link state and the developer can call link up/down APIs if needed for
>>>>>>> others reasons.
>>>>>>>
>>>>>>> I assume the link up/down only effects the link state and the
>>>>>>> start/stop is creating/destroying resources.
>>>>>>>
>>>>>>> My only solution I guess is to add the link up/down code to the
>>>>>>> start/stop API.
>>>>>>>
>>>>>>
>>>>>> I'm not sure I understand your conclusion.
>>>>>> If the apps usually call start/stop only, then definitely those
>>>>>> functions should set the link state appropriately.
>>>>>> To that effect, I think it best to just call tap_link_set() in
>>>>>> tap_dev_start() (and similar for stopping).
>>>>>> Apps with just start/stop functions would get the expected behavior,
>>>>>> and the tap PMD would also support setting the link up/down independently,
>>>>>> for testpmd and ip_pipeline for example.
>>>>>>
>>>>>> Does that sound fine?
>>>>>>
>>>>>
>>>>> Yes, this was what I was trying to say and calling tap_link_set() in
>>>>> tap_dev_start() is the solution.
>>>>>
>>>>
>>>> Great, that looks good to me!
>>>>
>>>
>>> Ferruh, Please apply Pascal’s 6 patches and I will based my changes on
>>> top of those changes. Does that sound reasonable?
>>>
>>
>> That is good, I will.
>>
>> Pascal,
>>
>> Only patch 1/6 commit log needs reworking, rest looks good, (although I
>> still will do one more round of basic tests). For commit log update, do
>> you want to send a v2 or prefer me do the update?
>>
>
> I'd prefer if you did the update. Thank you!
>
> Pascal


Actually, I'm working on a patch to implement promiscuous_enable/disable
and allmulticast_enable/disable.
I'll have to change the tap_link_set() to be more generic and support the
appropriate flags.
So I'll send a v2 of my patches with patch 1/6 commit-log updated, and the
promisc allmulti support, later today.

Forget my previous email, please.

Regards,
Pascal


>
>
>
>> Thanks,
>> ferruh
>>
>>
>>>
>>>> Regards,
>>>> Pascal
>>>>
>>>
>>> Regards,
>>> Keith
>>>
>>>

Re: [dpdk-dev] [PATCH 1/6] net/tap: use correct tap name

[Ferruh Yigit]

On 2/2/2017 8:25 AM, Pascal Mazon wrote:
<...>
> 
> Actually, I'm working on a patch to implement promiscuous_enable/disable
> and allmulticast_enable/disable.
> I'll have to change the tap_link_set() to be more generic and support
> the appropriate flags.
> So I'll send a v2 of my patches with patch 1/6 commit-log updated, and
> the promisc allmulti support, later today.

OK, thanks.

Re: [dpdk-dev] [PATCH 1/6] net/tap: use correct tap name

[Wiles, Keith]

> On Jan 31, 2017, at 3:42 AM, Pascal Mazon <pascal.mazon@6wind.com> wrote:
> 
> dev->data->name contains "net_tap", the device driver name.
> dev->data->dev_private->name contains the actual iface name,
> e.g. "dtap0".
> 
> In tun_alloc() especially, we want to use the latter. Otherwise the
> netdevice would be wrongly named "net_tap". Furthermore, creating
> several tap vdev would point to the same netdevice.
> 
> In any case, it must to be consistent with the tun_alloc() call in
> eth_dev_tap_create().
> 
> Signed-off-by: Pascal Mazon <pascal.mazon@6wind.com>
> —

Acked by Keith Wiles <keith.wiles@intel.com> for the series 1-6

Regards,
Keith

[dpdk-dev] [PATCH v2 1/7] net/tap: use correct tap name

[Pascal Mazon]

dev->data->name contains the device name, e.g. "net_tap0".
dev->data->dev_private->name contains the actual iface name,
e.g. "dtap0".

In any case, the name must to be consistent with the tun_alloc() call in
eth_dev_tap_create().

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

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index 91f63f5468b2..8faf08551b9e 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -410,6 +410,7 @@ tap_setup_queue(struct rte_eth_dev *dev,
 		struct pmd_internals *internals,
 		uint16_t qid)
 {
+	struct pmd_internals *pmd = dev->data->dev_private;
 	struct rx_queue *rx = &internals->rxq[qid];
 	struct tx_queue *tx = &internals->txq[qid];
 	int fd;
@@ -419,11 +420,10 @@ tap_setup_queue(struct rte_eth_dev *dev,
 		fd = tx->fd;
 		if (fd < 0) {
 			RTE_LOG(INFO, PMD, "Add queue to TAP %s for qid %d\n",
-				dev->data->name, qid);
-			fd = tun_alloc(dev->data->name);
+				pmd->name, qid);
+			fd = tun_alloc(pmd->name);
 			if (fd < 0) {
-				RTE_LOG(ERR, PMD, "tun_alloc(%s) failed\n",
-					dev->data->name);
+				RTE_LOG(ERR, PMD, "tun_alloc(%s) failed\n", pmd->name);
 				return -1;
 			}
 		}
@@ -493,7 +493,7 @@ tap_rx_queue_setup(struct rte_eth_dev *dev,
 
 	internals->fds[rx_queue_id] = fd;
 	RTE_LOG(INFO, PMD, "RX TAP device name %s, qid %d on fd %d\n",
-		dev->data->name, rx_queue_id, internals->rxq[rx_queue_id].fd);
+		internals->name, rx_queue_id, internals->rxq[rx_queue_id].fd);
 
 	return 0;
 }
@@ -516,7 +516,7 @@ tap_tx_queue_setup(struct rte_eth_dev *dev,
 		return -1;
 
 	RTE_LOG(INFO, PMD, "TX TAP device name %s, qid %d on fd %d\n",
-		dev->data->name, tx_queue_id, internals->txq[tx_queue_id].fd);
+		internals->name, tx_queue_id, internals->txq[tx_queue_id].fd);
 
 	return 0;
 }
-- 
2.8.0.rc0

[dpdk-dev] [PATCH v2 2/7] net/tap: use correct channel for error logs

[Pascal Mazon]

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

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index 8faf08551b9e..4cc1767da5e8 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -594,20 +594,20 @@ eth_dev_tap_create(const char *name, char *tap_name)
 
 	data = rte_zmalloc_socket(tap_name, sizeof(*data), 0, numa_node);
 	if (!data) {
-		RTE_LOG(INFO, PMD, "Failed to allocate data\n");
+		RTE_LOG(ERR, PMD, "Failed to allocate data\n");
 		goto error_exit;
 	}
 
 	pmd = rte_zmalloc_socket(tap_name, sizeof(*pmd), 0, numa_node);
 	if (!pmd) {
-		RTE_LOG(INFO, PMD, "Unable to allocate internal struct\n");
+		RTE_LOG(ERR, PMD, "Unable to allocate internal struct\n");
 		goto error_exit;
 	}
 
 	/* Use the name and not the tap_name */
 	dev = rte_eth_dev_allocate(tap_name);
 	if (!dev) {
-		RTE_LOG(INFO, PMD, "Unable to allocate device struct\n");
+		RTE_LOG(ERR, PMD, "Unable to allocate device struct\n");
 		goto error_exit;
 	}
 
@@ -638,7 +638,7 @@ eth_dev_tap_create(const char *name, char *tap_name)
 	/* Create the first Tap device */
 	fd = tun_alloc(tap_name);
 	if (fd < 0) {
-		RTE_LOG(INFO, PMD, "tun_alloc() failed\n");
+		RTE_LOG(ERR, PMD, "tun_alloc() failed\n");
 		goto error_exit;
 	}
 
@@ -655,7 +655,7 @@ eth_dev_tap_create(const char *name, char *tap_name)
 	pmd->fds[0] = fd;
 
 	if (pmd_mac_address(fd, dev, &pmd->eth_addr) < 0) {
-		RTE_LOG(INFO, PMD, "Unable to get MAC address\n");
+		RTE_LOG(ERR, PMD, "Unable to get MAC address\n");
 		goto error_exit;
 	}
 
-- 
2.8.0.rc0

[dpdk-dev] [PATCH v2 3/7] net/tap: don't set fd value overwritten just below

[Pascal Mazon]

pmd->fds[0], pmd->rxq[0] and pmd->txq[0] are set a couple of lines after
the for loop that initializes them to -1.

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 4cc1767da5e8..3d15031008c6 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -643,7 +643,7 @@ eth_dev_tap_create(const char *name, char *tap_name)
 	}
 
 	/* Presetup the fds to -1 as being not working */
-	for (i = 0; i < RTE_PMD_TAP_MAX_QUEUES; i++) {
+	for (i = 1; i < RTE_PMD_TAP_MAX_QUEUES; i++) {
 		pmd->fds[i] = -1;
 		pmd->rxq[i].fd = -1;
 		pmd->txq[i].fd = -1;
-- 
2.8.0.rc0

[dpdk-dev] [PATCH v2 4/7] net/tap: keep kernel-assigned MAC address

[Pascal Mazon]

There's no point in having a different internal MAC address than the one
provided by the kernel when creating the netdevice.

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

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index 3d15031008c6..f8b07b4a8fa1 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -552,28 +552,6 @@ pmd_mac_address(int fd, struct rte_eth_dev *dev, struct ether_addr *addr)
 		return -1;
 	}
 
-	/* Set the host based MAC address to this special MAC format */
-	ifr.ifr_hwaddr.sa_data[0] = 'T';
-	ifr.ifr_hwaddr.sa_data[1] = 'a';
-	ifr.ifr_hwaddr.sa_data[2] = 'p';
-	ifr.ifr_hwaddr.sa_data[3] = '-';
-	ifr.ifr_hwaddr.sa_data[4] = dev->data->port_id;
-	ifr.ifr_hwaddr.sa_data[5] = dev->data->numa_node;
-	if (ioctl(fd, SIOCSIFHWADDR, &ifr) == -1) {
-		RTE_LOG(ERR, PMD, "%s: ioctl failed (SIOCSIFHWADDR) (%s)\n",
-			dev->data->name, ifr.ifr_name);
-		return -1;
-	}
-
-	/* Set the local application MAC address, needs to be different then
-	 * the host based MAC address.
-	 */
-	ifr.ifr_hwaddr.sa_data[0] = 'd';
-	ifr.ifr_hwaddr.sa_data[1] = 'n';
-	ifr.ifr_hwaddr.sa_data[2] = 'e';
-	ifr.ifr_hwaddr.sa_data[3] = 't';
-	ifr.ifr_hwaddr.sa_data[4] = dev->data->port_id;
-	ifr.ifr_hwaddr.sa_data[5] = dev->data->numa_node;
 	rte_memcpy(addr, ifr.ifr_hwaddr.sa_data, ETH_ALEN);
 
 	return 0;
-- 
2.8.0.rc0

[dpdk-dev] [PATCH v2 5/7] net/tap: display tap name after parsing

[Pascal Mazon]

The probe parses for user-defined iface name. Let's use that value.

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

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index f8b07b4a8fa1..734e3a579219 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -690,9 +690,6 @@ rte_pmd_tap_probe(const char *name, const char *params)
 	snprintf(tap_name, sizeof(tap_name), "%s%d",
 		 DEFAULT_TAP_NAME, tap_unit++);
 
-	RTE_LOG(INFO, PMD, "Initializing pmd_tap for %s as %s\n",
-		name, tap_name);
-
 	if (params && (params[0] != '\0')) {
 		RTE_LOG(INFO, PMD, "paramaters (%s)\n", params);
 
@@ -719,6 +716,9 @@ rte_pmd_tap_probe(const char *name, const char *params)
 	}
 	pmd_link.link_speed = speed;
 
+	RTE_LOG(INFO, PMD, "Initializing pmd_tap for %s as %s\n",
+		name, tap_name);
+
 	ret = eth_dev_tap_create(name, tap_name);
 
 leave:
-- 
2.8.0.rc0

[dpdk-dev] [PATCH v2 6/7] net/tap: implement link up and down callbacks

[Pascal Mazon]

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

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index 734e3a579219..275af9df2252 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -275,13 +275,69 @@ pmd_tx_burst(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 	return num_tx;
 }
 
+static int tap_link_set_flags(struct pmd_internals *pmd, short flags, int add)
+{
+	struct ifreq ifr;
+	int err, s;
+
+	/*
+	 * An AF_INET/DGRAM socket is needed for
+	 * SIOCGIFFLAGS/SIOCSIFFLAGS, using fd won't work.
+	 */
+	s = socket(AF_INET, SOCK_DGRAM, 0);
+	if (s < 0) {
+		RTE_LOG(ERR, PMD,
+			"Unable to get a socket to set flags: %s\n",
+			strerror(errno));
+		return -1;
+	}
+	memset(&ifr, 0, sizeof(ifr));
+	strncpy(ifr.ifr_name, pmd->name, IFNAMSIZ);
+	err = ioctl(s, SIOCGIFFLAGS, &ifr);
+	if (err < 0) {
+		RTE_LOG(ERR, PMD, "Unable to get tap netdevice flags: %s\n",
+			strerror(errno));
+		close(s);
+		return -1;
+	}
+	if (add)
+		ifr.ifr_flags |= flags;
+	else
+		ifr.ifr_flags &= ~flags;
+	err = ioctl(s, SIOCSIFFLAGS, &ifr);
+	if (err < 0) {
+		RTE_LOG(ERR, PMD, "Unable to %s flags 0x%x: %s\n",
+			add ? "set" : "unset", flags, strerror(errno));
+		close(s);
+		return -1;
+	}
+	close(s);
+
+	return 0;
+}
+
 static int
-tap_dev_start(struct rte_eth_dev *dev)
+tap_link_set_down(struct rte_eth_dev *dev)
 {
-	/* Force the Link up */
+	struct pmd_internals *pmd = dev->data->dev_private;
+
+	dev->data->dev_link.link_status = ETH_LINK_DOWN;
+	return tap_link_set_flags(pmd, IFF_UP | IFF_NOARP, 0);
+}
+
+static int
+tap_link_set_up(struct rte_eth_dev *dev)
+{
+	struct pmd_internals *pmd = dev->data->dev_private;
+
 	dev->data->dev_link.link_status = ETH_LINK_UP;
+	return tap_link_set_flags(pmd, IFF_UP | IFF_NOARP, 1);
+}
 
-	return 0;
+static int
+tap_dev_start(struct rte_eth_dev *dev)
+{
+	return tap_link_set_up(dev);
 }
 
 /* This function gets called when the current port gets stopped.
@@ -295,8 +351,7 @@ tap_dev_stop(struct rte_eth_dev *dev)
 	for (i = 0; i < internals->nb_queues; i++)
 		if (internals->fds[i] != -1)
 			close(internals->fds[i]);
-
-	dev->data->dev_link.link_status = ETH_LINK_DOWN;
+	tap_link_set_down(dev);
 }
 
 static int
@@ -532,6 +587,8 @@ static const struct eth_dev_ops ops = {
 	.rx_queue_release       = tap_rx_queue_release,
 	.tx_queue_release       = tap_tx_queue_release,
 	.link_update            = tap_link_update,
+	.dev_set_link_up        = tap_link_set_up,
+	.dev_set_link_down      = tap_link_set_down,
 	.stats_get              = tap_stats_get,
 	.stats_reset            = tap_stats_reset,
 };
-- 
2.8.0.rc0

[dpdk-dev] [PATCH v2 7/7] net/tap: support promiscuous and allmulti setting

[Pascal Mazon]

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

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index 275af9df2252..3f179c3dfb3c 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -460,6 +460,42 @@ tap_link_update(struct rte_eth_dev *dev __rte_unused,
 	return 0;
 }
 
+static void
+tap_promisc_enable(struct rte_eth_dev *dev)
+{
+	struct pmd_internals *pmd = dev->data->dev_private;
+
+	dev->data->promiscuous = 1;
+	tap_link_set_flags(pmd, IFF_PROMISC, 1);
+}
+
+static void
+tap_promisc_disable(struct rte_eth_dev *dev)
+{
+	struct pmd_internals *pmd = dev->data->dev_private;
+
+	dev->data->promiscuous = 0;
+	tap_link_set_flags(pmd, IFF_PROMISC, 0);
+}
+
+static void
+tap_allmulti_enable(struct rte_eth_dev *dev)
+{
+	struct pmd_internals *pmd = dev->data->dev_private;
+
+	dev->data->all_multicast = 1;
+	tap_link_set_flags(pmd, IFF_ALLMULTI, 1);
+}
+
+static void
+tap_allmulti_disable(struct rte_eth_dev *dev)
+{
+	struct pmd_internals *pmd = dev->data->dev_private;
+
+	dev->data->all_multicast = 0;
+	tap_link_set_flags(pmd, IFF_ALLMULTI, 0);
+}
+
 static int
 tap_setup_queue(struct rte_eth_dev *dev,
 		struct pmd_internals *internals,
@@ -589,6 +625,10 @@ static const struct eth_dev_ops ops = {
 	.link_update            = tap_link_update,
 	.dev_set_link_up        = tap_link_set_up,
 	.dev_set_link_down      = tap_link_set_down,
+	.promiscuous_enable     = tap_promisc_enable,
+	.promiscuous_disable    = tap_promisc_disable,
+	.allmulticast_enable    = tap_allmulti_enable,
+	.allmulticast_disable   = tap_allmulti_disable,
 	.stats_get              = tap_stats_get,
 	.stats_reset            = tap_stats_reset,
 };
-- 
2.8.0.rc0

Re: [dpdk-dev] [PATCH v2 1/7] net/tap: use correct tap name

[Wiles, Keith]

> On Feb 2, 2017, at 10:17 AM, Pascal Mazon <pascal.mazon@6wind.com> wrote:
> 
> dev->data->name contains the device name, e.g. "net_tap0".
> dev->data->dev_private->name contains the actual iface name,
> e.g. "dtap0".
> 
> In any case, the name must to be consistent with the tun_alloc() call in
> eth_dev_tap_create().
> 
> Signed-off-by: Pascal Mazon <pascal.mazon@6wind.com>
> —

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

Regards,
Keith

Re: [dpdk-dev] [PATCH v2 1/7] net/tap: use correct tap name

[Wiles, Keith]

> On Feb 2, 2017, at 10:23 AM, Wiles, Keith <keith.wiles@intel.com> wrote:
> 
> 
>> On Feb 2, 2017, at 10:17 AM, Pascal Mazon <pascal.mazon@6wind.com> wrote:
>> 
>> dev->data->name contains the device name, e.g. "net_tap0".
>> dev->data->dev_private->name contains the actual iface name,
>> e.g. "dtap0".
>> 
>> In any case, the name must to be consistent with the tun_alloc() call in
>> eth_dev_tap_create().
>> 
>> Signed-off-by: Pascal Mazon <pascal.mazon@6wind.com>
>> —
> 
> Acked-by: Keith Wiles <keith.wiles@intel.com>

Acked-by: Keith Wiles <keith.wiles@intel.com> for the series 1-7

> 
> Regards,
> Keith
> 

Regards,
Keith

Re: [dpdk-dev] [PATCH v2 1/7] net/tap: use correct tap name

[Ferruh Yigit]

On 2/2/2017 4:24 PM, Wiles, Keith wrote:
> 
>> On Feb 2, 2017, at 10:23 AM, Wiles, Keith <keith.wiles@intel.com> wrote:
>>
>>
>>> On Feb 2, 2017, at 10:17 AM, Pascal Mazon <pascal.mazon@6wind.com> wrote:
>>>
>>> dev->data->name contains the device name, e.g. "net_tap0".
>>> dev->data->dev_private->name contains the actual iface name,
>>> e.g. "dtap0".
>>>
>>> In any case, the name must to be consistent with the tun_alloc() call in
>>> eth_dev_tap_create().
>>>
>>> Signed-off-by: Pascal Mazon <pascal.mazon@6wind.com>
>>> —
>>
>> Acked-by: Keith Wiles <keith.wiles@intel.com>
> 
> Acked-by: Keith Wiles <keith.wiles@intel.com> for the series 1-7

Series applied to dpdk-next-net/master, thanks.