[dpdk-dev] [PATCH] net/tap: fix flow and port commands

[dpdk-dev] [PATCH] net/tap: fix flow and port commands

[Ophir Munk]

This commit fixes two bugs related to tap devices. The first bug occurs
when executing in testpmd the following flow rule assuming tap device has
4 rx and tx pair queues
"flow create 0 ingress pattern eth / end actions queue index 5 / end"
This command will report on success and will print ""Flow rule #0 created"
although it should have failed as queue index number 5 does not exist

The second bug occurs when executing in testpmd "port start all" following
a port configuration. Assuming 1 pair of rx and tx queues an error is
reported: "Fail to start port 0"

Before this commit a fixed max number (16) of rx and tx queue pairs were
created on startup where the file descriptors (fds) of rx and tx pairs were
identical.  As a result in the first bug queue index 5 existed because the
tap device was created with 16 rx and tx queue pairs regardless of the
configured number of queues. In the second bug when tap device was started
tx fd was closed before opening it and executing ioctl() on it. However
closing the sole fd of the device caused ioctl to fail with "No such
device".

This commit creates the configured number of rx and tx queue pairs (up to
max 16) and assigns a unique fd to each queue. It was written to solve the
first bug and was found as the right fix for the second bug as well.

Fixes: 02f96a0a82d1 ("net/tap: add TUN/TAP device PMD")
Fixes: bf7b7f437b49 ("net/tap: create netdevice during probing")
Fixes: de96fe68ae95 ("net/tap: add basic flow API patterns and actions")
Cc: stable@dpdk.org

Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
---
Pascal - I would appreciate your feedback on the following discussion:

> > /* Immediately create the netdevice (this will create the 1st queue). */
> > - if (tap_setup_queue(dev, pmd, 0) == -1)
> > + // rx queue
> > + if (tap_setup_queue(dev, pmd, 0, 1) == -1)
> > + goto error_exit;
> > + // tx queue
> > + if (tap_setup_queue(dev, pmd, 0, 0) == -1)
> > goto error_exit;
> >
> Adrien Mazarguil wrote:
> Can this be removed entirely to instead wait until the application actually
> requests the creation of the first queue?

Ophir Munk wrote:
Removing this piece of code is causing failure during tap device initialization and testpmd as well.
The subsequent code following this removal contains hundreds of lines so I prefer getting Pasca
advise why the immediate creation is required here and if possible - how to avoid it.

 drivers/net/tap/rte_eth_tap.c | 126 ++++++++++++++++++++++++++++++++----------
 drivers/net/tap/rte_eth_tap.h |   1 +
 drivers/net/tap/tap_flow.c    |   2 +-
 3 files changed, 99 insertions(+), 30 deletions(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index 9acea83..5520c24 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -603,8 +603,25 @@ enum ioctl_mode {
 }
 
 static int
-tap_dev_configure(struct rte_eth_dev *dev __rte_unused)
+tap_dev_configure(struct rte_eth_dev *dev)
 {
+	if (dev->data->nb_rx_queues > RTE_PMD_TAP_MAX_QUEUES) {
+		RTE_LOG(ERR, PMD, "%s: number of rx queues %d exceeds max num of queues %d\n",
+			dev->device->name, dev->data->nb_rx_queues, RTE_PMD_TAP_MAX_QUEUES);
+		return -1;
+	}
+	if (dev->data->nb_tx_queues > RTE_PMD_TAP_MAX_QUEUES) {
+		RTE_LOG(ERR, PMD, "%s: number of tx queues %d exceeds max num of queues %d\n",
+			dev->device->name, dev->data->nb_tx_queues, RTE_PMD_TAP_MAX_QUEUES);
+		return -1;
+	}
+
+	RTE_LOG(INFO, PMD, "%s: %p: TX queues number update: 0 -> %u\n",
+	     dev->device->name, (void *)dev, dev->data->nb_tx_queues);
+
+	RTE_LOG(INFO, PMD, "%s: %p: RX queues number update: 0 -> %u\n",
+	     dev->device->name, (void *)dev, dev->data->nb_rx_queues);
+
 	return 0;
 }
 
@@ -673,9 +690,9 @@ enum ioctl_mode {
 	unsigned long rx_nombuf = 0, ierrors = 0;
 	const struct pmd_internals *pmd = dev->data->dev_private;
 
-	imax = (pmd->nb_queues < RTE_ETHDEV_QUEUE_STAT_CNTRS) ?
-		pmd->nb_queues : RTE_ETHDEV_QUEUE_STAT_CNTRS;
-
+	/* rx queue statistics */
+	imax = (dev->data->nb_rx_queues < RTE_ETHDEV_QUEUE_STAT_CNTRS) ?
+		dev->data->nb_rx_queues : RTE_ETHDEV_QUEUE_STAT_CNTRS;
 	for (i = 0; i < imax; i++) {
 		tap_stats->q_ipackets[i] = pmd->rxq[i].stats.ipackets;
 		tap_stats->q_ibytes[i] = pmd->rxq[i].stats.ibytes;
@@ -683,7 +700,13 @@ enum ioctl_mode {
 		rx_bytes_total += tap_stats->q_ibytes[i];
 		rx_nombuf += pmd->rxq[i].stats.rx_nombuf;
 		ierrors += pmd->rxq[i].stats.ierrors;
+	}
+
+	/* tx queue statistics */
+	imax = (dev->data->nb_tx_queues < RTE_ETHDEV_QUEUE_STAT_CNTRS) ?
+		dev->data->nb_tx_queues : RTE_ETHDEV_QUEUE_STAT_CNTRS;
 
+	for (i = 0; i < imax; i++) {
 		tap_stats->q_opackets[i] = pmd->txq[i].stats.opackets;
 		tap_stats->q_errors[i] = pmd->txq[i].stats.errs;
 		tap_stats->q_obytes[i] = pmd->txq[i].stats.obytes;
@@ -730,10 +753,14 @@ enum ioctl_mode {
 	tap_flow_implicit_flush(internals, NULL);
 
 	for (i = 0; i < internals->nb_queues; i++) {
-		if (internals->rxq[i].fd != -1)
+		if (internals->rxq[i].fd != -1) {
 			close(internals->rxq[i].fd);
-		internals->rxq[i].fd = -1;
-		internals->txq[i].fd = -1;
+			internals->rxq[i].fd = -1;
+		}
+		if (internals->txq[i].fd != -1) {
+			close(internals->txq[i].fd);
+			internals->txq[i].fd = -1;
+		}
 	}
 
 	if (internals->remote_if_index) {
@@ -887,30 +914,59 @@ enum ioctl_mode {
 static int
 tap_setup_queue(struct rte_eth_dev *dev,
 		struct pmd_internals *internals,
-		uint16_t qid)
+		uint16_t qid,
+		int is_rx)
 {
+	int *fd;
+	int *other_fd;
+	const char *dir;
 	struct pmd_internals *pmd = dev->data->dev_private;
 	struct rx_queue *rx = &internals->rxq[qid];
 	struct tx_queue *tx = &internals->txq[qid];
-	int fd = rx->fd == -1 ? tx->fd : rx->fd;
 
-	if (fd == -1) {
-		RTE_LOG(INFO, PMD, "Add queue to TAP %s for qid %d\n",
-			pmd->name, qid);
-		fd = tun_alloc(pmd);
-		if (fd < 0) {
+	if (is_rx) {
+		fd = &rx->fd;
+		other_fd = &tx->fd;
+		dir = "rx";
+	} else {
+		fd = &tx->fd;
+		other_fd = &rx->fd;
+		dir = "tx";
+	}
+	if (*fd != -1) {
+		/* fd for this queue already exists */
+		RTE_LOG(DEBUG, PMD, "%s: fd %d for %s queue qid %d exists\n",
+			pmd->name, *fd, dir, qid);
+	} else if (*other_fd != -1) {
+		/* Only other_fd exists. dup it */
+		*fd = dup(*other_fd);
+		if (*fd < 0) {
+			*fd = -1;
+			RTE_LOG(ERR, PMD, "%s: dup() failed.\n",
+				pmd->name);
+			return -1;
+		} else {
+			RTE_LOG(DEBUG, PMD, "%s: dup fd %d for %s queue qid %d (%d)\n",
+				pmd->name, *other_fd, dir, qid, *fd);
+		}
+	} else {
+		/* Both RX and TX fds do not exist (equal -1). Create fd */
+		*fd = tun_alloc(pmd);
+		if (*fd < 0) {
+			*fd = -1; /* restore original value */
 			RTE_LOG(ERR, PMD, "%s: tun_alloc() failed.\n",
 				pmd->name);
 			return -1;
+		} else {
+			RTE_LOG(DEBUG, PMD, "%s: add %s queue for qid %d fd %d\n",
+				pmd->name, dir, qid, *fd);
 		}
 	}
 
-	rx->fd = fd;
-	tx->fd = fd;
 	tx->mtu = &dev->data->mtu;
 	rx->rxmode = &dev->data->dev_conf.rxmode;
 
-	return fd;
+	return *fd;
 }
 
 static int
@@ -932,10 +988,10 @@ enum ioctl_mode {
 	int fd;
 	int i;
 
-	if ((rx_queue_id >= internals->nb_queues) || !mp) {
+	if ((rx_queue_id >= dev->data->nb_rx_queues) || !mp) {
 		RTE_LOG(WARNING, PMD,
-			"nb_queues %d too small or mempool NULL\n",
-			internals->nb_queues);
+			"nb_rx_queues %d too small or mempool NULL\n",
+			dev->data->nb_rx_queues);
 		return -1;
 	}
 
@@ -954,7 +1010,7 @@ enum ioctl_mode {
 	rxq->iovecs = iovecs;
 
 	dev->data->rx_queues[rx_queue_id] = rxq;
-	fd = tap_setup_queue(dev, internals, rx_queue_id);
+	fd = tap_setup_queue(dev, internals, rx_queue_id, 1);
 	if (fd == -1) {
 		ret = fd;
 		goto error;
@@ -1002,11 +1058,11 @@ enum ioctl_mode {
 	struct pmd_internals *internals = dev->data->dev_private;
 	int ret;
 
-	if (tx_queue_id >= internals->nb_queues)
+	if (tx_queue_id >= dev->data->nb_tx_queues)
 		return -1;
 
 	dev->data->tx_queues[tx_queue_id] = &internals->txq[tx_queue_id];
-	ret = tap_setup_queue(dev, internals, tx_queue_id);
+	ret = tap_setup_queue(dev, internals, tx_queue_id, 0);
 	if (ret == -1)
 		return -1;
 
@@ -1166,7 +1222,6 @@ enum ioctl_mode {
 	.filter_ctrl            = tap_dev_filter_ctrl,
 };
 
-
 static int
 eth_dev_tap_create(struct rte_vdev_device *vdev, char *tap_name,
 		   char *remote_iface, int fixed_mac_type)
@@ -1193,6 +1248,7 @@ enum ioctl_mode {
 	}
 
 	pmd = dev->data->dev_private;
+	pmd->dev = dev;
 	snprintf(pmd->name, sizeof(pmd->name), "%s", tap_name);
 	pmd->nb_queues = RTE_PMD_TAP_MAX_QUEUES;
 
@@ -1212,8 +1268,9 @@ enum ioctl_mode {
 
 	data->dev_link = pmd_link;
 	data->mac_addrs = &pmd->eth_addr;
-	data->nb_rx_queues = pmd->nb_queues;
-	data->nb_tx_queues = pmd->nb_queues;
+	/* Set the number of RX and TX queues */
+	data->nb_rx_queues = 0;
+	data->nb_tx_queues = 0;
 
 	dev->data = data;
 	dev->dev_ops = &ops;
@@ -1241,7 +1298,11 @@ enum ioctl_mode {
 	}
 
 	/* Immediately create the netdevice (this will create the 1st queue). */
-	if (tap_setup_queue(dev, pmd, 0) == -1)
+	/* rx queue */
+	if (tap_setup_queue(dev, pmd, 0, 1) == -1)
+		goto error_exit;
+	/* tx queue */
+	if (tap_setup_queue(dev, pmd, 0, 0) == -1)
 		goto error_exit;
 
 	ifr.ifr_mtu = dev->data->mtu;
@@ -1515,9 +1576,16 @@ enum ioctl_mode {
 		tap_flow_implicit_flush(internals, NULL);
 		nl_final(internals->nlsk_fd);
 	}
-	for (i = 0; i < internals->nb_queues; i++)
-		if (internals->rxq[i].fd != -1)
+	for (i = 0; i < internals->nb_queues; i++) {
+		if (internals->rxq[i].fd != -1) {
 			close(internals->rxq[i].fd);
+			internals->rxq[i].fd = -1;
+		}
+		if (internals->txq[i].fd != -1) {
+			close(internals->txq[i].fd);
+			internals->txq[i].fd = -1;
+		}
+	}
 
 	close(internals->ioctl_sock);
 	rte_free(eth_dev->data->dev_private);
diff --git a/drivers/net/tap/rte_eth_tap.h b/drivers/net/tap/rte_eth_tap.h
index 928a045..be57cc7 100644
--- a/drivers/net/tap/rte_eth_tap.h
+++ b/drivers/net/tap/rte_eth_tap.h
@@ -80,6 +80,7 @@ struct tx_queue {
 };
 
 struct pmd_internals {
+	struct rte_eth_dev *dev;          /* Ethernet device. */
 	char remote_iface[RTE_ETH_NAME_MAX_LEN]; /* Remote netdevice name */
 	char name[RTE_ETH_NAME_MAX_LEN];  /* Internal Tap device name */
 	uint16_t nb_queues;               /* Number of queues supported */
diff --git a/drivers/net/tap/tap_flow.c b/drivers/net/tap/tap_flow.c
index 41f7345..c51f708 100644
--- a/drivers/net/tap/tap_flow.c
+++ b/drivers/net/tap/tap_flow.c
@@ -1092,7 +1092,7 @@ struct tap_flow_items {
 			if (action)
 				goto exit_action_not_supported;
 			action = 1;
-			if (!queue || (queue->index >= pmd->nb_queues))
+			if (!queue || (queue->index > pmd->dev->data->nb_rx_queues - 1))
 				goto exit_action_not_supported;
 			if (flow)
 				err = add_action_skbedit(flow, queue->index);
-- 
1.8.3.1

[dpdk-dev] [PATCH v2] net/tap: fix flow and port commands

[Ophir Munk]

This commit fixes two bugs related to tap devices. The first bug occurs
when executing in testpmd the following flow rule assuming tap device has
4 rx and tx pair queues
"flow create 0 ingress pattern eth / end actions queue index 5 / end"
This command will report on success and will print ""Flow rule #0 created"
although it should have failed as queue index number 5 does not exist

The second bug occurs when executing in testpmd "port start all" following
a port configuration. Assuming 1 pair of rx and tx queues an error is
reported: "Fail to start port 0"

Before this commit a fixed max number (16) of rx and tx queue pairs were
created on startup where the file descriptors (fds) of rx and tx pairs were
identical.  As a result in the first bug queue index 5 existed because the
tap device was created with 16 rx and tx queue pairs regardless of the
configured number of queues. In the second bug when tap device was started
tx fd was closed before opening it and executing ioctl() on it. However
closing the sole fd of the device caused ioctl to fail with "No such
device".

This commit creates the configured number of rx and tx queue pairs (up to
max 16) and assigns a unique fd to each queue. It was written to solve the
first bug and was found as the right fix for the second bug as well.

Fixes: 02f96a0a82d1 ("net/tap: add TUN/TAP device PMD")
Fixes: bf7b7f437b49 ("net/tap: create netdevice during probing")
Fixes: de96fe68ae95 ("net/tap: add basic flow API patterns and actions")
Cc: stable@dpdk.org

Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
---
Comment 1:
Reason for V2: fixing coding style. Previous versions of this patch are supersseded

Comment 2:
Pascal - I would appreciate your feedback on the following discussion:

> > /* Immediately create the netdevice (this will create the 1st 
> > queue). */
> > - if (tap_setup_queue(dev, pmd, 0) == -1)
> > + // rx queue
> > + if (tap_setup_queue(dev, pmd, 0, 1) == -1) goto error_exit; // tx 
> > + queue if (tap_setup_queue(dev, pmd, 0, 0) == -1)
> > goto error_exit;
> >
> Adrien Mazarguil wrote:
> Can this be removed entirely to instead wait until the application 
> actually requests the creation of the first queue?

Ophir Munk wrote:
Removing this piece of code is causing failure during tap device initialization and testpmd as well.
The subsequent code following this removal contains hundreds of lines so I prefer getting Pasca advise why the immediate creation is required here and if possible - how to avoid it.

 drivers/net/tap/rte_eth_tap.c | 130 ++++++++++++++++++++++++++++++++----------
 drivers/net/tap/rte_eth_tap.h |   1 +
 drivers/net/tap/tap_flow.c    |   3 +-
 3 files changed, 104 insertions(+), 30 deletions(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index 9acea83..f8cf330 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -603,8 +603,31 @@ enum ioctl_mode {
 }
 
 static int
-tap_dev_configure(struct rte_eth_dev *dev __rte_unused)
+tap_dev_configure(struct rte_eth_dev *dev)
 {
+	if (dev->data->nb_rx_queues > RTE_PMD_TAP_MAX_QUEUES) {
+		RTE_LOG(ERR, PMD,
+			"%s: number of rx queues %d exceeds max num of queues %d\n",
+			dev->device->name,
+			dev->data->nb_rx_queues,
+			RTE_PMD_TAP_MAX_QUEUES);
+		return -1;
+	}
+	if (dev->data->nb_tx_queues > RTE_PMD_TAP_MAX_QUEUES) {
+		RTE_LOG(ERR, PMD,
+			"%s: number of tx queues %d exceeds max num of queues %d\n",
+			dev->device->name,
+			dev->data->nb_tx_queues,
+			RTE_PMD_TAP_MAX_QUEUES);
+		return -1;
+	}
+
+	RTE_LOG(INFO, PMD, "%s: %p: TX queues number update: 0 -> %u\n",
+	     dev->device->name, (void *)dev, dev->data->nb_tx_queues);
+
+	RTE_LOG(INFO, PMD, "%s: %p: RX queues number update: 0 -> %u\n",
+	     dev->device->name, (void *)dev, dev->data->nb_rx_queues);
+
 	return 0;
 }
 
@@ -673,9 +696,9 @@ enum ioctl_mode {
 	unsigned long rx_nombuf = 0, ierrors = 0;
 	const struct pmd_internals *pmd = dev->data->dev_private;
 
-	imax = (pmd->nb_queues < RTE_ETHDEV_QUEUE_STAT_CNTRS) ?
-		pmd->nb_queues : RTE_ETHDEV_QUEUE_STAT_CNTRS;
-
+	/* rx queue statistics */
+	imax = (dev->data->nb_rx_queues < RTE_ETHDEV_QUEUE_STAT_CNTRS) ?
+		dev->data->nb_rx_queues : RTE_ETHDEV_QUEUE_STAT_CNTRS;
 	for (i = 0; i < imax; i++) {
 		tap_stats->q_ipackets[i] = pmd->rxq[i].stats.ipackets;
 		tap_stats->q_ibytes[i] = pmd->rxq[i].stats.ibytes;
@@ -683,7 +706,13 @@ enum ioctl_mode {
 		rx_bytes_total += tap_stats->q_ibytes[i];
 		rx_nombuf += pmd->rxq[i].stats.rx_nombuf;
 		ierrors += pmd->rxq[i].stats.ierrors;
+	}
+
+	/* tx queue statistics */
+	imax = (dev->data->nb_tx_queues < RTE_ETHDEV_QUEUE_STAT_CNTRS) ?
+		dev->data->nb_tx_queues : RTE_ETHDEV_QUEUE_STAT_CNTRS;
 
+	for (i = 0; i < imax; i++) {
 		tap_stats->q_opackets[i] = pmd->txq[i].stats.opackets;
 		tap_stats->q_errors[i] = pmd->txq[i].stats.errs;
 		tap_stats->q_obytes[i] = pmd->txq[i].stats.obytes;
@@ -730,10 +759,14 @@ enum ioctl_mode {
 	tap_flow_implicit_flush(internals, NULL);
 
 	for (i = 0; i < internals->nb_queues; i++) {
-		if (internals->rxq[i].fd != -1)
+		if (internals->rxq[i].fd != -1) {
 			close(internals->rxq[i].fd);
-		internals->rxq[i].fd = -1;
-		internals->txq[i].fd = -1;
+			internals->rxq[i].fd = -1;
+		}
+		if (internals->txq[i].fd != -1) {
+			close(internals->txq[i].fd);
+			internals->txq[i].fd = -1;
+		}
 	}
 
 	if (internals->remote_if_index) {
@@ -887,30 +920,57 @@ enum ioctl_mode {
 static int
 tap_setup_queue(struct rte_eth_dev *dev,
 		struct pmd_internals *internals,
-		uint16_t qid)
+		uint16_t qid,
+		int is_rx)
 {
+	int *fd;
+	int *other_fd;
+	const char *dir;
 	struct pmd_internals *pmd = dev->data->dev_private;
 	struct rx_queue *rx = &internals->rxq[qid];
 	struct tx_queue *tx = &internals->txq[qid];
-	int fd = rx->fd == -1 ? tx->fd : rx->fd;
 
-	if (fd == -1) {
-		RTE_LOG(INFO, PMD, "Add queue to TAP %s for qid %d\n",
-			pmd->name, qid);
-		fd = tun_alloc(pmd);
-		if (fd < 0) {
+	if (is_rx) {
+		fd = &rx->fd;
+		other_fd = &tx->fd;
+		dir = "rx";
+	} else {
+		fd = &tx->fd;
+		other_fd = &rx->fd;
+		dir = "tx";
+	}
+	if (*fd != -1) {
+		/* fd for this queue already exists */
+		RTE_LOG(DEBUG, PMD, "%s: fd %d for %s queue qid %d exists\n",
+			pmd->name, *fd, dir, qid);
+	} else if (*other_fd != -1) {
+		/* Only other_fd exists. dup it */
+		*fd = dup(*other_fd);
+		if (*fd < 0) {
+			*fd = -1;
+			RTE_LOG(ERR, PMD, "%s: dup() failed.\n",
+				pmd->name);
+			return -1;
+		}
+		RTE_LOG(DEBUG, PMD, "%s: dup fd %d for %s queue qid %d (%d)\n",
+			pmd->name, *other_fd, dir, qid, *fd);
+	} else {
+		/* Both RX and TX fds do not exist (equal -1). Create fd */
+		*fd = tun_alloc(pmd);
+		if (*fd < 0) {
+			*fd = -1; /* restore original value */
 			RTE_LOG(ERR, PMD, "%s: tun_alloc() failed.\n",
 				pmd->name);
 			return -1;
 		}
+		RTE_LOG(DEBUG, PMD, "%s: add %s queue for qid %d fd %d\n",
+			pmd->name, dir, qid, *fd);
 	}
 
-	rx->fd = fd;
-	tx->fd = fd;
 	tx->mtu = &dev->data->mtu;
 	rx->rxmode = &dev->data->dev_conf.rxmode;
 
-	return fd;
+	return *fd;
 }
 
 static int
@@ -932,10 +992,10 @@ enum ioctl_mode {
 	int fd;
 	int i;
 
-	if ((rx_queue_id >= internals->nb_queues) || !mp) {
+	if (rx_queue_id >= dev->data->nb_rx_queues || !mp) {
 		RTE_LOG(WARNING, PMD,
-			"nb_queues %d too small or mempool NULL\n",
-			internals->nb_queues);
+			"nb_rx_queues %d too small or mempool NULL\n",
+			dev->data->nb_rx_queues);
 		return -1;
 	}
 
@@ -954,7 +1014,7 @@ enum ioctl_mode {
 	rxq->iovecs = iovecs;
 
 	dev->data->rx_queues[rx_queue_id] = rxq;
-	fd = tap_setup_queue(dev, internals, rx_queue_id);
+	fd = tap_setup_queue(dev, internals, rx_queue_id, 1);
 	if (fd == -1) {
 		ret = fd;
 		goto error;
@@ -1002,11 +1062,11 @@ enum ioctl_mode {
 	struct pmd_internals *internals = dev->data->dev_private;
 	int ret;
 
-	if (tx_queue_id >= internals->nb_queues)
+	if (tx_queue_id >= dev->data->nb_tx_queues)
 		return -1;
 
 	dev->data->tx_queues[tx_queue_id] = &internals->txq[tx_queue_id];
-	ret = tap_setup_queue(dev, internals, tx_queue_id);
+	ret = tap_setup_queue(dev, internals, tx_queue_id, 0);
 	if (ret == -1)
 		return -1;
 
@@ -1166,7 +1226,6 @@ enum ioctl_mode {
 	.filter_ctrl            = tap_dev_filter_ctrl,
 };
 
-
 static int
 eth_dev_tap_create(struct rte_vdev_device *vdev, char *tap_name,
 		   char *remote_iface, int fixed_mac_type)
@@ -1193,6 +1252,7 @@ enum ioctl_mode {
 	}
 
 	pmd = dev->data->dev_private;
+	pmd->dev = dev;
 	snprintf(pmd->name, sizeof(pmd->name), "%s", tap_name);
 	pmd->nb_queues = RTE_PMD_TAP_MAX_QUEUES;
 
@@ -1212,8 +1272,9 @@ enum ioctl_mode {
 
 	data->dev_link = pmd_link;
 	data->mac_addrs = &pmd->eth_addr;
-	data->nb_rx_queues = pmd->nb_queues;
-	data->nb_tx_queues = pmd->nb_queues;
+	/* Set the number of RX and TX queues */
+	data->nb_rx_queues = 0;
+	data->nb_tx_queues = 0;
 
 	dev->data = data;
 	dev->dev_ops = &ops;
@@ -1241,7 +1302,11 @@ enum ioctl_mode {
 	}
 
 	/* Immediately create the netdevice (this will create the 1st queue). */
-	if (tap_setup_queue(dev, pmd, 0) == -1)
+	/* rx queue */
+	if (tap_setup_queue(dev, pmd, 0, 1) == -1)
+		goto error_exit;
+	/* tx queue */
+	if (tap_setup_queue(dev, pmd, 0, 0) == -1)
 		goto error_exit;
 
 	ifr.ifr_mtu = dev->data->mtu;
@@ -1515,9 +1580,16 @@ enum ioctl_mode {
 		tap_flow_implicit_flush(internals, NULL);
 		nl_final(internals->nlsk_fd);
 	}
-	for (i = 0; i < internals->nb_queues; i++)
-		if (internals->rxq[i].fd != -1)
+	for (i = 0; i < internals->nb_queues; i++) {
+		if (internals->rxq[i].fd != -1) {
 			close(internals->rxq[i].fd);
+			internals->rxq[i].fd = -1;
+		}
+		if (internals->txq[i].fd != -1) {
+			close(internals->txq[i].fd);
+			internals->txq[i].fd = -1;
+		}
+	}
 
 	close(internals->ioctl_sock);
 	rte_free(eth_dev->data->dev_private);
diff --git a/drivers/net/tap/rte_eth_tap.h b/drivers/net/tap/rte_eth_tap.h
index 928a045..be57cc7 100644
--- a/drivers/net/tap/rte_eth_tap.h
+++ b/drivers/net/tap/rte_eth_tap.h
@@ -80,6 +80,7 @@ struct tx_queue {
 };
 
 struct pmd_internals {
+	struct rte_eth_dev *dev;          /* Ethernet device. */
 	char remote_iface[RTE_ETH_NAME_MAX_LEN]; /* Remote netdevice name */
 	char name[RTE_ETH_NAME_MAX_LEN];  /* Internal Tap device name */
 	uint16_t nb_queues;               /* Number of queues supported */
diff --git a/drivers/net/tap/tap_flow.c b/drivers/net/tap/tap_flow.c
index 41f7345..eefa868 100644
--- a/drivers/net/tap/tap_flow.c
+++ b/drivers/net/tap/tap_flow.c
@@ -1092,7 +1092,8 @@ struct tap_flow_items {
 			if (action)
 				goto exit_action_not_supported;
 			action = 1;
-			if (!queue || (queue->index >= pmd->nb_queues))
+			if (!queue ||
+			(queue->index > pmd->dev->data->nb_rx_queues - 1))
 				goto exit_action_not_supported;
 			if (flow)
 				err = add_action_skbedit(flow, queue->index);
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH v2] net/tap: fix flow and port commands

[Pascal Mazon]

Hi,

The patch looks good globally, but I have a few comments/requests, inline.

Best regards,

Pascal

On 14/09/2017 17:07, Ophir Munk wrote:
> This commit fixes two bugs related to tap devices. The first bug occurs
> when executing in testpmd the following flow rule assuming tap device has
> 4 rx and tx pair queues
> "flow create 0 ingress pattern eth / end actions queue index 5 / end"
> This command will report on success and will print ""Flow rule #0 created"
> although it should have failed as queue index number 5 does not exist
>
> The second bug occurs when executing in testpmd "port start all" following
> a port configuration. Assuming 1 pair of rx and tx queues an error is
> reported: "Fail to start port 0"
>
> Before this commit a fixed max number (16) of rx and tx queue pairs were
> created on startup where the file descriptors (fds) of rx and tx pairs were
> identical.  As a result in the first bug queue index 5 existed because the
> tap device was created with 16 rx and tx queue pairs regardless of the
> configured number of queues. In the second bug when tap device was started
> tx fd was closed before opening it and executing ioctl() on it. However
> closing the sole fd of the device caused ioctl to fail with "No such
> device".
>
> This commit creates the configured number of rx and tx queue pairs (up to
> max 16) and assigns a unique fd to each queue. It was written to solve the
> first bug and was found as the right fix for the second bug as well.
>
> Fixes: 02f96a0a82d1 ("net/tap: add TUN/TAP device PMD")
> Fixes: bf7b7f437b49 ("net/tap: create netdevice during probing")
> Fixes: de96fe68ae95 ("net/tap: add basic flow API patterns and actions")
> Cc: stable@dpdk.org
>
> Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
> ---
> Comment 1:
> Reason for V2: fixing coding style. Previous versions of this patch are supersseded
>
> Comment 2:
> Pascal - I would appreciate your feedback on the following discussion:
>
>>> /* Immediately create the netdevice (this will create the 1st 
>>> queue). */
>>> - if (tap_setup_queue(dev, pmd, 0) == -1)
>>> + // rx queue
>>> + if (tap_setup_queue(dev, pmd, 0, 1) == -1) goto error_exit; // tx 
>>> + queue if (tap_setup_queue(dev, pmd, 0, 0) == -1)
>>> goto error_exit;
>>>
>> Adrien Mazarguil wrote:
>> Can this be removed entirely to instead wait until the application 
>> actually requests the creation of the first queue?
> Ophir Munk wrote:
> Removing this piece of code is causing failure during tap device initialization and testpmd as well.
> The subsequent code following this removal contains hundreds of lines so I prefer getting Pasca advise why the immediate creation is required here and if possible - how to avoid it.
I think it was required to have the netdevice present as soon as we left
the create function, as some other function occurred just a little later
(prior to queue_setup) that needed ioctl() on the netdevice.
I would leave it like this for this patch, but it would be more elegant
to create the netdevice only when really needed.

>
>  drivers/net/tap/rte_eth_tap.c | 130 ++++++++++++++++++++++++++++++++----------
>  drivers/net/tap/rte_eth_tap.h |   1 +
>  drivers/net/tap/tap_flow.c    |   3 +-
>  3 files changed, 104 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> index 9acea83..f8cf330 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -603,8 +603,31 @@ enum ioctl_mode {
>  }
>  
>  static int
> -tap_dev_configure(struct rte_eth_dev *dev __rte_unused)
> +tap_dev_configure(struct rte_eth_dev *dev)
>  {
> +	if (dev->data->nb_rx_queues > RTE_PMD_TAP_MAX_QUEUES) {
> +		RTE_LOG(ERR, PMD,
> +			"%s: number of rx queues %d exceeds max num of queues %d\n",
> +			dev->device->name,
> +			dev->data->nb_rx_queues,
> +			RTE_PMD_TAP_MAX_QUEUES);
> +		return -1;
> +	}
> +	if (dev->data->nb_tx_queues > RTE_PMD_TAP_MAX_QUEUES) {
> +		RTE_LOG(ERR, PMD,
> +			"%s: number of tx queues %d exceeds max num of queues %d\n",
> +			dev->device->name,
> +			dev->data->nb_tx_queues,
> +			RTE_PMD_TAP_MAX_QUEUES);
> +		return -1;
> +	}
> +
> +	RTE_LOG(INFO, PMD, "%s: %p: TX queues number update: 0 -> %u\n",
> +	     dev->device->name, (void *)dev, dev->data->nb_tx_queues);
> +
> +	RTE_LOG(INFO, PMD, "%s: %p: RX queues number update: 0 -> %u\n",
> +	     dev->device->name, (void *)dev, dev->data->nb_rx_queues);
> +
>  	return 0;
>  }
tap_dev_configure() can be called when restarting a port, I'm not so
sure the number update changes number of queues from "0".
Why not a simple "TX/RX configured queues number: %u"?
> @@ -673,9 +696,9 @@ enum ioctl_mode {
>  	unsigned long rx_nombuf = 0, ierrors = 0;
>  	const struct pmd_internals *pmd = dev->data->dev_private;
>  
> -	imax = (pmd->nb_queues < RTE_ETHDEV_QUEUE_STAT_CNTRS) ?
> -		pmd->nb_queues : RTE_ETHDEV_QUEUE_STAT_CNTRS;
> -
> +	/* rx queue statistics */
> +	imax = (dev->data->nb_rx_queues < RTE_ETHDEV_QUEUE_STAT_CNTRS) ?
> +		dev->data->nb_rx_queues : RTE_ETHDEV_QUEUE_STAT_CNTRS;
>  	for (i = 0; i < imax; i++) {
>  		tap_stats->q_ipackets[i] = pmd->rxq[i].stats.ipackets;
>  		tap_stats->q_ibytes[i] = pmd->rxq[i].stats.ibytes;
> @@ -683,7 +706,13 @@ enum ioctl_mode {
>  		rx_bytes_total += tap_stats->q_ibytes[i];
>  		rx_nombuf += pmd->rxq[i].stats.rx_nombuf;
>  		ierrors += pmd->rxq[i].stats.ierrors;
> +	}
> +
> +	/* tx queue statistics */
> +	imax = (dev->data->nb_tx_queues < RTE_ETHDEV_QUEUE_STAT_CNTRS) ?
> +		dev->data->nb_tx_queues : RTE_ETHDEV_QUEUE_STAT_CNTRS;
>  
> +	for (i = 0; i < imax; i++) {
>  		tap_stats->q_opackets[i] = pmd->txq[i].stats.opackets;
>  		tap_stats->q_errors[i] = pmd->txq[i].stats.errs;
>  		tap_stats->q_obytes[i] = pmd->txq[i].stats.obytes;
> @@ -730,10 +759,14 @@ enum ioctl_mode {
>  	tap_flow_implicit_flush(internals, NULL);
>  
>  	for (i = 0; i < internals->nb_queues; i++) {
> -		if (internals->rxq[i].fd != -1)
> +		if (internals->rxq[i].fd != -1) {
>  			close(internals->rxq[i].fd);
> -		internals->rxq[i].fd = -1;
> -		internals->txq[i].fd = -1;
> +			internals->rxq[i].fd = -1;
> +		}
> +		if (internals->txq[i].fd != -1) {
> +			close(internals->txq[i].fd);
> +			internals->txq[i].fd = -1;
> +		}
>  	}
>  
>  	if (internals->remote_if_index) {
> @@ -887,30 +920,57 @@ enum ioctl_mode {
>  static int
>  tap_setup_queue(struct rte_eth_dev *dev,
>  		struct pmd_internals *internals,
> -		uint16_t qid)
> +		uint16_t qid,
> +		int is_rx)
>  {
> +	int *fd;
> +	int *other_fd;
> +	const char *dir;
>  	struct pmd_internals *pmd = dev->data->dev_private;
>  	struct rx_queue *rx = &internals->rxq[qid];
>  	struct tx_queue *tx = &internals->txq[qid];
> -	int fd = rx->fd == -1 ? tx->fd : rx->fd;
>  
> -	if (fd == -1) {
> -		RTE_LOG(INFO, PMD, "Add queue to TAP %s for qid %d\n",
> -			pmd->name, qid);
> -		fd = tun_alloc(pmd);
> -		if (fd < 0) {
> +	if (is_rx) {
> +		fd = &rx->fd;
> +		other_fd = &tx->fd;
> +		dir = "rx";
> +	} else {
> +		fd = &tx->fd;
> +		other_fd = &rx->fd;
> +		dir = "tx";
> +	}
> +	if (*fd != -1) {
> +		/* fd for this queue already exists */
> +		RTE_LOG(DEBUG, PMD, "%s: fd %d for %s queue qid %d exists\n",
> +			pmd->name, *fd, dir, qid);
> +	} else if (*other_fd != -1) {
> +		/* Only other_fd exists. dup it */
> +		*fd = dup(*other_fd);
> +		if (*fd < 0) {
> +			*fd = -1;
> +			RTE_LOG(ERR, PMD, "%s: dup() failed.\n",
> +				pmd->name);
> +			return -1;
> +		}
> +		RTE_LOG(DEBUG, PMD, "%s: dup fd %d for %s queue qid %d (%d)\n",
> +			pmd->name, *other_fd, dir, qid, *fd);
> +	} else {
> +		/* Both RX and TX fds do not exist (equal -1). Create fd */
> +		*fd = tun_alloc(pmd);
> +		if (*fd < 0) {
> +			*fd = -1; /* restore original value */
>  			RTE_LOG(ERR, PMD, "%s: tun_alloc() failed.\n",
>  				pmd->name);
>  			return -1;
>  		}
> +		RTE_LOG(DEBUG, PMD, "%s: add %s queue for qid %d fd %d\n",
> +			pmd->name, dir, qid, *fd);
>  	}
>  
> -	rx->fd = fd;
> -	tx->fd = fd;
>  	tx->mtu = &dev->data->mtu;
>  	rx->rxmode = &dev->data->dev_conf.rxmode;
>  
> -	return fd;
> +	return *fd;
>  }
>  
>  static int
> @@ -932,10 +992,10 @@ enum ioctl_mode {
>  	int fd;
>  	int i;
>  
> -	if ((rx_queue_id >= internals->nb_queues) || !mp) {
> +	if (rx_queue_id >= dev->data->nb_rx_queues || !mp) {
>  		RTE_LOG(WARNING, PMD,
> -			"nb_queues %d too small or mempool NULL\n",
> -			internals->nb_queues);
> +			"nb_rx_queues %d too small or mempool NULL\n",
> +			dev->data->nb_rx_queues);
>  		return -1;
>  	}
>  
> @@ -954,7 +1014,7 @@ enum ioctl_mode {
>  	rxq->iovecs = iovecs;
>  
>  	dev->data->rx_queues[rx_queue_id] = rxq;
> -	fd = tap_setup_queue(dev, internals, rx_queue_id);
> +	fd = tap_setup_queue(dev, internals, rx_queue_id, 1);
>  	if (fd == -1) {
>  		ret = fd;
>  		goto error;
> @@ -1002,11 +1062,11 @@ enum ioctl_mode {
>  	struct pmd_internals *internals = dev->data->dev_private;
>  	int ret;
>  
> -	if (tx_queue_id >= internals->nb_queues)
> +	if (tx_queue_id >= dev->data->nb_tx_queues)
>  		return -1;
>  
>  	dev->data->tx_queues[tx_queue_id] = &internals->txq[tx_queue_id];
> -	ret = tap_setup_queue(dev, internals, tx_queue_id);
> +	ret = tap_setup_queue(dev, internals, tx_queue_id, 0);
>  	if (ret == -1)
>  		return -1;
>  
> @@ -1166,7 +1226,6 @@ enum ioctl_mode {
>  	.filter_ctrl            = tap_dev_filter_ctrl,
>  };
>  
> -
>  static int
>  eth_dev_tap_create(struct rte_vdev_device *vdev, char *tap_name,
>  		   char *remote_iface, int fixed_mac_type)
> @@ -1193,6 +1252,7 @@ enum ioctl_mode {
>  	}
>  
>  	pmd = dev->data->dev_private;
> +	pmd->dev = dev;
You use pmd->dev only to access dev->nb_rx_queues.
I would suggest instead to add *nb_rx_queues in struct pmd_internals,
and remove *dev from it.
You can even remove nb_queues from pmd_internals, and only use
nb_rx_queues and RTE_PMD_TAP_MAX_QUEUES where relevant.

>  	snprintf(pmd->name, sizeof(pmd->name), "%s", tap_name);
>  	pmd->nb_queues = RTE_PMD_TAP_MAX_QUEUES;
>  
> @@ -1212,8 +1272,9 @@ enum ioctl_mode {
>  
>  	data->dev_link = pmd_link;
>  	data->mac_addrs = &pmd->eth_addr;
> -	data->nb_rx_queues = pmd->nb_queues;
> -	data->nb_tx_queues = pmd->nb_queues;
> +	/* Set the number of RX and TX queues */
> +	data->nb_rx_queues = 0;
> +	data->nb_tx_queues = 0;
>  
>  	dev->data = data;
>  	dev->dev_ops = &ops;
> @@ -1241,7 +1302,11 @@ enum ioctl_mode {
>  	}
>  
>  	/* Immediately create the netdevice (this will create the 1st queue). */
> -	if (tap_setup_queue(dev, pmd, 0) == -1)
> +	/* rx queue */
> +	if (tap_setup_queue(dev, pmd, 0, 1) == -1)
> +		goto error_exit;
> +	/* tx queue */
> +	if (tap_setup_queue(dev, pmd, 0, 0) == -1)
>  		goto error_exit;
>  
>  	ifr.ifr_mtu = dev->data->mtu;
> @@ -1515,9 +1580,16 @@ enum ioctl_mode {
>  		tap_flow_implicit_flush(internals, NULL);
>  		nl_final(internals->nlsk_fd);
>  	}
> -	for (i = 0; i < internals->nb_queues; i++)
> -		if (internals->rxq[i].fd != -1)
> +	for (i = 0; i < internals->nb_queues; i++) {
> +		if (internals->rxq[i].fd != -1) {
>  			close(internals->rxq[i].fd);
> +			internals->rxq[i].fd = -1;
> +		}
> +		if (internals->txq[i].fd != -1) {
> +			close(internals->txq[i].fd);
> +			internals->txq[i].fd = -1;
> +		}
> +	}
>  
>  	close(internals->ioctl_sock);
>  	rte_free(eth_dev->data->dev_private);
> diff --git a/drivers/net/tap/rte_eth_tap.h b/drivers/net/tap/rte_eth_tap.h
> index 928a045..be57cc7 100644
> --- a/drivers/net/tap/rte_eth_tap.h
> +++ b/drivers/net/tap/rte_eth_tap.h
> @@ -80,6 +80,7 @@ struct tx_queue {
>  };
>  
>  struct pmd_internals {
> +	struct rte_eth_dev *dev;          /* Ethernet device. */
>  	char remote_iface[RTE_ETH_NAME_MAX_LEN]; /* Remote netdevice name */
>  	char name[RTE_ETH_NAME_MAX_LEN];  /* Internal Tap device name */
>  	uint16_t nb_queues;               /* Number of queues supported */
See comment above.
> diff --git a/drivers/net/tap/tap_flow.c b/drivers/net/tap/tap_flow.c
> index 41f7345..eefa868 100644
> --- a/drivers/net/tap/tap_flow.c
> +++ b/drivers/net/tap/tap_flow.c
> @@ -1092,7 +1092,8 @@ struct tap_flow_items {
>  			if (action)
>  				goto exit_action_not_supported;
>  			action = 1;
> -			if (!queue || (queue->index >= pmd->nb_queues))
> +			if (!queue ||
> +			(queue->index > pmd->dev->data->nb_rx_queues - 1))
>  				goto exit_action_not_supported;
>  			if (flow)
>  				err = add_action_skbedit(flow, queue->index);

[dpdk-dev] [PATCH v3] net/tap: fix flow and port commands

[Ophir Munk]

This commit fixes two bugs related to tap devices. The first bug occurs
when executing in testpmd the following flow rule assuming tap device has
4 rx and tx pair queues
"flow create 0 ingress pattern eth / end actions queue index 5 / end"
This command will report on success and will print ""Flow rule #0 created"
although it should have failed as queue index number 5 does not exist

The second bug occurs when executing in testpmd "port start all" following
a port configuration. Assuming 1 pair of rx and tx queues an error is
reported: "Fail to start port 0"

Before this commit a fixed max number (16) of rx and tx queue pairs were
created on startup where the file descriptors (fds) of rx and tx pairs were
identical.  As a result in the first bug queue index 5 existed because the
tap device was created with 16 rx and tx queue pairs regardless of the
configured number of queues. In the second bug when tap device was started
tx fd was closed before opening it and executing ioctl() on it. However
closing the sole fd of the device caused ioctl to fail with "No such
device".

This commit creates the configured number of rx and tx queue pairs (up to
max 16) and assigns a unique fd to each queue. It was written to solve the
first bug and was found as the right fix for the second bug as well.

Fixes: 02f96a0a82d1 ("net/tap: add TUN/TAP device PMD")
Fixes: bf7b7f437b49 ("net/tap: create netdevice during probing")
Fixes: de96fe68ae95 ("net/tap: add basic flow API patterns and actions")
Cc: stable@dpdk.org

Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
---
In reply to Pascal Mazon review meesage-id: <0e18a611-3b35-6624-013b-59c72f71307b@6wind.com>

 drivers/net/tap/rte_eth_tap.c | 139 +++++++++++++++++++++++++++++++-----------
 drivers/net/tap/rte_eth_tap.h |   2 +-
 drivers/net/tap/tap_flow.c    |   3 +-
 3 files changed, 108 insertions(+), 36 deletions(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index 9acea83..d926d4b 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -603,8 +603,31 @@ enum ioctl_mode {
 }
 
 static int
-tap_dev_configure(struct rte_eth_dev *dev __rte_unused)
+tap_dev_configure(struct rte_eth_dev *dev)
 {
+	if (dev->data->nb_rx_queues > RTE_PMD_TAP_MAX_QUEUES) {
+		RTE_LOG(ERR, PMD,
+			"%s: number of rx queues %d exceeds max num of queues %d\n",
+			dev->device->name,
+			dev->data->nb_rx_queues,
+			RTE_PMD_TAP_MAX_QUEUES);
+		return -1;
+	}
+	if (dev->data->nb_tx_queues > RTE_PMD_TAP_MAX_QUEUES) {
+		RTE_LOG(ERR, PMD,
+			"%s: number of tx queues %d exceeds max num of queues %d\n",
+			dev->device->name,
+			dev->data->nb_tx_queues,
+			RTE_PMD_TAP_MAX_QUEUES);
+		return -1;
+	}
+
+	RTE_LOG(INFO, PMD, "%s: %p: TX configured queues number: %u\n",
+	     dev->device->name, (void *)dev, dev->data->nb_tx_queues);
+
+	RTE_LOG(INFO, PMD, "%s: %p: RX configured queues number: %u\n",
+	     dev->device->name, (void *)dev, dev->data->nb_rx_queues);
+
 	return 0;
 }
 
@@ -650,8 +673,8 @@ enum ioctl_mode {
 	dev_info->if_index = internals->if_index;
 	dev_info->max_mac_addrs = 1;
 	dev_info->max_rx_pktlen = (uint32_t)ETHER_MAX_VLAN_FRAME_LEN;
-	dev_info->max_rx_queues = internals->nb_queues;
-	dev_info->max_tx_queues = internals->nb_queues;
+	dev_info->max_rx_queues = RTE_PMD_TAP_MAX_QUEUES;
+	dev_info->max_tx_queues = RTE_PMD_TAP_MAX_QUEUES;
 	dev_info->min_rx_bufsize = 0;
 	dev_info->pci_dev = NULL;
 	dev_info->speed_capa = tap_dev_speed_capa();
@@ -673,9 +696,9 @@ enum ioctl_mode {
 	unsigned long rx_nombuf = 0, ierrors = 0;
 	const struct pmd_internals *pmd = dev->data->dev_private;
 
-	imax = (pmd->nb_queues < RTE_ETHDEV_QUEUE_STAT_CNTRS) ?
-		pmd->nb_queues : RTE_ETHDEV_QUEUE_STAT_CNTRS;
-
+	/* rx queue statistics */
+	imax = (dev->data->nb_rx_queues < RTE_ETHDEV_QUEUE_STAT_CNTRS) ?
+		dev->data->nb_rx_queues : RTE_ETHDEV_QUEUE_STAT_CNTRS;
 	for (i = 0; i < imax; i++) {
 		tap_stats->q_ipackets[i] = pmd->rxq[i].stats.ipackets;
 		tap_stats->q_ibytes[i] = pmd->rxq[i].stats.ibytes;
@@ -683,7 +706,13 @@ enum ioctl_mode {
 		rx_bytes_total += tap_stats->q_ibytes[i];
 		rx_nombuf += pmd->rxq[i].stats.rx_nombuf;
 		ierrors += pmd->rxq[i].stats.ierrors;
+	}
 
+	/* tx queue statistics */
+	imax = (dev->data->nb_tx_queues < RTE_ETHDEV_QUEUE_STAT_CNTRS) ?
+		dev->data->nb_tx_queues : RTE_ETHDEV_QUEUE_STAT_CNTRS;
+
+	for (i = 0; i < imax; i++) {
 		tap_stats->q_opackets[i] = pmd->txq[i].stats.opackets;
 		tap_stats->q_errors[i] = pmd->txq[i].stats.errs;
 		tap_stats->q_obytes[i] = pmd->txq[i].stats.obytes;
@@ -707,7 +736,7 @@ enum ioctl_mode {
 	int i;
 	struct pmd_internals *pmd = dev->data->dev_private;
 
-	for (i = 0; i < pmd->nb_queues; i++) {
+	for (i = 0; i < RTE_PMD_TAP_MAX_QUEUES; i++) {
 		pmd->rxq[i].stats.ipackets = 0;
 		pmd->rxq[i].stats.ibytes = 0;
 		pmd->rxq[i].stats.ierrors = 0;
@@ -729,11 +758,15 @@ enum ioctl_mode {
 	tap_flow_flush(dev, NULL);
 	tap_flow_implicit_flush(internals, NULL);
 
-	for (i = 0; i < internals->nb_queues; i++) {
-		if (internals->rxq[i].fd != -1)
+	for (i = 0; i < RTE_PMD_TAP_MAX_QUEUES; i++) {
+		if (internals->rxq[i].fd != -1) {
 			close(internals->rxq[i].fd);
-		internals->rxq[i].fd = -1;
-		internals->txq[i].fd = -1;
+			internals->rxq[i].fd = -1;
+		}
+		if (internals->txq[i].fd != -1) {
+			close(internals->txq[i].fd);
+			internals->txq[i].fd = -1;
+		}
 	}
 
 	if (internals->remote_if_index) {
@@ -887,30 +920,57 @@ enum ioctl_mode {
 static int
 tap_setup_queue(struct rte_eth_dev *dev,
 		struct pmd_internals *internals,
-		uint16_t qid)
+		uint16_t qid,
+		int is_rx)
 {
+	int *fd;
+	int *other_fd;
+	const char *dir;
 	struct pmd_internals *pmd = dev->data->dev_private;
 	struct rx_queue *rx = &internals->rxq[qid];
 	struct tx_queue *tx = &internals->txq[qid];
-	int fd = rx->fd == -1 ? tx->fd : rx->fd;
 
-	if (fd == -1) {
-		RTE_LOG(INFO, PMD, "Add queue to TAP %s for qid %d\n",
-			pmd->name, qid);
-		fd = tun_alloc(pmd);
-		if (fd < 0) {
+	if (is_rx) {
+		fd = &rx->fd;
+		other_fd = &tx->fd;
+		dir = "rx";
+	} else {
+		fd = &tx->fd;
+		other_fd = &rx->fd;
+		dir = "tx";
+	}
+	if (*fd != -1) {
+		/* fd for this queue already exists */
+		RTE_LOG(DEBUG, PMD, "%s: fd %d for %s queue qid %d exists\n",
+			pmd->name, *fd, dir, qid);
+	} else if (*other_fd != -1) {
+		/* Only other_fd exists. dup it */
+		*fd = dup(*other_fd);
+		if (*fd < 0) {
+			*fd = -1;
+			RTE_LOG(ERR, PMD, "%s: dup() failed.\n",
+				pmd->name);
+			return -1;
+		}
+		RTE_LOG(DEBUG, PMD, "%s: dup fd %d for %s queue qid %d (%d)\n",
+			pmd->name, *other_fd, dir, qid, *fd);
+	} else {
+		/* Both RX and TX fds do not exist (equal -1). Create fd */
+		*fd = tun_alloc(pmd);
+		if (*fd < 0) {
+			*fd = -1; /* restore original value */
 			RTE_LOG(ERR, PMD, "%s: tun_alloc() failed.\n",
 				pmd->name);
 			return -1;
 		}
+		RTE_LOG(DEBUG, PMD, "%s: add %s queue for qid %d fd %d\n",
+			pmd->name, dir, qid, *fd);
 	}
 
-	rx->fd = fd;
-	tx->fd = fd;
 	tx->mtu = &dev->data->mtu;
 	rx->rxmode = &dev->data->dev_conf.rxmode;
 
-	return fd;
+	return *fd;
 }
 
 static int
@@ -932,10 +992,10 @@ enum ioctl_mode {
 	int fd;
 	int i;
 
-	if ((rx_queue_id >= internals->nb_queues) || !mp) {
+	if (rx_queue_id >= dev->data->nb_rx_queues || !mp) {
 		RTE_LOG(WARNING, PMD,
-			"nb_queues %d too small or mempool NULL\n",
-			internals->nb_queues);
+			"nb_rx_queues %d too small or mempool NULL\n",
+			dev->data->nb_rx_queues);
 		return -1;
 	}
 
@@ -954,7 +1014,7 @@ enum ioctl_mode {
 	rxq->iovecs = iovecs;
 
 	dev->data->rx_queues[rx_queue_id] = rxq;
-	fd = tap_setup_queue(dev, internals, rx_queue_id);
+	fd = tap_setup_queue(dev, internals, rx_queue_id, 1);
 	if (fd == -1) {
 		ret = fd;
 		goto error;
@@ -1002,11 +1062,11 @@ enum ioctl_mode {
 	struct pmd_internals *internals = dev->data->dev_private;
 	int ret;
 
-	if (tx_queue_id >= internals->nb_queues)
+	if (tx_queue_id >= dev->data->nb_tx_queues)
 		return -1;
 
 	dev->data->tx_queues[tx_queue_id] = &internals->txq[tx_queue_id];
-	ret = tap_setup_queue(dev, internals, tx_queue_id);
+	ret = tap_setup_queue(dev, internals, tx_queue_id, 0);
 	if (ret == -1)
 		return -1;
 
@@ -1166,7 +1226,6 @@ enum ioctl_mode {
 	.filter_ctrl            = tap_dev_filter_ctrl,
 };
 
-
 static int
 eth_dev_tap_create(struct rte_vdev_device *vdev, char *tap_name,
 		   char *remote_iface, int fixed_mac_type)
@@ -1193,8 +1252,8 @@ enum ioctl_mode {
 	}
 
 	pmd = dev->data->dev_private;
+	pmd->dev = dev;
 	snprintf(pmd->name, sizeof(pmd->name), "%s", tap_name);
-	pmd->nb_queues = RTE_PMD_TAP_MAX_QUEUES;
 
 	pmd->ioctl_sock = socket(AF_INET, SOCK_DGRAM, 0);
 	if (pmd->ioctl_sock == -1) {
@@ -1212,8 +1271,9 @@ enum ioctl_mode {
 
 	data->dev_link = pmd_link;
 	data->mac_addrs = &pmd->eth_addr;
-	data->nb_rx_queues = pmd->nb_queues;
-	data->nb_tx_queues = pmd->nb_queues;
+	/* Set the number of RX and TX queues */
+	data->nb_rx_queues = 0;
+	data->nb_tx_queues = 0;
 
 	dev->data = data;
 	dev->dev_ops = &ops;
@@ -1241,7 +1301,11 @@ enum ioctl_mode {
 	}
 
 	/* Immediately create the netdevice (this will create the 1st queue). */
-	if (tap_setup_queue(dev, pmd, 0) == -1)
+	/* rx queue */
+	if (tap_setup_queue(dev, pmd, 0, 1) == -1)
+		goto error_exit;
+	/* tx queue */
+	if (tap_setup_queue(dev, pmd, 0, 0) == -1)
 		goto error_exit;
 
 	ifr.ifr_mtu = dev->data->mtu;
@@ -1515,9 +1579,16 @@ enum ioctl_mode {
 		tap_flow_implicit_flush(internals, NULL);
 		nl_final(internals->nlsk_fd);
 	}
-	for (i = 0; i < internals->nb_queues; i++)
-		if (internals->rxq[i].fd != -1)
+	for (i = 0; i < RTE_PMD_TAP_MAX_QUEUES; i++) {
+		if (internals->rxq[i].fd != -1) {
 			close(internals->rxq[i].fd);
+			internals->rxq[i].fd = -1;
+		}
+		if (internals->txq[i].fd != -1) {
+			close(internals->txq[i].fd);
+			internals->txq[i].fd = -1;
+		}
+	}
 
 	close(internals->ioctl_sock);
 	rte_free(eth_dev->data->dev_private);
diff --git a/drivers/net/tap/rte_eth_tap.h b/drivers/net/tap/rte_eth_tap.h
index 928a045..829f32f 100644
--- a/drivers/net/tap/rte_eth_tap.h
+++ b/drivers/net/tap/rte_eth_tap.h
@@ -80,9 +80,9 @@ struct tx_queue {
 };
 
 struct pmd_internals {
+	struct rte_eth_dev *dev;          /* Ethernet device. */
 	char remote_iface[RTE_ETH_NAME_MAX_LEN]; /* Remote netdevice name */
 	char name[RTE_ETH_NAME_MAX_LEN];  /* Internal Tap device name */
-	uint16_t nb_queues;               /* Number of queues supported */
 	struct ether_addr eth_addr;       /* Mac address of the device port */
 	struct ifreq remote_initial_flags;   /* Remote netdevice flags on init */
 	int remote_if_index;              /* remote netdevice IF_INDEX */
diff --git a/drivers/net/tap/tap_flow.c b/drivers/net/tap/tap_flow.c
index 41f7345..eefa868 100644
--- a/drivers/net/tap/tap_flow.c
+++ b/drivers/net/tap/tap_flow.c
@@ -1092,7 +1092,8 @@ struct tap_flow_items {
 			if (action)
 				goto exit_action_not_supported;
 			action = 1;
-			if (!queue || (queue->index >= pmd->nb_queues))
+			if (!queue ||
+			(queue->index > pmd->dev->data->nb_rx_queues - 1))
 				goto exit_action_not_supported;
 			if (flow)
 				err = add_action_skbedit(flow, queue->index);
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH v3] net/tap: fix flow and port commands

[Pascal Mazon]

Hi,

Your reasons to keep *dev in pmd_internals are good enough for me!

The rest of the patch looks fine.

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

On 17/09/2017 00:32, Ophir Munk wrote:
> This commit fixes two bugs related to tap devices. The first bug occurs
> when executing in testpmd the following flow rule assuming tap device has
> 4 rx and tx pair queues
> "flow create 0 ingress pattern eth / end actions queue index 5 / end"
> This command will report on success and will print ""Flow rule #0 created"
> although it should have failed as queue index number 5 does not exist
>
> The second bug occurs when executing in testpmd "port start all" following
> a port configuration. Assuming 1 pair of rx and tx queues an error is
> reported: "Fail to start port 0"
>
> Before this commit a fixed max number (16) of rx and tx queue pairs were
> created on startup where the file descriptors (fds) of rx and tx pairs were
> identical.  As a result in the first bug queue index 5 existed because the
> tap device was created with 16 rx and tx queue pairs regardless of the
> configured number of queues. In the second bug when tap device was started
> tx fd was closed before opening it and executing ioctl() on it. However
> closing the sole fd of the device caused ioctl to fail with "No such
> device".
>
> This commit creates the configured number of rx and tx queue pairs (up to
> max 16) and assigns a unique fd to each queue. It was written to solve the
> first bug and was found as the right fix for the second bug as well.
>
> Fixes: 02f96a0a82d1 ("net/tap: add TUN/TAP device PMD")
> Fixes: bf7b7f437b49 ("net/tap: create netdevice during probing")
> Fixes: de96fe68ae95 ("net/tap: add basic flow API patterns and actions")
> Cc: stable@dpdk.org
>
> Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
> ---
> In reply to Pascal Mazon review meesage-id: <0e18a611-3b35-6624-013b-59c72f71307b@6wind.com>
>
>  drivers/net/tap/rte_eth_tap.c | 139 +++++++++++++++++++++++++++++++-----------
>  drivers/net/tap/rte_eth_tap.h |   2 +-
>  drivers/net/tap/tap_flow.c    |   3 +-
>  3 files changed, 108 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> index 9acea83..d926d4b 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -603,8 +603,31 @@ enum ioctl_mode {
>  }
>  
>  static int
> -tap_dev_configure(struct rte_eth_dev *dev __rte_unused)
> +tap_dev_configure(struct rte_eth_dev *dev)
>  {
> +	if (dev->data->nb_rx_queues > RTE_PMD_TAP_MAX_QUEUES) {
> +		RTE_LOG(ERR, PMD,
> +			"%s: number of rx queues %d exceeds max num of queues %d\n",
> +			dev->device->name,
> +			dev->data->nb_rx_queues,
> +			RTE_PMD_TAP_MAX_QUEUES);
> +		return -1;
> +	}
> +	if (dev->data->nb_tx_queues > RTE_PMD_TAP_MAX_QUEUES) {
> +		RTE_LOG(ERR, PMD,
> +			"%s: number of tx queues %d exceeds max num of queues %d\n",
> +			dev->device->name,
> +			dev->data->nb_tx_queues,
> +			RTE_PMD_TAP_MAX_QUEUES);
> +		return -1;
> +	}
> +
> +	RTE_LOG(INFO, PMD, "%s: %p: TX configured queues number: %u\n",
> +	     dev->device->name, (void *)dev, dev->data->nb_tx_queues);
> +
> +	RTE_LOG(INFO, PMD, "%s: %p: RX configured queues number: %u\n",
> +	     dev->device->name, (void *)dev, dev->data->nb_rx_queues);
> +
>  	return 0;
>  }
>  
> @@ -650,8 +673,8 @@ enum ioctl_mode {
>  	dev_info->if_index = internals->if_index;
>  	dev_info->max_mac_addrs = 1;
>  	dev_info->max_rx_pktlen = (uint32_t)ETHER_MAX_VLAN_FRAME_LEN;
> -	dev_info->max_rx_queues = internals->nb_queues;
> -	dev_info->max_tx_queues = internals->nb_queues;
> +	dev_info->max_rx_queues = RTE_PMD_TAP_MAX_QUEUES;
> +	dev_info->max_tx_queues = RTE_PMD_TAP_MAX_QUEUES;
>  	dev_info->min_rx_bufsize = 0;
>  	dev_info->pci_dev = NULL;
>  	dev_info->speed_capa = tap_dev_speed_capa();
> @@ -673,9 +696,9 @@ enum ioctl_mode {
>  	unsigned long rx_nombuf = 0, ierrors = 0;
>  	const struct pmd_internals *pmd = dev->data->dev_private;
>  
> -	imax = (pmd->nb_queues < RTE_ETHDEV_QUEUE_STAT_CNTRS) ?
> -		pmd->nb_queues : RTE_ETHDEV_QUEUE_STAT_CNTRS;
> -
> +	/* rx queue statistics */
> +	imax = (dev->data->nb_rx_queues < RTE_ETHDEV_QUEUE_STAT_CNTRS) ?
> +		dev->data->nb_rx_queues : RTE_ETHDEV_QUEUE_STAT_CNTRS;
>  	for (i = 0; i < imax; i++) {
>  		tap_stats->q_ipackets[i] = pmd->rxq[i].stats.ipackets;
>  		tap_stats->q_ibytes[i] = pmd->rxq[i].stats.ibytes;
> @@ -683,7 +706,13 @@ enum ioctl_mode {
>  		rx_bytes_total += tap_stats->q_ibytes[i];
>  		rx_nombuf += pmd->rxq[i].stats.rx_nombuf;
>  		ierrors += pmd->rxq[i].stats.ierrors;
> +	}
>  
> +	/* tx queue statistics */
> +	imax = (dev->data->nb_tx_queues < RTE_ETHDEV_QUEUE_STAT_CNTRS) ?
> +		dev->data->nb_tx_queues : RTE_ETHDEV_QUEUE_STAT_CNTRS;
> +
> +	for (i = 0; i < imax; i++) {
>  		tap_stats->q_opackets[i] = pmd->txq[i].stats.opackets;
>  		tap_stats->q_errors[i] = pmd->txq[i].stats.errs;
>  		tap_stats->q_obytes[i] = pmd->txq[i].stats.obytes;
> @@ -707,7 +736,7 @@ enum ioctl_mode {
>  	int i;
>  	struct pmd_internals *pmd = dev->data->dev_private;
>  
> -	for (i = 0; i < pmd->nb_queues; i++) {
> +	for (i = 0; i < RTE_PMD_TAP_MAX_QUEUES; i++) {
>  		pmd->rxq[i].stats.ipackets = 0;
>  		pmd->rxq[i].stats.ibytes = 0;
>  		pmd->rxq[i].stats.ierrors = 0;
> @@ -729,11 +758,15 @@ enum ioctl_mode {
>  	tap_flow_flush(dev, NULL);
>  	tap_flow_implicit_flush(internals, NULL);
>  
> -	for (i = 0; i < internals->nb_queues; i++) {
> -		if (internals->rxq[i].fd != -1)
> +	for (i = 0; i < RTE_PMD_TAP_MAX_QUEUES; i++) {
> +		if (internals->rxq[i].fd != -1) {
>  			close(internals->rxq[i].fd);
> -		internals->rxq[i].fd = -1;
> -		internals->txq[i].fd = -1;
> +			internals->rxq[i].fd = -1;
> +		}
> +		if (internals->txq[i].fd != -1) {
> +			close(internals->txq[i].fd);
> +			internals->txq[i].fd = -1;
> +		}
>  	}
>  
>  	if (internals->remote_if_index) {
> @@ -887,30 +920,57 @@ enum ioctl_mode {
>  static int
>  tap_setup_queue(struct rte_eth_dev *dev,
>  		struct pmd_internals *internals,
> -		uint16_t qid)
> +		uint16_t qid,
> +		int is_rx)
>  {
> +	int *fd;
> +	int *other_fd;
> +	const char *dir;
>  	struct pmd_internals *pmd = dev->data->dev_private;
>  	struct rx_queue *rx = &internals->rxq[qid];
>  	struct tx_queue *tx = &internals->txq[qid];
> -	int fd = rx->fd == -1 ? tx->fd : rx->fd;
>  
> -	if (fd == -1) {
> -		RTE_LOG(INFO, PMD, "Add queue to TAP %s for qid %d\n",
> -			pmd->name, qid);
> -		fd = tun_alloc(pmd);
> -		if (fd < 0) {
> +	if (is_rx) {
> +		fd = &rx->fd;
> +		other_fd = &tx->fd;
> +		dir = "rx";
> +	} else {
> +		fd = &tx->fd;
> +		other_fd = &rx->fd;
> +		dir = "tx";
> +	}
> +	if (*fd != -1) {
> +		/* fd for this queue already exists */
> +		RTE_LOG(DEBUG, PMD, "%s: fd %d for %s queue qid %d exists\n",
> +			pmd->name, *fd, dir, qid);
> +	} else if (*other_fd != -1) {
> +		/* Only other_fd exists. dup it */
> +		*fd = dup(*other_fd);
> +		if (*fd < 0) {
> +			*fd = -1;
> +			RTE_LOG(ERR, PMD, "%s: dup() failed.\n",
> +				pmd->name);
> +			return -1;
> +		}
> +		RTE_LOG(DEBUG, PMD, "%s: dup fd %d for %s queue qid %d (%d)\n",
> +			pmd->name, *other_fd, dir, qid, *fd);
> +	} else {
> +		/* Both RX and TX fds do not exist (equal -1). Create fd */
> +		*fd = tun_alloc(pmd);
> +		if (*fd < 0) {
> +			*fd = -1; /* restore original value */
>  			RTE_LOG(ERR, PMD, "%s: tun_alloc() failed.\n",
>  				pmd->name);
>  			return -1;
>  		}
> +		RTE_LOG(DEBUG, PMD, "%s: add %s queue for qid %d fd %d\n",
> +			pmd->name, dir, qid, *fd);
>  	}
>  
> -	rx->fd = fd;
> -	tx->fd = fd;
>  	tx->mtu = &dev->data->mtu;
>  	rx->rxmode = &dev->data->dev_conf.rxmode;
>  
> -	return fd;
> +	return *fd;
>  }
>  
>  static int
> @@ -932,10 +992,10 @@ enum ioctl_mode {
>  	int fd;
>  	int i;
>  
> -	if ((rx_queue_id >= internals->nb_queues) || !mp) {
> +	if (rx_queue_id >= dev->data->nb_rx_queues || !mp) {
>  		RTE_LOG(WARNING, PMD,
> -			"nb_queues %d too small or mempool NULL\n",
> -			internals->nb_queues);
> +			"nb_rx_queues %d too small or mempool NULL\n",
> +			dev->data->nb_rx_queues);
>  		return -1;
>  	}
>  
> @@ -954,7 +1014,7 @@ enum ioctl_mode {
>  	rxq->iovecs = iovecs;
>  
>  	dev->data->rx_queues[rx_queue_id] = rxq;
> -	fd = tap_setup_queue(dev, internals, rx_queue_id);
> +	fd = tap_setup_queue(dev, internals, rx_queue_id, 1);
>  	if (fd == -1) {
>  		ret = fd;
>  		goto error;
> @@ -1002,11 +1062,11 @@ enum ioctl_mode {
>  	struct pmd_internals *internals = dev->data->dev_private;
>  	int ret;
>  
> -	if (tx_queue_id >= internals->nb_queues)
> +	if (tx_queue_id >= dev->data->nb_tx_queues)
>  		return -1;
>  
>  	dev->data->tx_queues[tx_queue_id] = &internals->txq[tx_queue_id];
> -	ret = tap_setup_queue(dev, internals, tx_queue_id);
> +	ret = tap_setup_queue(dev, internals, tx_queue_id, 0);
>  	if (ret == -1)
>  		return -1;
>  
> @@ -1166,7 +1226,6 @@ enum ioctl_mode {
>  	.filter_ctrl            = tap_dev_filter_ctrl,
>  };
>  
> -
>  static int
>  eth_dev_tap_create(struct rte_vdev_device *vdev, char *tap_name,
>  		   char *remote_iface, int fixed_mac_type)
> @@ -1193,8 +1252,8 @@ enum ioctl_mode {
>  	}
>  
>  	pmd = dev->data->dev_private;
> +	pmd->dev = dev;
>  	snprintf(pmd->name, sizeof(pmd->name), "%s", tap_name);
> -	pmd->nb_queues = RTE_PMD_TAP_MAX_QUEUES;
>  
>  	pmd->ioctl_sock = socket(AF_INET, SOCK_DGRAM, 0);
>  	if (pmd->ioctl_sock == -1) {
> @@ -1212,8 +1271,9 @@ enum ioctl_mode {
>  
>  	data->dev_link = pmd_link;
>  	data->mac_addrs = &pmd->eth_addr;
> -	data->nb_rx_queues = pmd->nb_queues;
> -	data->nb_tx_queues = pmd->nb_queues;
> +	/* Set the number of RX and TX queues */
> +	data->nb_rx_queues = 0;
> +	data->nb_tx_queues = 0;
>  
>  	dev->data = data;
>  	dev->dev_ops = &ops;
> @@ -1241,7 +1301,11 @@ enum ioctl_mode {
>  	}
>  
>  	/* Immediately create the netdevice (this will create the 1st queue). */
> -	if (tap_setup_queue(dev, pmd, 0) == -1)
> +	/* rx queue */
> +	if (tap_setup_queue(dev, pmd, 0, 1) == -1)
> +		goto error_exit;
> +	/* tx queue */
> +	if (tap_setup_queue(dev, pmd, 0, 0) == -1)
>  		goto error_exit;
>  
>  	ifr.ifr_mtu = dev->data->mtu;
> @@ -1515,9 +1579,16 @@ enum ioctl_mode {
>  		tap_flow_implicit_flush(internals, NULL);
>  		nl_final(internals->nlsk_fd);
>  	}
> -	for (i = 0; i < internals->nb_queues; i++)
> -		if (internals->rxq[i].fd != -1)
> +	for (i = 0; i < RTE_PMD_TAP_MAX_QUEUES; i++) {
> +		if (internals->rxq[i].fd != -1) {
>  			close(internals->rxq[i].fd);
> +			internals->rxq[i].fd = -1;
> +		}
> +		if (internals->txq[i].fd != -1) {
> +			close(internals->txq[i].fd);
> +			internals->txq[i].fd = -1;
> +		}
> +	}
>  
>  	close(internals->ioctl_sock);
>  	rte_free(eth_dev->data->dev_private);
> diff --git a/drivers/net/tap/rte_eth_tap.h b/drivers/net/tap/rte_eth_tap.h
> index 928a045..829f32f 100644
> --- a/drivers/net/tap/rte_eth_tap.h
> +++ b/drivers/net/tap/rte_eth_tap.h
> @@ -80,9 +80,9 @@ struct tx_queue {
>  };
>  
>  struct pmd_internals {
> +	struct rte_eth_dev *dev;          /* Ethernet device. */
>  	char remote_iface[RTE_ETH_NAME_MAX_LEN]; /* Remote netdevice name */
>  	char name[RTE_ETH_NAME_MAX_LEN];  /* Internal Tap device name */
> -	uint16_t nb_queues;               /* Number of queues supported */
>  	struct ether_addr eth_addr;       /* Mac address of the device port */
>  	struct ifreq remote_initial_flags;   /* Remote netdevice flags on init */
>  	int remote_if_index;              /* remote netdevice IF_INDEX */
> diff --git a/drivers/net/tap/tap_flow.c b/drivers/net/tap/tap_flow.c
> index 41f7345..eefa868 100644
> --- a/drivers/net/tap/tap_flow.c
> +++ b/drivers/net/tap/tap_flow.c
> @@ -1092,7 +1092,8 @@ struct tap_flow_items {
>  			if (action)
>  				goto exit_action_not_supported;
>  			action = 1;
> -			if (!queue || (queue->index >= pmd->nb_queues))
> +			if (!queue ||
> +			(queue->index > pmd->dev->data->nb_rx_queues - 1))
>  				goto exit_action_not_supported;
>  			if (flow)
>  				err = add_action_skbedit(flow, queue->index);

Re: [dpdk-dev] [dpdk-stable] [PATCH v3] net/tap: fix flow and port commands

[Ferruh Yigit]

On 9/18/2017 8:46 AM, Pascal Mazon wrote:

> 
> On 17/09/2017 00:32, Ophir Munk wrote:
>> This commit fixes two bugs related to tap devices. The first bug occurs
>> when executing in testpmd the following flow rule assuming tap device has
>> 4 rx and tx pair queues
>> "flow create 0 ingress pattern eth / end actions queue index 5 / end"
>> This command will report on success and will print ""Flow rule #0 created"
>> although it should have failed as queue index number 5 does not exist
>>
>> The second bug occurs when executing in testpmd "port start all" following
>> a port configuration. Assuming 1 pair of rx and tx queues an error is
>> reported: "Fail to start port 0"
>>
>> Before this commit a fixed max number (16) of rx and tx queue pairs were
>> created on startup where the file descriptors (fds) of rx and tx pairs were
>> identical.  As a result in the first bug queue index 5 existed because the
>> tap device was created with 16 rx and tx queue pairs regardless of the
>> configured number of queues. In the second bug when tap device was started
>> tx fd was closed before opening it and executing ioctl() on it. However
>> closing the sole fd of the device caused ioctl to fail with "No such
>> device".
>>
>> This commit creates the configured number of rx and tx queue pairs (up to
>> max 16) and assigns a unique fd to each queue. It was written to solve the
>> first bug and was found as the right fix for the second bug as well.
>>
>> Fixes: 02f96a0a82d1 ("net/tap: add TUN/TAP device PMD")
>> Fixes: bf7b7f437b49 ("net/tap: create netdevice during probing")
>> Fixes: de96fe68ae95 ("net/tap: add basic flow API patterns and actions")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Ophir Munk <ophirmu@mellanox.com>

> Hi,
>
> Your reasons to keep *dev in pmd_internals are good enough for me!
>
> The rest of the patch looks fine.
>
> Acked-by: Pascal Mazon <pascal.mazon@6wind.com>

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

(Welcome Ophir!)