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

[Pascal Mazon <pascal.mazon@6wind.com>]

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