[PATCH 0/5] improve tap behavior

[PATCH 0/5] improve tap behavior

[Pascal Mazon]

The tap does not behave properly in some cases.

It is generally expected that a real device should be available once the
probing has been done.

It is also better to check if an operation (here, setting MAC) is
mandatory before performing it. Typically in cases where the remote
netdevice is a VF with limited capabilities.

This series ensures that the tap works more logically.

Pascal Mazon (5):
  net/tap: add debug messages
  net/tap: remove unnecessary functions
  net/tap: drop unnecessary nested block
  net/tap: create netdevice during probing
  net/tap: do not set remote MAC if not necessary

 drivers/net/tap/rte_eth_tap.c | 299 +++++++++++++++++++++---------------------
 1 file changed, 153 insertions(+), 146 deletions(-)

-- 
2.12.0.306.g4a9b9b3

[PATCH 1/5] net/tap: add debug messages

[Pascal Mazon]

Print a detailed debug message inside tap_ioctl() directly. The caller
now only needs to check for return value.

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

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index b75663838844..0c89b63a2357 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -40,6 +40,7 @@
 #include <rte_vdev.h>
 #include <rte_kvargs.h>
 #include <rte_net.h>
+#include <rte_debug.h>
 
 #include <sys/types.h>
 #include <sys/stat.h>
@@ -444,6 +445,24 @@ pmd_tx_burst(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 	return num_tx;
 }
 
+static const char *
+tap_ioctl_req2str(unsigned long request)
+{
+	switch (request) {
+	case SIOCSIFFLAGS:
+		return "SIOCSIFFLAGS";
+	case SIOCGIFFLAGS:
+		return "SIOCGIFFLAGS";
+	case SIOCGIFHWADDR:
+		return "SIOCGIFHWADDR";
+	case SIOCSIFHWADDR:
+		return "SIOCSIFHWADDR";
+	case SIOCSIFMTU:
+		return "SIOCSIFMTU";
+	}
+	return "UNKNOWN";
+}
+
 static int
 tap_ioctl(struct pmd_internals *pmd, unsigned long request,
 	  struct ifreq *ifr, int set, enum ioctl_mode mode)
@@ -479,9 +498,7 @@ tap_ioctl(struct pmd_internals *pmd, unsigned long request,
 	case SIOCSIFMTU:
 		break;
 	default:
-		RTE_LOG(WARNING, PMD, "%s: ioctl() called with wrong arg\n",
-			pmd->name);
-		return -EINVAL;
+		RTE_ASSERT(!"unsupported request type: must not happen");
 	}
 	if (ioctl(pmd->ioctl_sock, request, ifr) < 0)
 		goto error;
@@ -490,8 +507,8 @@ tap_ioctl(struct pmd_internals *pmd, unsigned long request,
 	return 0;
 
 error:
-	RTE_LOG(ERR, PMD, "%s: ioctl(%lu) failed with error: %s\n",
-		ifr->ifr_name, request, strerror(errno));
+	RTE_LOG(DEBUG, PMD, "%s: %s(%s) failed: %s(%d)\n", ifr->ifr_name,
+		__func__, tap_ioctl_req2str(request), strerror(errno), errno);
 	return -errno;
 }
 
@@ -773,12 +790,8 @@ tap_mac_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr)
 		return;
 	}
 	/* Check the actual current MAC address on the tap netdevice */
-	if (tap_ioctl(pmd, SIOCGIFHWADDR, &ifr, 0, LOCAL_ONLY) != 0) {
-		RTE_LOG(ERR, PMD,
-			"%s: couldn't check current tap MAC address\n",
-			dev->data->name);
+	if (tap_ioctl(pmd, SIOCGIFHWADDR, &ifr, 0, LOCAL_ONLY) != 0)
 		return;
-	}
 	if (is_same_ether_addr((struct ether_addr *)&ifr.ifr_hwaddr.sa_data,
 			       mac_addr))
 		return;
@@ -1238,10 +1251,8 @@ eth_dev_tap_create(const char *name, char *tap_name, char *remote_iface,
 				remote_iface);
 			return 0;
 		}
-		if (tap_ioctl(pmd, SIOCGIFHWADDR, &ifr, 0, REMOTE_ONLY) < 0) {
-			RTE_LOG(ERR, PMD, "Could not get remote MAC address\n");
+		if (tap_ioctl(pmd, SIOCGIFHWADDR, &ifr, 0, REMOTE_ONLY) < 0)
 			goto error_exit;
-		}
 		rte_memcpy(&pmd->eth_addr, ifr.ifr_hwaddr.sa_data,
 			   ETHER_ADDR_LEN);
 	}
-- 
2.12.0.306.g4a9b9b3

[PATCH 2/5] net/tap: remove unnecessary functions

[Pascal Mazon]

These functions are only two lines each and are used only once.

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

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index 0c89b63a2357..c9d107c25ac2 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -860,26 +860,6 @@ tap_setup_queue(struct rte_eth_dev *dev,
 }
 
 static int
-rx_setup_queue(struct rte_eth_dev *dev,
-		struct pmd_internals *internals,
-		uint16_t qid)
-{
-	dev->data->rx_queues[qid] = &internals->rxq[qid];
-
-	return tap_setup_queue(dev, internals, qid);
-}
-
-static int
-tx_setup_queue(struct rte_eth_dev *dev,
-		struct pmd_internals *internals,
-		uint16_t qid)
-{
-	dev->data->tx_queues[qid] = &internals->txq[qid];
-
-	return tap_setup_queue(dev, internals, qid);
-}
-
-static int
 tap_rx_queue_setup(struct rte_eth_dev *dev,
 		   uint16_t rx_queue_id,
 		   uint16_t nb_rx_desc,
@@ -917,7 +897,8 @@ tap_rx_queue_setup(struct rte_eth_dev *dev,
 	}
 	rxq->iovecs = iovecs;
 
-	fd = rx_setup_queue(dev, internals, rx_queue_id);
+	dev->data->rx_queues[rx_queue_id] = rxq;
+	fd = tap_setup_queue(dev, internals, rx_queue_id);
 	if (fd == -1) {
 		ret = fd;
 		goto error;
@@ -968,7 +949,8 @@ tap_tx_queue_setup(struct rte_eth_dev *dev,
 	if (tx_queue_id >= internals->nb_queues)
 		return -1;
 
-	ret = tx_setup_queue(dev, internals, tx_queue_id);
+	dev->data->tx_queues[tx_queue_id] = &internals->txq[tx_queue_id];
+	ret = tap_setup_queue(dev, internals, tx_queue_id);
 	if (ret == -1)
 		return -1;
 
-- 
2.12.0.306.g4a9b9b3

[PATCH 3/5] net/tap: drop unnecessary nested block

[Pascal Mazon]

This is cosmetic; the code is functionally equivalent.

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

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index c9d107c25ac2..770f08ce12f5 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -826,28 +826,24 @@ tap_setup_queue(struct rte_eth_dev *dev,
 	struct tx_queue *tx = &internals->txq[qid];
 	int fd;
 
-	fd = rx->fd;
-	if (fd < 0) {
-		fd = tx->fd;
+	if (rx->fd == -1 || tx->fd == -1) {
+		RTE_LOG(INFO, PMD, "Add queue to TAP %s for qid %d\n",
+			pmd->name, qid);
+		fd = tun_alloc(pmd, qid);
 		if (fd < 0) {
-			RTE_LOG(INFO, PMD, "Add queue to TAP %s for qid %d\n",
+			RTE_LOG(ERR, PMD, "tun_alloc(%s, %d) failed\n",
 				pmd->name, qid);
-			fd = tun_alloc(pmd, qid);
-			if (fd < 0) {
-				RTE_LOG(ERR, PMD, "tun_alloc(%s, %d) failed\n",
-					pmd->name, qid);
+			return -1;
+		}
+		if (qid == 0) {
+			struct ifreq ifr;
+
+			ifr.ifr_mtu = dev->data->mtu;
+			if (tap_ioctl(pmd, SIOCSIFMTU, &ifr, 1,
+				      LOCAL_AND_REMOTE) < 0) {
+				close(fd);
 				return -1;
 			}
-			if (qid == 0) {
-				struct ifreq ifr;
-
-				ifr.ifr_mtu = dev->data->mtu;
-				if (tap_ioctl(pmd, SIOCSIFMTU, &ifr, 1,
-					      LOCAL_AND_REMOTE) < 0) {
-					close(fd);
-					return -1;
-				}
-			}
 		}
 	}
 
-- 
2.12.0.306.g4a9b9b3

[PATCH 4/5] net/tap: create netdevice during probing

[Pascal Mazon]

This has three main benefits:
 - tun_alloc is now generic again for any queue,
 - mtu no longer needs to be handled in tap_setup_queue(),
 - an actual netdevice is created as soon as the device is probed.

On top of it, code in eth_dev_tap_create() has been reworked to have a
more logical behavior; initialization can now fail if a remote is
requested but cannot be set up.

Fixes: 2bc06869cd94 ("net/tap: add remote netdevice traffic capture")

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

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index 770f08ce12f5..52bce08780ce 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -113,10 +113,6 @@ enum ioctl_mode {
 	REMOTE_ONLY,
 };
 
-static int
-tap_ioctl(struct pmd_internals *pmd, unsigned long request,
-	  struct ifreq *ifr, int set, enum ioctl_mode mode);
-
 static int tap_intr_handle_set(struct rte_eth_dev *dev, int set);
 
 /* Tun/Tap allocation routine
@@ -125,7 +121,7 @@ static int tap_intr_handle_set(struct rte_eth_dev *dev, int set);
  * supplied name.
  */
 static int
-tun_alloc(struct pmd_internals *pmd, uint16_t qid)
+tun_alloc(struct pmd_internals *pmd)
 {
 	struct ifreq ifr;
 #ifdef IFF_MULTI_QUEUE
@@ -224,75 +220,6 @@ tun_alloc(struct pmd_internals *pmd, uint16_t qid)
 			strerror(errno));
 	}
 
-	if (qid == 0) {
-		struct ifreq ifr;
-
-		/*
-		 * pmd->eth_addr contains the desired MAC, either from remote
-		 * or from a random assignment. Sync it with the tap netdevice.
-		 */
-		ifr.ifr_hwaddr.sa_family = AF_LOCAL;
-		rte_memcpy(ifr.ifr_hwaddr.sa_data, &pmd->eth_addr,
-			   ETHER_ADDR_LEN);
-		if (tap_ioctl(pmd, SIOCSIFHWADDR, &ifr, 0, LOCAL_ONLY) < 0)
-			goto error;
-
-		pmd->if_index = if_nametoindex(pmd->name);
-		if (!pmd->if_index) {
-			RTE_LOG(ERR, PMD,
-				"Could not find ifindex for %s: rte_flow won't be usable.\n",
-				pmd->name);
-			return fd;
-		}
-		if (!pmd->flower_support)
-			return fd;
-		if (qdisc_create_multiq(pmd->nlsk_fd, pmd->if_index) < 0) {
-			RTE_LOG(ERR, PMD,
-				"Could not create multiq qdisc for %s: rte_flow won't be usable.\n",
-				pmd->name);
-			return fd;
-		}
-		if (qdisc_create_ingress(pmd->nlsk_fd, pmd->if_index) < 0) {
-			RTE_LOG(ERR, PMD,
-				"Could not create multiq qdisc for %s: rte_flow won't be usable.\n",
-				pmd->name);
-			return fd;
-		}
-		if (pmd->remote_if_index) {
-			/*
-			 * Flush usually returns negative value because it tries
-			 * to delete every QDISC (and on a running device, one
-			 * QDISC at least is needed). Ignore negative return
-			 * value.
-			 */
-			qdisc_flush(pmd->nlsk_fd, pmd->remote_if_index);
-			if (qdisc_create_ingress(pmd->nlsk_fd,
-						 pmd->remote_if_index) < 0)
-				goto remote_fail;
-			LIST_INIT(&pmd->implicit_flows);
-			if (tap_flow_implicit_create(
-				    pmd, TAP_REMOTE_LOCAL_MAC) < 0)
-				goto remote_fail;
-			if (tap_flow_implicit_create(
-				    pmd, TAP_REMOTE_BROADCAST) < 0)
-				goto remote_fail;
-			if (tap_flow_implicit_create(
-				    pmd, TAP_REMOTE_BROADCASTV6) < 0)
-				goto remote_fail;
-			if (tap_flow_implicit_create(
-				    pmd, TAP_REMOTE_TX) < 0)
-				goto remote_fail;
-		}
-	}
-
-	return fd;
-
-remote_fail:
-	RTE_LOG(ERR, PMD,
-		"Could not set up remote flow rules for %s: remote disabled.\n",
-		pmd->name);
-	pmd->remote_if_index = 0;
-	tap_flow_implicit_flush(pmd, NULL);
 	return fd;
 
 error:
@@ -829,22 +756,12 @@ tap_setup_queue(struct rte_eth_dev *dev,
 	if (rx->fd == -1 || tx->fd == -1) {
 		RTE_LOG(INFO, PMD, "Add queue to TAP %s for qid %d\n",
 			pmd->name, qid);
-		fd = tun_alloc(pmd, qid);
+		fd = tun_alloc(pmd);
 		if (fd < 0) {
-			RTE_LOG(ERR, PMD, "tun_alloc(%s, %d) failed\n",
-				pmd->name, qid);
+			RTE_LOG(ERR, PMD, "%s: tun_alloc() failed.\n",
+				pmd->name);
 			return -1;
 		}
-		if (qid == 0) {
-			struct ifreq ifr;
-
-			ifr.ifr_mtu = dev->data->mtu;
-			if (tap_ioctl(pmd, SIOCSIFMTU, &ifr, 1,
-				      LOCAL_AND_REMOTE) < 0) {
-				close(fd);
-				return -1;
-			}
-		}
 	}
 
 	rx->fd = fd;
@@ -1132,6 +1049,7 @@ eth_dev_tap_create(const char *name, char *tap_name, char *remote_iface,
 	struct rte_eth_dev *dev = NULL;
 	struct pmd_internals *pmd = NULL;
 	struct rte_eth_dev_data *data = NULL;
+	struct ifreq ifr;
 	int i;
 
 	RTE_LOG(DEBUG, PMD, "  TAP device on numa %u\n", rte_socket_id());
@@ -1208,37 +1126,132 @@ eth_dev_tap_create(const char *name, char *tap_name, char *remote_iface,
 		eth_random_addr((uint8_t *)&pmd->eth_addr);
 	}
 
+	/* Immediately create the netdevice (this will create the 1st queue). */
+	if (tap_setup_queue(dev, pmd, 0) == -1)
+		goto error_exit;
+
+	ifr.ifr_mtu = dev->data->mtu;
+	if (tap_ioctl(pmd, SIOCSIFMTU, &ifr, 1, LOCAL_AND_REMOTE) < 0)
+		goto error_exit;
+
+	memset(&ifr, 0, sizeof(struct ifreq));
+	ifr.ifr_hwaddr.sa_family = AF_LOCAL;
+	rte_memcpy(ifr.ifr_hwaddr.sa_data, &pmd->eth_addr, ETHER_ADDR_LEN);
+	if (tap_ioctl(pmd, SIOCSIFHWADDR, &ifr, 0, LOCAL_ONLY) < 0)
+		goto error_exit;
+
 	tap_kernel_support(pmd);
-	if (!pmd->flower_support)
-		return 0;
-	LIST_INIT(&pmd->flows);
+	if (!pmd->flower_support) {
+		if (remote_iface[0]) {
+			RTE_LOG(ERR, PMD,
+				"%s: kernel does not support TC rules, required for remote feature.",
+				pmd->name);
+			goto error_exit;
+		} else {
+			RTE_LOG(INFO, PMD,
+				"%s: kernel too old for Flow API support.\n",
+				pmd->name);
+			return 0;
+		}
+	}
+
 	/*
-	 * If no netlink socket can be created, then it will fail when
-	 * creating/destroying flow rules.
+	 * Set up everything related to rte_flow:
+	 * - netlink socket
+	 * - tap / remote if_index
+	 * - mandatory QDISCs
+	 * - rte_flow actual/implicit lists
+	 * - implicit rules
 	 */
 	pmd->nlsk_fd = nl_init(0);
-	if (strlen(remote_iface)) {
-		struct ifreq ifr;
+	if (pmd->nlsk_fd == -1) {
+		RTE_LOG(WARNING, PMD, "%s: failed to create netlink socket.",
+			pmd->name);
+		goto disable_rte_flow;
+	}
+	pmd->if_index = if_nametoindex(pmd->name);
+	if (!pmd->if_index) {
+		RTE_LOG(ERR, PMD, "%s: failed to get if_index.", pmd->name);
+		goto disable_rte_flow;
+	}
+	if (qdisc_create_multiq(pmd->nlsk_fd, pmd->if_index) < 0) {
+		RTE_LOG(ERR, PMD, "%s: failed to create multiq qdisc.",
+			pmd->name);
+		goto disable_rte_flow;
+	}
+	if (qdisc_create_ingress(pmd->nlsk_fd, pmd->if_index) < 0) {
+		RTE_LOG(ERR, PMD, "%s: failed to create ingress qdisc.",
+			pmd->name);
+		goto disable_rte_flow;
+	}
+	LIST_INIT(&pmd->flows);
 
+	if (strlen(remote_iface)) {
 		pmd->remote_if_index = if_nametoindex(remote_iface);
+		if (!pmd->remote_if_index) {
+			RTE_LOG(ERR, PMD, "%s: failed to get %s if_index.",
+				pmd->name, remote_iface);
+			goto error_remote;
+		}
 		snprintf(pmd->remote_iface, RTE_ETH_NAME_MAX_LEN,
 			 "%s", remote_iface);
-		if (!pmd->remote_if_index) {
-			RTE_LOG(ERR, PMD, "Could not find %s ifindex: "
-				"remote interface will remain unconfigured\n",
-				remote_iface);
-			return 0;
+		if (tap_ioctl(pmd, SIOCGIFHWADDR, &ifr, 0, REMOTE_ONLY) < 0) {
+			RTE_LOG(ERR, PMD, "%s: failed to get %s MAC address.",
+				pmd->name, pmd->remote_iface);
+			goto error_remote;
 		}
-		if (tap_ioctl(pmd, SIOCGIFHWADDR, &ifr, 0, REMOTE_ONLY) < 0)
-			goto error_exit;
 		rte_memcpy(&pmd->eth_addr, ifr.ifr_hwaddr.sa_data,
 			   ETHER_ADDR_LEN);
+		/* The desired MAC is already in ifreq after SIOCGIFHWADDR. */
+		if (tap_ioctl(pmd, SIOCSIFHWADDR, &ifr, 0, LOCAL_ONLY) < 0) {
+			RTE_LOG(ERR, PMD, "%s: failed to get %s MAC address.",
+				pmd->name, remote_iface);
+			goto error_remote;
+		}
+
+		/*
+		 * Flush usually returns negative value because it tries to
+		 * delete every QDISC (and on a running device, one QDISC at
+		 * least is needed). Ignore negative return value.
+		 */
+		qdisc_flush(pmd->nlsk_fd, pmd->remote_if_index);
+		if (qdisc_create_ingress(pmd->nlsk_fd,
+					 pmd->remote_if_index) < 0) {
+			RTE_LOG(ERR, PMD, "%s: failed to create ingress qdisc.",
+				pmd->remote_iface);
+			goto error_remote;
+		}
+		LIST_INIT(&pmd->implicit_flows);
+		if (tap_flow_implicit_create(pmd, TAP_REMOTE_TX) < 0 ||
+		    tap_flow_implicit_create(pmd, TAP_REMOTE_LOCAL_MAC) < 0 ||
+		    tap_flow_implicit_create(pmd, TAP_REMOTE_BROADCAST) < 0 ||
+		    tap_flow_implicit_create(pmd, TAP_REMOTE_BROADCASTV6) < 0) {
+			RTE_LOG(ERR, PMD,
+				"%s: failed to create implicit rules.",
+				pmd->name);
+			goto error_remote;
+		}
 	}
 
 	return 0;
 
+disable_rte_flow:
+	RTE_LOG(ERR, PMD, " Disabling rte flow support: %s(%d)\n",
+		strerror(errno), errno);
+	if (strlen(remote_iface)) {
+		RTE_LOG(ERR, PMD, "Remote feature requires flow support.\n");
+		goto error_exit;
+	}
+	pmd->flower_support = 0;
+	return 0;
+
+error_remote:
+	RTE_LOG(ERR, PMD, " Can't set up remote feature: %s(%d)\n",
+		strerror(errno), errno);
+	tap_flow_implicit_flush(pmd, NULL);
+
 error_exit:
-	RTE_LOG(DEBUG, PMD, "TAP Unable to initialize %s\n", name);
+	RTE_LOG(ERR, PMD, "TAP Unable to initialize %s\n", name);
 
 	rte_free(data);
 	rte_free(pmd);
-- 
2.12.0.306.g4a9b9b3

[PATCH 5/5] net/tap: do not set remote MAC if not necessary

[Pascal Mazon]

Check for the current MAC address on both the remote and the tap
netdevices before setting a new value.

While there, remove wrong empty lines and ensure tap_ioctl() return
value is negative, just like what is done throughout this code.

Fixes: 2bc06869cd94 ("net/tap: add remote netdevice traffic capture")

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

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index 52bce08780ce..41276eacfec3 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -704,11 +704,11 @@ tap_allmulti_disable(struct rte_eth_dev *dev)
 		tap_flow_implicit_destroy(pmd, TAP_REMOTE_ALLMULTI);
 }
 
-
 static void
 tap_mac_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr)
 {
 	struct pmd_internals *pmd = dev->data->dev_private;
+	enum ioctl_mode mode = LOCAL_ONLY;
 	struct ifreq ifr;
 
 	if (is_zero_ether_addr(mac_addr)) {
@@ -717,15 +717,20 @@ tap_mac_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr)
 		return;
 	}
 	/* Check the actual current MAC address on the tap netdevice */
-	if (tap_ioctl(pmd, SIOCGIFHWADDR, &ifr, 0, LOCAL_ONLY) != 0)
+	if (tap_ioctl(pmd, SIOCGIFHWADDR, &ifr, 0, LOCAL_ONLY) < 0)
 		return;
 	if (is_same_ether_addr((struct ether_addr *)&ifr.ifr_hwaddr.sa_data,
 			       mac_addr))
 		return;
-
+	/* Check the current MAC address on the remote */
+	if (tap_ioctl(pmd, SIOCGIFHWADDR, &ifr, 0, REMOTE_ONLY) < 0)
+		return;
+	if (!is_same_ether_addr((struct ether_addr *)&ifr.ifr_hwaddr.sa_data,
+			       mac_addr))
+		mode = LOCAL_AND_REMOTE;
 	ifr.ifr_hwaddr.sa_family = AF_LOCAL;
 	rte_memcpy(ifr.ifr_hwaddr.sa_data, mac_addr, ETHER_ADDR_LEN);
-	if (tap_ioctl(pmd, SIOCSIFHWADDR, &ifr, 1, LOCAL_AND_REMOTE) < 0)
+	if (tap_ioctl(pmd, SIOCSIFHWADDR, &ifr, 1, mode) < 0)
 		return;
 	rte_memcpy(&pmd->eth_addr, mac_addr, ETHER_ADDR_LEN);
 	if (pmd->remote_if_index) {
-- 
2.12.0.306.g4a9b9b3

Re: [PATCH 0/5] improve tap behavior

[Ferruh Yigit]

On 4/14/2017 10:32 AM, Pascal Mazon wrote:
> The tap does not behave properly in some cases.
> 
> It is generally expected that a real device should be available once the
> probing has been done.
> 
> It is also better to check if an operation (here, setting MAC) is
> mandatory before performing it. Typically in cases where the remote
> netdevice is a VF with limited capabilities.
> 
> This series ensures that the tap works more logically.
> 
> Pascal Mazon (5):
>   net/tap: add debug messages
>   net/tap: remove unnecessary functions
>   net/tap: drop unnecessary nested block
>   net/tap: create netdevice during probing
>   net/tap: do not set remote MAC if not necessary

Thanks for the patches, they will be considered for 17.08

[PATCH 0/5] improve tap behavior

[Pascal Mazon]

The tap does not behave properly in some cases.

It is generally expected that a real device should be available once the
probing has been done.

It is also better to check if an operation (here, setting MAC) is
mandatory before performing it. Typically in cases where the remote
netdevice is a VF with limited capabilities.

This series ensures that the tap works more logically.

v2 changes:
  - fix uninitialized fd variable

Pascal Mazon (5):
  net/tap: add debug messages
  net/tap: remove unnecessary functions
  net/tap: drop unnecessary nested block
  net/tap: create netdevice during probing
  net/tap: do not set remote MAC if not necessary

 drivers/net/tap/rte_eth_tap.c | 301 +++++++++++++++++++++---------------------
 1 file changed, 154 insertions(+), 147 deletions(-)

-- 
2.12.0.306.g4a9b9b3

[PATCH v2 1/5] net/tap: add debug messages

[Pascal Mazon]

Print a detailed debug message inside tap_ioctl() directly. The caller
now only needs to check for return value.

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

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index b75663838844..0c89b63a2357 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -40,6 +40,7 @@
 #include <rte_vdev.h>
 #include <rte_kvargs.h>
 #include <rte_net.h>
+#include <rte_debug.h>
 
 #include <sys/types.h>
 #include <sys/stat.h>
@@ -444,6 +445,24 @@ pmd_tx_burst(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 	return num_tx;
 }
 
+static const char *
+tap_ioctl_req2str(unsigned long request)
+{
+	switch (request) {
+	case SIOCSIFFLAGS:
+		return "SIOCSIFFLAGS";
+	case SIOCGIFFLAGS:
+		return "SIOCGIFFLAGS";
+	case SIOCGIFHWADDR:
+		return "SIOCGIFHWADDR";
+	case SIOCSIFHWADDR:
+		return "SIOCSIFHWADDR";
+	case SIOCSIFMTU:
+		return "SIOCSIFMTU";
+	}
+	return "UNKNOWN";
+}
+
 static int
 tap_ioctl(struct pmd_internals *pmd, unsigned long request,
 	  struct ifreq *ifr, int set, enum ioctl_mode mode)
@@ -479,9 +498,7 @@ tap_ioctl(struct pmd_internals *pmd, unsigned long request,
 	case SIOCSIFMTU:
 		break;
 	default:
-		RTE_LOG(WARNING, PMD, "%s: ioctl() called with wrong arg\n",
-			pmd->name);
-		return -EINVAL;
+		RTE_ASSERT(!"unsupported request type: must not happen");
 	}
 	if (ioctl(pmd->ioctl_sock, request, ifr) < 0)
 		goto error;
@@ -490,8 +507,8 @@ tap_ioctl(struct pmd_internals *pmd, unsigned long request,
 	return 0;
 
 error:
-	RTE_LOG(ERR, PMD, "%s: ioctl(%lu) failed with error: %s\n",
-		ifr->ifr_name, request, strerror(errno));
+	RTE_LOG(DEBUG, PMD, "%s: %s(%s) failed: %s(%d)\n", ifr->ifr_name,
+		__func__, tap_ioctl_req2str(request), strerror(errno), errno);
 	return -errno;
 }
 
@@ -773,12 +790,8 @@ tap_mac_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr)
 		return;
 	}
 	/* Check the actual current MAC address on the tap netdevice */
-	if (tap_ioctl(pmd, SIOCGIFHWADDR, &ifr, 0, LOCAL_ONLY) != 0) {
-		RTE_LOG(ERR, PMD,
-			"%s: couldn't check current tap MAC address\n",
-			dev->data->name);
+	if (tap_ioctl(pmd, SIOCGIFHWADDR, &ifr, 0, LOCAL_ONLY) != 0)
 		return;
-	}
 	if (is_same_ether_addr((struct ether_addr *)&ifr.ifr_hwaddr.sa_data,
 			       mac_addr))
 		return;
@@ -1238,10 +1251,8 @@ eth_dev_tap_create(const char *name, char *tap_name, char *remote_iface,
 				remote_iface);
 			return 0;
 		}
-		if (tap_ioctl(pmd, SIOCGIFHWADDR, &ifr, 0, REMOTE_ONLY) < 0) {
-			RTE_LOG(ERR, PMD, "Could not get remote MAC address\n");
+		if (tap_ioctl(pmd, SIOCGIFHWADDR, &ifr, 0, REMOTE_ONLY) < 0)
 			goto error_exit;
-		}
 		rte_memcpy(&pmd->eth_addr, ifr.ifr_hwaddr.sa_data,
 			   ETHER_ADDR_LEN);
 	}
-- 
2.12.0.306.g4a9b9b3

[PATCH v2 2/5] net/tap: remove unnecessary functions

[Pascal Mazon]

These functions are only two lines each and are used only once.

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

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index 0c89b63a2357..c9d107c25ac2 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -860,26 +860,6 @@ tap_setup_queue(struct rte_eth_dev *dev,
 }
 
 static int
-rx_setup_queue(struct rte_eth_dev *dev,
-		struct pmd_internals *internals,
-		uint16_t qid)
-{
-	dev->data->rx_queues[qid] = &internals->rxq[qid];
-
-	return tap_setup_queue(dev, internals, qid);
-}
-
-static int
-tx_setup_queue(struct rte_eth_dev *dev,
-		struct pmd_internals *internals,
-		uint16_t qid)
-{
-	dev->data->tx_queues[qid] = &internals->txq[qid];
-
-	return tap_setup_queue(dev, internals, qid);
-}
-
-static int
 tap_rx_queue_setup(struct rte_eth_dev *dev,
 		   uint16_t rx_queue_id,
 		   uint16_t nb_rx_desc,
@@ -917,7 +897,8 @@ tap_rx_queue_setup(struct rte_eth_dev *dev,
 	}
 	rxq->iovecs = iovecs;
 
-	fd = rx_setup_queue(dev, internals, rx_queue_id);
+	dev->data->rx_queues[rx_queue_id] = rxq;
+	fd = tap_setup_queue(dev, internals, rx_queue_id);
 	if (fd == -1) {
 		ret = fd;
 		goto error;
@@ -968,7 +949,8 @@ tap_tx_queue_setup(struct rte_eth_dev *dev,
 	if (tx_queue_id >= internals->nb_queues)
 		return -1;
 
-	ret = tx_setup_queue(dev, internals, tx_queue_id);
+	dev->data->tx_queues[tx_queue_id] = &internals->txq[tx_queue_id];
+	ret = tap_setup_queue(dev, internals, tx_queue_id);
 	if (ret == -1)
 		return -1;
 
-- 
2.12.0.306.g4a9b9b3

[PATCH v2 3/5] net/tap: drop unnecessary nested block

[Pascal Mazon]

This is cosmetic; the code is functionally equivalent.

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

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index c9d107c25ac2..403aca9f7307 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -824,30 +824,26 @@ tap_setup_queue(struct rte_eth_dev *dev,
 	struct pmd_internals *pmd = dev->data->dev_private;
 	struct rx_queue *rx = &internals->rxq[qid];
 	struct tx_queue *tx = &internals->txq[qid];
-	int fd;
+	int fd = rx->fd == -1 ? tx->fd : rx->fd;
 
-	fd = rx->fd;
-	if (fd < 0) {
-		fd = tx->fd;
+	if (fd == -1) {
+		RTE_LOG(INFO, PMD, "Add queue to TAP %s for qid %d\n",
+			pmd->name, qid);
+		fd = tun_alloc(pmd, qid);
 		if (fd < 0) {
-			RTE_LOG(INFO, PMD, "Add queue to TAP %s for qid %d\n",
+			RTE_LOG(ERR, PMD, "tun_alloc(%s, %d) failed\n",
 				pmd->name, qid);
-			fd = tun_alloc(pmd, qid);
-			if (fd < 0) {
-				RTE_LOG(ERR, PMD, "tun_alloc(%s, %d) failed\n",
-					pmd->name, qid);
+			return -1;
+		}
+		if (qid == 0) {
+			struct ifreq ifr;
+
+			ifr.ifr_mtu = dev->data->mtu;
+			if (tap_ioctl(pmd, SIOCSIFMTU, &ifr, 1,
+				      LOCAL_AND_REMOTE) < 0) {
+				close(fd);
 				return -1;
 			}
-			if (qid == 0) {
-				struct ifreq ifr;
-
-				ifr.ifr_mtu = dev->data->mtu;
-				if (tap_ioctl(pmd, SIOCSIFMTU, &ifr, 1,
-					      LOCAL_AND_REMOTE) < 0) {
-					close(fd);
-					return -1;
-				}
-			}
 		}
 	}
 
-- 
2.12.0.306.g4a9b9b3

[PATCH v2 4/5] net/tap: create netdevice during probing

[Pascal Mazon]

This has three main benefits:
 - tun_alloc is now generic again for any queue,
 - mtu no longer needs to be handled in tap_setup_queue(),
 - an actual netdevice is created as soon as the device is probed.

On top of it, code in eth_dev_tap_create() has been reworked to have a
more logical behavior; initialization can now fail if a remote is
requested but cannot be set up.

Fixes: 2bc06869cd94 ("net/tap: add remote netdevice traffic capture")

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

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index 403aca9f7307..24ad9ead3b4c 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -113,10 +113,6 @@ enum ioctl_mode {
 	REMOTE_ONLY,
 };
 
-static int
-tap_ioctl(struct pmd_internals *pmd, unsigned long request,
-	  struct ifreq *ifr, int set, enum ioctl_mode mode);
-
 static int tap_intr_handle_set(struct rte_eth_dev *dev, int set);
 
 /* Tun/Tap allocation routine
@@ -125,7 +121,7 @@ static int tap_intr_handle_set(struct rte_eth_dev *dev, int set);
  * supplied name.
  */
 static int
-tun_alloc(struct pmd_internals *pmd, uint16_t qid)
+tun_alloc(struct pmd_internals *pmd)
 {
 	struct ifreq ifr;
 #ifdef IFF_MULTI_QUEUE
@@ -224,75 +220,6 @@ tun_alloc(struct pmd_internals *pmd, uint16_t qid)
 			strerror(errno));
 	}
 
-	if (qid == 0) {
-		struct ifreq ifr;
-
-		/*
-		 * pmd->eth_addr contains the desired MAC, either from remote
-		 * or from a random assignment. Sync it with the tap netdevice.
-		 */
-		ifr.ifr_hwaddr.sa_family = AF_LOCAL;
-		rte_memcpy(ifr.ifr_hwaddr.sa_data, &pmd->eth_addr,
-			   ETHER_ADDR_LEN);
-		if (tap_ioctl(pmd, SIOCSIFHWADDR, &ifr, 0, LOCAL_ONLY) < 0)
-			goto error;
-
-		pmd->if_index = if_nametoindex(pmd->name);
-		if (!pmd->if_index) {
-			RTE_LOG(ERR, PMD,
-				"Could not find ifindex for %s: rte_flow won't be usable.\n",
-				pmd->name);
-			return fd;
-		}
-		if (!pmd->flower_support)
-			return fd;
-		if (qdisc_create_multiq(pmd->nlsk_fd, pmd->if_index) < 0) {
-			RTE_LOG(ERR, PMD,
-				"Could not create multiq qdisc for %s: rte_flow won't be usable.\n",
-				pmd->name);
-			return fd;
-		}
-		if (qdisc_create_ingress(pmd->nlsk_fd, pmd->if_index) < 0) {
-			RTE_LOG(ERR, PMD,
-				"Could not create multiq qdisc for %s: rte_flow won't be usable.\n",
-				pmd->name);
-			return fd;
-		}
-		if (pmd->remote_if_index) {
-			/*
-			 * Flush usually returns negative value because it tries
-			 * to delete every QDISC (and on a running device, one
-			 * QDISC at least is needed). Ignore negative return
-			 * value.
-			 */
-			qdisc_flush(pmd->nlsk_fd, pmd->remote_if_index);
-			if (qdisc_create_ingress(pmd->nlsk_fd,
-						 pmd->remote_if_index) < 0)
-				goto remote_fail;
-			LIST_INIT(&pmd->implicit_flows);
-			if (tap_flow_implicit_create(
-				    pmd, TAP_REMOTE_LOCAL_MAC) < 0)
-				goto remote_fail;
-			if (tap_flow_implicit_create(
-				    pmd, TAP_REMOTE_BROADCAST) < 0)
-				goto remote_fail;
-			if (tap_flow_implicit_create(
-				    pmd, TAP_REMOTE_BROADCASTV6) < 0)
-				goto remote_fail;
-			if (tap_flow_implicit_create(
-				    pmd, TAP_REMOTE_TX) < 0)
-				goto remote_fail;
-		}
-	}
-
-	return fd;
-
-remote_fail:
-	RTE_LOG(ERR, PMD,
-		"Could not set up remote flow rules for %s: remote disabled.\n",
-		pmd->name);
-	pmd->remote_if_index = 0;
-	tap_flow_implicit_flush(pmd, NULL);
 	return fd;
 
 error:
@@ -829,22 +756,12 @@ tap_setup_queue(struct rte_eth_dev *dev,
 	if (fd == -1) {
 		RTE_LOG(INFO, PMD, "Add queue to TAP %s for qid %d\n",
 			pmd->name, qid);
-		fd = tun_alloc(pmd, qid);
+		fd = tun_alloc(pmd);
 		if (fd < 0) {
-			RTE_LOG(ERR, PMD, "tun_alloc(%s, %d) failed\n",
-				pmd->name, qid);
+			RTE_LOG(ERR, PMD, "%s: tun_alloc() failed.\n",
+				pmd->name);
 			return -1;
 		}
-		if (qid == 0) {
-			struct ifreq ifr;
-
-			ifr.ifr_mtu = dev->data->mtu;
-			if (tap_ioctl(pmd, SIOCSIFMTU, &ifr, 1,
-				      LOCAL_AND_REMOTE) < 0) {
-				close(fd);
-				return -1;
-			}
-		}
 	}
 
 	rx->fd = fd;
@@ -1132,6 +1049,7 @@ eth_dev_tap_create(const char *name, char *tap_name, char *remote_iface,
 	struct rte_eth_dev *dev = NULL;
 	struct pmd_internals *pmd = NULL;
 	struct rte_eth_dev_data *data = NULL;
+	struct ifreq ifr;
 	int i;
 
 	RTE_LOG(DEBUG, PMD, "  TAP device on numa %u\n", rte_socket_id());
@@ -1208,37 +1126,132 @@ eth_dev_tap_create(const char *name, char *tap_name, char *remote_iface,
 		eth_random_addr((uint8_t *)&pmd->eth_addr);
 	}
 
+	/* Immediately create the netdevice (this will create the 1st queue). */
+	if (tap_setup_queue(dev, pmd, 0) == -1)
+		goto error_exit;
+
+	ifr.ifr_mtu = dev->data->mtu;
+	if (tap_ioctl(pmd, SIOCSIFMTU, &ifr, 1, LOCAL_AND_REMOTE) < 0)
+		goto error_exit;
+
+	memset(&ifr, 0, sizeof(struct ifreq));
+	ifr.ifr_hwaddr.sa_family = AF_LOCAL;
+	rte_memcpy(ifr.ifr_hwaddr.sa_data, &pmd->eth_addr, ETHER_ADDR_LEN);
+	if (tap_ioctl(pmd, SIOCSIFHWADDR, &ifr, 0, LOCAL_ONLY) < 0)
+		goto error_exit;
+
 	tap_kernel_support(pmd);
-	if (!pmd->flower_support)
-		return 0;
-	LIST_INIT(&pmd->flows);
+	if (!pmd->flower_support) {
+		if (remote_iface[0]) {
+			RTE_LOG(ERR, PMD,
+				"%s: kernel does not support TC rules, required for remote feature.",
+				pmd->name);
+			goto error_exit;
+		} else {
+			RTE_LOG(INFO, PMD,
+				"%s: kernel too old for Flow API support.\n",
+				pmd->name);
+			return 0;
+		}
+	}
+
 	/*
-	 * If no netlink socket can be created, then it will fail when
-	 * creating/destroying flow rules.
+	 * Set up everything related to rte_flow:
+	 * - netlink socket
+	 * - tap / remote if_index
+	 * - mandatory QDISCs
+	 * - rte_flow actual/implicit lists
+	 * - implicit rules
 	 */
 	pmd->nlsk_fd = nl_init(0);
-	if (strlen(remote_iface)) {
-		struct ifreq ifr;
+	if (pmd->nlsk_fd == -1) {
+		RTE_LOG(WARNING, PMD, "%s: failed to create netlink socket.",
+			pmd->name);
+		goto disable_rte_flow;
+	}
+	pmd->if_index = if_nametoindex(pmd->name);
+	if (!pmd->if_index) {
+		RTE_LOG(ERR, PMD, "%s: failed to get if_index.", pmd->name);
+		goto disable_rte_flow;
+	}
+	if (qdisc_create_multiq(pmd->nlsk_fd, pmd->if_index) < 0) {
+		RTE_LOG(ERR, PMD, "%s: failed to create multiq qdisc.",
+			pmd->name);
+		goto disable_rte_flow;
+	}
+	if (qdisc_create_ingress(pmd->nlsk_fd, pmd->if_index) < 0) {
+		RTE_LOG(ERR, PMD, "%s: failed to create ingress qdisc.",
+			pmd->name);
+		goto disable_rte_flow;
+	}
+	LIST_INIT(&pmd->flows);
 
+	if (strlen(remote_iface)) {
 		pmd->remote_if_index = if_nametoindex(remote_iface);
+		if (!pmd->remote_if_index) {
+			RTE_LOG(ERR, PMD, "%s: failed to get %s if_index.",
+				pmd->name, remote_iface);
+			goto error_remote;
+		}
 		snprintf(pmd->remote_iface, RTE_ETH_NAME_MAX_LEN,
 			 "%s", remote_iface);
-		if (!pmd->remote_if_index) {
-			RTE_LOG(ERR, PMD, "Could not find %s ifindex: "
-				"remote interface will remain unconfigured\n",
-				remote_iface);
-			return 0;
+		if (tap_ioctl(pmd, SIOCGIFHWADDR, &ifr, 0, REMOTE_ONLY) < 0) {
+			RTE_LOG(ERR, PMD, "%s: failed to get %s MAC address.",
+				pmd->name, pmd->remote_iface);
+			goto error_remote;
 		}
-		if (tap_ioctl(pmd, SIOCGIFHWADDR, &ifr, 0, REMOTE_ONLY) < 0)
-			goto error_exit;
 		rte_memcpy(&pmd->eth_addr, ifr.ifr_hwaddr.sa_data,
 			   ETHER_ADDR_LEN);
+		/* The desired MAC is already in ifreq after SIOCGIFHWADDR. */
+		if (tap_ioctl(pmd, SIOCSIFHWADDR, &ifr, 0, LOCAL_ONLY) < 0) {
+			RTE_LOG(ERR, PMD, "%s: failed to get %s MAC address.",
+				pmd->name, remote_iface);
+			goto error_remote;
+		}
+
+		/*
+		 * Flush usually returns negative value because it tries to
+		 * delete every QDISC (and on a running device, one QDISC at
+		 * least is needed). Ignore negative return value.
+		 */
+		qdisc_flush(pmd->nlsk_fd, pmd->remote_if_index);
+		if (qdisc_create_ingress(pmd->nlsk_fd,
+					 pmd->remote_if_index) < 0) {
+			RTE_LOG(ERR, PMD, "%s: failed to create ingress qdisc.",
+				pmd->remote_iface);
+			goto error_remote;
+		}
+		LIST_INIT(&pmd->implicit_flows);
+		if (tap_flow_implicit_create(pmd, TAP_REMOTE_TX) < 0 ||
+		    tap_flow_implicit_create(pmd, TAP_REMOTE_LOCAL_MAC) < 0 ||
+		    tap_flow_implicit_create(pmd, TAP_REMOTE_BROADCAST) < 0 ||
+		    tap_flow_implicit_create(pmd, TAP_REMOTE_BROADCASTV6) < 0) {
+			RTE_LOG(ERR, PMD,
+				"%s: failed to create implicit rules.",
+				pmd->name);
+			goto error_remote;
+		}
 	}
 
 	return 0;
 
+disable_rte_flow:
+	RTE_LOG(ERR, PMD, " Disabling rte flow support: %s(%d)\n",
+		strerror(errno), errno);
+	if (strlen(remote_iface)) {
+		RTE_LOG(ERR, PMD, "Remote feature requires flow support.\n");
+		goto error_exit;
+	}
+	pmd->flower_support = 0;
+	return 0;
+
+error_remote:
+	RTE_LOG(ERR, PMD, " Can't set up remote feature: %s(%d)\n",
+		strerror(errno), errno);
+	tap_flow_implicit_flush(pmd, NULL);
+
 error_exit:
-	RTE_LOG(DEBUG, PMD, "TAP Unable to initialize %s\n", name);
+	RTE_LOG(ERR, PMD, "TAP Unable to initialize %s\n", name);
 
 	rte_free(data);
 	rte_free(pmd);
-- 
2.12.0.306.g4a9b9b3

[PATCH v2 5/5] net/tap: do not set remote MAC if not necessary

[Pascal Mazon]

Check for the current MAC address on both the remote and the tap
netdevices before setting a new value.

While there, remove wrong empty lines and ensure tap_ioctl() return
value is negative, just like what is done throughout this code.

Fixes: 2bc06869cd94 ("net/tap: add remote netdevice traffic capture")

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

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index 24ad9ead3b4c..58c1b4cb3ddf 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -704,11 +704,11 @@ tap_allmulti_disable(struct rte_eth_dev *dev)
 		tap_flow_implicit_destroy(pmd, TAP_REMOTE_ALLMULTI);
 }
 
-
 static void
 tap_mac_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr)
 {
 	struct pmd_internals *pmd = dev->data->dev_private;
+	enum ioctl_mode mode = LOCAL_ONLY;
 	struct ifreq ifr;
 
 	if (is_zero_ether_addr(mac_addr)) {
@@ -717,15 +717,20 @@ tap_mac_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr)
 		return;
 	}
 	/* Check the actual current MAC address on the tap netdevice */
-	if (tap_ioctl(pmd, SIOCGIFHWADDR, &ifr, 0, LOCAL_ONLY) != 0)
+	if (tap_ioctl(pmd, SIOCGIFHWADDR, &ifr, 0, LOCAL_ONLY) < 0)
 		return;
 	if (is_same_ether_addr((struct ether_addr *)&ifr.ifr_hwaddr.sa_data,
 			       mac_addr))
 		return;
-
+	/* Check the current MAC address on the remote */
+	if (tap_ioctl(pmd, SIOCGIFHWADDR, &ifr, 0, REMOTE_ONLY) < 0)
+		return;
+	if (!is_same_ether_addr((struct ether_addr *)&ifr.ifr_hwaddr.sa_data,
+			       mac_addr))
+		mode = LOCAL_AND_REMOTE;
 	ifr.ifr_hwaddr.sa_family = AF_LOCAL;
 	rte_memcpy(ifr.ifr_hwaddr.sa_data, mac_addr, ETHER_ADDR_LEN);
-	if (tap_ioctl(pmd, SIOCSIFHWADDR, &ifr, 1, LOCAL_AND_REMOTE) < 0)
+	if (tap_ioctl(pmd, SIOCSIFHWADDR, &ifr, 1, mode) < 0)
 		return;
 	rte_memcpy(&pmd->eth_addr, mac_addr, ETHER_ADDR_LEN);
 	if (pmd->remote_if_index) {
-- 
2.12.0.306.g4a9b9b3

Re: [PATCH 0/5] improve tap behavior

[Ferruh Yigit]

On 4/18/2017 9:17 AM, Pascal Mazon wrote:
> The tap does not behave properly in some cases.
> 
> It is generally expected that a real device should be available once the
> probing has been done.
> 
> It is also better to check if an operation (here, setting MAC) is
> mandatory before performing it. Typically in cases where the remote
> netdevice is a VF with limited capabilities.
> 
> This series ensures that the tap works more logically.
> 
> v2 changes:
>   - fix uninitialized fd variable
> 
> Pascal Mazon (5):
>   net/tap: add debug messages
>   net/tap: remove unnecessary functions
>   net/tap: drop unnecessary nested block
>   net/tap: create netdevice during probing
>   net/tap: do not set remote MAC if not necessary

Hi Pascal,

Can you please rebase the patchset on top of latest next-net?

Thanks,
ferruh

[PATCH v3 0/5] improve tap behavior

[Pascal Mazon]

The tap does not behave properly in some cases.

It is generally expected that a real device should be available once the
probing has been done.

It is also better to check if an operation (here, setting MAC) is
mandatory before performing it. Typically in cases where the remote
netdevice is a VF with limited capabilities.

This series ensures that the tap works more logically.

v3 changes:
  - rebase on top of next-net/master

v2 changes:
  - fix uninitialized fd variable

Pascal Mazon (5):
  net/tap: add debug messages
  net/tap: remove unnecessary functions
  net/tap: drop unnecessary nested block
  net/tap: create netdevice during probing
  net/tap: do not set remote MAC if not necessary

 drivers/net/tap/rte_eth_tap.c | 301 +++++++++++++++++++++---------------------
 1 file changed, 154 insertions(+), 147 deletions(-)

-- 
2.12.0.306.g4a9b9b3

[PATCH v3 1/5] net/tap: add debug messages

[Pascal Mazon]

Print a detailed debug message inside tap_ioctl() directly. The caller
now only needs to check for return value.

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

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index f1bb55e9769b..3d08ef2ca4d4 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -41,6 +41,7 @@
 #include <rte_vdev.h>
 #include <rte_kvargs.h>
 #include <rte_net.h>
+#include <rte_debug.h>
 
 #include <sys/types.h>
 #include <sys/stat.h>
@@ -445,6 +446,24 @@ pmd_tx_burst(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 	return num_tx;
 }
 
+static const char *
+tap_ioctl_req2str(unsigned long request)
+{
+	switch (request) {
+	case SIOCSIFFLAGS:
+		return "SIOCSIFFLAGS";
+	case SIOCGIFFLAGS:
+		return "SIOCGIFFLAGS";
+	case SIOCGIFHWADDR:
+		return "SIOCGIFHWADDR";
+	case SIOCSIFHWADDR:
+		return "SIOCSIFHWADDR";
+	case SIOCSIFMTU:
+		return "SIOCSIFMTU";
+	}
+	return "UNKNOWN";
+}
+
 static int
 tap_ioctl(struct pmd_internals *pmd, unsigned long request,
 	  struct ifreq *ifr, int set, enum ioctl_mode mode)
@@ -480,9 +499,7 @@ tap_ioctl(struct pmd_internals *pmd, unsigned long request,
 	case SIOCSIFMTU:
 		break;
 	default:
-		RTE_LOG(WARNING, PMD, "%s: ioctl() called with wrong arg\n",
-			pmd->name);
-		return -EINVAL;
+		RTE_ASSERT(!"unsupported request type: must not happen");
 	}
 	if (ioctl(pmd->ioctl_sock, request, ifr) < 0)
 		goto error;
@@ -491,8 +508,8 @@ tap_ioctl(struct pmd_internals *pmd, unsigned long request,
 	return 0;
 
 error:
-	RTE_LOG(ERR, PMD, "%s: ioctl(%lu) failed with error: %s\n",
-		ifr->ifr_name, request, strerror(errno));
+	RTE_LOG(DEBUG, PMD, "%s: %s(%s) failed: %s(%d)\n", ifr->ifr_name,
+		__func__, tap_ioctl_req2str(request), strerror(errno), errno);
 	return -errno;
 }
 
@@ -774,12 +791,8 @@ tap_mac_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr)
 		return;
 	}
 	/* Check the actual current MAC address on the tap netdevice */
-	if (tap_ioctl(pmd, SIOCGIFHWADDR, &ifr, 0, LOCAL_ONLY) != 0) {
-		RTE_LOG(ERR, PMD,
-			"%s: couldn't check current tap MAC address\n",
-			dev->data->name);
+	if (tap_ioctl(pmd, SIOCGIFHWADDR, &ifr, 0, LOCAL_ONLY) != 0)
 		return;
-	}
 	if (is_same_ether_addr((struct ether_addr *)&ifr.ifr_hwaddr.sa_data,
 			       mac_addr))
 		return;
@@ -1230,10 +1243,8 @@ eth_dev_tap_create(struct rte_vdev_device *vdev, char *tap_name,
 				remote_iface);
 			return 0;
 		}
-		if (tap_ioctl(pmd, SIOCGIFHWADDR, &ifr, 0, REMOTE_ONLY) < 0) {
-			RTE_LOG(ERR, PMD, "Could not get remote MAC address\n");
+		if (tap_ioctl(pmd, SIOCGIFHWADDR, &ifr, 0, REMOTE_ONLY) < 0)
 			goto error_exit;
-		}
 		rte_memcpy(&pmd->eth_addr, ifr.ifr_hwaddr.sa_data,
 			   ETHER_ADDR_LEN);
 	}
-- 
2.12.0.306.g4a9b9b3

[PATCH v3 2/5] net/tap: remove unnecessary functions

[Pascal Mazon]

These functions are only two lines each and are used only once.

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

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index 3d08ef2ca4d4..5b99a812fda0 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -861,26 +861,6 @@ tap_setup_queue(struct rte_eth_dev *dev,
 }
 
 static int
-rx_setup_queue(struct rte_eth_dev *dev,
-		struct pmd_internals *internals,
-		uint16_t qid)
-{
-	dev->data->rx_queues[qid] = &internals->rxq[qid];
-
-	return tap_setup_queue(dev, internals, qid);
-}
-
-static int
-tx_setup_queue(struct rte_eth_dev *dev,
-		struct pmd_internals *internals,
-		uint16_t qid)
-{
-	dev->data->tx_queues[qid] = &internals->txq[qid];
-
-	return tap_setup_queue(dev, internals, qid);
-}
-
-static int
 tap_rx_queue_setup(struct rte_eth_dev *dev,
 		   uint16_t rx_queue_id,
 		   uint16_t nb_rx_desc,
@@ -920,7 +900,8 @@ tap_rx_queue_setup(struct rte_eth_dev *dev,
 	}
 	rxq->iovecs = iovecs;
 
-	fd = rx_setup_queue(dev, internals, rx_queue_id);
+	dev->data->rx_queues[rx_queue_id] = rxq;
+	fd = tap_setup_queue(dev, internals, rx_queue_id);
 	if (fd == -1) {
 		ret = fd;
 		goto error;
@@ -971,7 +952,8 @@ tap_tx_queue_setup(struct rte_eth_dev *dev,
 	if (tx_queue_id >= internals->nb_queues)
 		return -1;
 
-	ret = tx_setup_queue(dev, internals, tx_queue_id);
+	dev->data->tx_queues[tx_queue_id] = &internals->txq[tx_queue_id];
+	ret = tap_setup_queue(dev, internals, tx_queue_id);
 	if (ret == -1)
 		return -1;
 
-- 
2.12.0.306.g4a9b9b3

[PATCH v3 3/5] net/tap: drop unnecessary nested block

[Pascal Mazon]

This is cosmetic; the code is functionally equivalent.

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

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index 5b99a812fda0..91a957edb333 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -825,30 +825,26 @@ tap_setup_queue(struct rte_eth_dev *dev,
 	struct pmd_internals *pmd = dev->data->dev_private;
 	struct rx_queue *rx = &internals->rxq[qid];
 	struct tx_queue *tx = &internals->txq[qid];
-	int fd;
+	int fd = rx->fd == -1 ? tx->fd : rx->fd;
 
-	fd = rx->fd;
-	if (fd < 0) {
-		fd = tx->fd;
+	if (fd == -1) {
+		RTE_LOG(INFO, PMD, "Add queue to TAP %s for qid %d\n",
+			pmd->name, qid);
+		fd = tun_alloc(pmd, qid);
 		if (fd < 0) {
-			RTE_LOG(INFO, PMD, "Add queue to TAP %s for qid %d\n",
+			RTE_LOG(ERR, PMD, "tun_alloc(%s, %d) failed\n",
 				pmd->name, qid);
-			fd = tun_alloc(pmd, qid);
-			if (fd < 0) {
-				RTE_LOG(ERR, PMD, "tun_alloc(%s, %d) failed\n",
-					pmd->name, qid);
+			return -1;
+		}
+		if (qid == 0) {
+			struct ifreq ifr;
+
+			ifr.ifr_mtu = dev->data->mtu;
+			if (tap_ioctl(pmd, SIOCSIFMTU, &ifr, 1,
+				      LOCAL_AND_REMOTE) < 0) {
+				close(fd);
 				return -1;
 			}
-			if (qid == 0) {
-				struct ifreq ifr;
-
-				ifr.ifr_mtu = dev->data->mtu;
-				if (tap_ioctl(pmd, SIOCSIFMTU, &ifr, 1,
-					      LOCAL_AND_REMOTE) < 0) {
-					close(fd);
-					return -1;
-				}
-			}
 		}
 	}
 
-- 
2.12.0.306.g4a9b9b3

[PATCH v3 4/5] net/tap: create netdevice during probing

[Pascal Mazon]

This has three main benefits:
 - tun_alloc is now generic again for any queue,
 - mtu no longer needs to be handled in tap_setup_queue(),
 - an actual netdevice is created as soon as the device is probed.

On top of it, code in eth_dev_tap_create() has been reworked to have a
more logical behavior; initialization can now fail if a remote is
requested but cannot be set up.

Fixes: 2bc06869cd94 ("net/tap: add remote netdevice traffic capture")

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

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index 91a957edb333..26a7f84d4f6b 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -114,10 +114,6 @@ enum ioctl_mode {
 	REMOTE_ONLY,
 };
 
-static int
-tap_ioctl(struct pmd_internals *pmd, unsigned long request,
-	  struct ifreq *ifr, int set, enum ioctl_mode mode);
-
 static int tap_intr_handle_set(struct rte_eth_dev *dev, int set);
 
 /* Tun/Tap allocation routine
@@ -126,7 +122,7 @@ static int tap_intr_handle_set(struct rte_eth_dev *dev, int set);
  * supplied name.
  */
 static int
-tun_alloc(struct pmd_internals *pmd, uint16_t qid)
+tun_alloc(struct pmd_internals *pmd)
 {
 	struct ifreq ifr;
 #ifdef IFF_MULTI_QUEUE
@@ -225,75 +221,6 @@ tun_alloc(struct pmd_internals *pmd, uint16_t qid)
 			strerror(errno));
 	}
 
-	if (qid == 0) {
-		struct ifreq ifr;
-
-		/*
-		 * pmd->eth_addr contains the desired MAC, either from remote
-		 * or from a random assignment. Sync it with the tap netdevice.
-		 */
-		ifr.ifr_hwaddr.sa_family = AF_LOCAL;
-		rte_memcpy(ifr.ifr_hwaddr.sa_data, &pmd->eth_addr,
-			   ETHER_ADDR_LEN);
-		if (tap_ioctl(pmd, SIOCSIFHWADDR, &ifr, 0, LOCAL_ONLY) < 0)
-			goto error;
-
-		pmd->if_index = if_nametoindex(pmd->name);
-		if (!pmd->if_index) {
-			RTE_LOG(ERR, PMD,
-				"Could not find ifindex for %s: rte_flow won't be usable.\n",
-				pmd->name);
-			return fd;
-		}
-		if (!pmd->flower_support)
-			return fd;
-		if (qdisc_create_multiq(pmd->nlsk_fd, pmd->if_index) < 0) {
-			RTE_LOG(ERR, PMD,
-				"Could not create multiq qdisc for %s: rte_flow won't be usable.\n",
-				pmd->name);
-			return fd;
-		}
-		if (qdisc_create_ingress(pmd->nlsk_fd, pmd->if_index) < 0) {
-			RTE_LOG(ERR, PMD,
-				"Could not create multiq qdisc for %s: rte_flow won't be usable.\n",
-				pmd->name);
-			return fd;
-		}
-		if (pmd->remote_if_index) {
-			/*
-			 * Flush usually returns negative value because it tries
-			 * to delete every QDISC (and on a running device, one
-			 * QDISC at least is needed). Ignore negative return
-			 * value.
-			 */
-			qdisc_flush(pmd->nlsk_fd, pmd->remote_if_index);
-			if (qdisc_create_ingress(pmd->nlsk_fd,
-						 pmd->remote_if_index) < 0)
-				goto remote_fail;
-			LIST_INIT(&pmd->implicit_flows);
-			if (tap_flow_implicit_create(
-				    pmd, TAP_REMOTE_LOCAL_MAC) < 0)
-				goto remote_fail;
-			if (tap_flow_implicit_create(
-				    pmd, TAP_REMOTE_BROADCAST) < 0)
-				goto remote_fail;
-			if (tap_flow_implicit_create(
-				    pmd, TAP_REMOTE_BROADCASTV6) < 0)
-				goto remote_fail;
-			if (tap_flow_implicit_create(
-				    pmd, TAP_REMOTE_TX) < 0)
-				goto remote_fail;
-		}
-	}
-
-	return fd;
-
-remote_fail:
-	RTE_LOG(ERR, PMD,
-		"Could not set up remote flow rules for %s: remote disabled.\n",
-		pmd->name);
-	pmd->remote_if_index = 0;
-	tap_flow_implicit_flush(pmd, NULL);
 	return fd;
 
 error:
@@ -830,22 +757,12 @@ tap_setup_queue(struct rte_eth_dev *dev,
 	if (fd == -1) {
 		RTE_LOG(INFO, PMD, "Add queue to TAP %s for qid %d\n",
 			pmd->name, qid);
-		fd = tun_alloc(pmd, qid);
+		fd = tun_alloc(pmd);
 		if (fd < 0) {
-			RTE_LOG(ERR, PMD, "tun_alloc(%s, %d) failed\n",
-				pmd->name, qid);
+			RTE_LOG(ERR, PMD, "%s: tun_alloc() failed.\n",
+				pmd->name);
 			return -1;
 		}
-		if (qid == 0) {
-			struct ifreq ifr;
-
-			ifr.ifr_mtu = dev->data->mtu;
-			if (tap_ioctl(pmd, SIOCSIFMTU, &ifr, 1,
-				      LOCAL_AND_REMOTE) < 0) {
-				close(fd);
-				return -1;
-			}
-		}
 	}
 
 	rx->fd = fd;
@@ -1135,6 +1052,7 @@ eth_dev_tap_create(struct rte_vdev_device *vdev, char *tap_name,
 	struct rte_eth_dev *dev;
 	struct pmd_internals *pmd;
 	struct rte_eth_dev_data *data;
+	struct ifreq ifr;
 	int i;
 
 	RTE_LOG(DEBUG, PMD, "  TAP device on numa %u\n", rte_socket_id());
@@ -1200,37 +1118,132 @@ eth_dev_tap_create(struct rte_vdev_device *vdev, char *tap_name,
 		eth_random_addr((uint8_t *)&pmd->eth_addr);
 	}
 
+	/* Immediately create the netdevice (this will create the 1st queue). */
+	if (tap_setup_queue(dev, pmd, 0) == -1)
+		goto error_exit;
+
+	ifr.ifr_mtu = dev->data->mtu;
+	if (tap_ioctl(pmd, SIOCSIFMTU, &ifr, 1, LOCAL_AND_REMOTE) < 0)
+		goto error_exit;
+
+	memset(&ifr, 0, sizeof(struct ifreq));
+	ifr.ifr_hwaddr.sa_family = AF_LOCAL;
+	rte_memcpy(ifr.ifr_hwaddr.sa_data, &pmd->eth_addr, ETHER_ADDR_LEN);
+	if (tap_ioctl(pmd, SIOCSIFHWADDR, &ifr, 0, LOCAL_ONLY) < 0)
+		goto error_exit;
+
 	tap_kernel_support(pmd);
-	if (!pmd->flower_support)
-		return 0;
-	LIST_INIT(&pmd->flows);
+	if (!pmd->flower_support) {
+		if (remote_iface[0]) {
+			RTE_LOG(ERR, PMD,
+				"%s: kernel does not support TC rules, required for remote feature.",
+				pmd->name);
+			goto error_exit;
+		} else {
+			RTE_LOG(INFO, PMD,
+				"%s: kernel too old for Flow API support.\n",
+				pmd->name);
+			return 0;
+		}
+	}
+
 	/*
-	 * If no netlink socket can be created, then it will fail when
-	 * creating/destroying flow rules.
+	 * Set up everything related to rte_flow:
+	 * - netlink socket
+	 * - tap / remote if_index
+	 * - mandatory QDISCs
+	 * - rte_flow actual/implicit lists
+	 * - implicit rules
 	 */
 	pmd->nlsk_fd = nl_init(0);
-	if (strlen(remote_iface)) {
-		struct ifreq ifr;
+	if (pmd->nlsk_fd == -1) {
+		RTE_LOG(WARNING, PMD, "%s: failed to create netlink socket.",
+			pmd->name);
+		goto disable_rte_flow;
+	}
+	pmd->if_index = if_nametoindex(pmd->name);
+	if (!pmd->if_index) {
+		RTE_LOG(ERR, PMD, "%s: failed to get if_index.", pmd->name);
+		goto disable_rte_flow;
+	}
+	if (qdisc_create_multiq(pmd->nlsk_fd, pmd->if_index) < 0) {
+		RTE_LOG(ERR, PMD, "%s: failed to create multiq qdisc.",
+			pmd->name);
+		goto disable_rte_flow;
+	}
+	if (qdisc_create_ingress(pmd->nlsk_fd, pmd->if_index) < 0) {
+		RTE_LOG(ERR, PMD, "%s: failed to create ingress qdisc.",
+			pmd->name);
+		goto disable_rte_flow;
+	}
+	LIST_INIT(&pmd->flows);
 
+	if (strlen(remote_iface)) {
 		pmd->remote_if_index = if_nametoindex(remote_iface);
+		if (!pmd->remote_if_index) {
+			RTE_LOG(ERR, PMD, "%s: failed to get %s if_index.",
+				pmd->name, remote_iface);
+			goto error_remote;
+		}
 		snprintf(pmd->remote_iface, RTE_ETH_NAME_MAX_LEN,
 			 "%s", remote_iface);
-		if (!pmd->remote_if_index) {
-			RTE_LOG(ERR, PMD, "Could not find %s ifindex: "
-				"remote interface will remain unconfigured\n",
-				remote_iface);
-			return 0;
+		if (tap_ioctl(pmd, SIOCGIFHWADDR, &ifr, 0, REMOTE_ONLY) < 0) {
+			RTE_LOG(ERR, PMD, "%s: failed to get %s MAC address.",
+				pmd->name, pmd->remote_iface);
+			goto error_remote;
 		}
-		if (tap_ioctl(pmd, SIOCGIFHWADDR, &ifr, 0, REMOTE_ONLY) < 0)
-			goto error_exit;
 		rte_memcpy(&pmd->eth_addr, ifr.ifr_hwaddr.sa_data,
 			   ETHER_ADDR_LEN);
+		/* The desired MAC is already in ifreq after SIOCGIFHWADDR. */
+		if (tap_ioctl(pmd, SIOCSIFHWADDR, &ifr, 0, LOCAL_ONLY) < 0) {
+			RTE_LOG(ERR, PMD, "%s: failed to get %s MAC address.",
+				pmd->name, remote_iface);
+			goto error_remote;
+		}
+
+		/*
+		 * Flush usually returns negative value because it tries to
+		 * delete every QDISC (and on a running device, one QDISC at
+		 * least is needed). Ignore negative return value.
+		 */
+		qdisc_flush(pmd->nlsk_fd, pmd->remote_if_index);
+		if (qdisc_create_ingress(pmd->nlsk_fd,
+					 pmd->remote_if_index) < 0) {
+			RTE_LOG(ERR, PMD, "%s: failed to create ingress qdisc.",
+				pmd->remote_iface);
+			goto error_remote;
+		}
+		LIST_INIT(&pmd->implicit_flows);
+		if (tap_flow_implicit_create(pmd, TAP_REMOTE_TX) < 0 ||
+		    tap_flow_implicit_create(pmd, TAP_REMOTE_LOCAL_MAC) < 0 ||
+		    tap_flow_implicit_create(pmd, TAP_REMOTE_BROADCAST) < 0 ||
+		    tap_flow_implicit_create(pmd, TAP_REMOTE_BROADCASTV6) < 0) {
+			RTE_LOG(ERR, PMD,
+				"%s: failed to create implicit rules.",
+				pmd->name);
+			goto error_remote;
+		}
 	}
 
 	return 0;
 
+disable_rte_flow:
+	RTE_LOG(ERR, PMD, " Disabling rte flow support: %s(%d)\n",
+		strerror(errno), errno);
+	if (strlen(remote_iface)) {
+		RTE_LOG(ERR, PMD, "Remote feature requires flow support.\n");
+		goto error_exit;
+	}
+	pmd->flower_support = 0;
+	return 0;
+
+error_remote:
+	RTE_LOG(ERR, PMD, " Can't set up remote feature: %s(%d)\n",
+		strerror(errno), errno);
+	tap_flow_implicit_flush(pmd, NULL);
+
 error_exit:
-	RTE_LOG(DEBUG, PMD, "TAP Unable to initialize %s\n",
+	RTE_LOG(ERR, PMD, "TAP Unable to initialize %s\n",
 		rte_vdev_device_name(vdev));
 
 	rte_free(data);
-- 
2.12.0.306.g4a9b9b3

[PATCH v3 5/5] net/tap: do not set remote MAC if not necessary

[Pascal Mazon]

Check for the current MAC address on both the remote and the tap
netdevices before setting a new value.

While there, remove wrong empty lines and ensure tap_ioctl() return
value is negative, just like what is done throughout this code.

Fixes: 2bc06869cd94 ("net/tap: add remote netdevice traffic capture")

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

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index 26a7f84d4f6b..49549b4f1822 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -705,11 +705,11 @@ tap_allmulti_disable(struct rte_eth_dev *dev)
 		tap_flow_implicit_destroy(pmd, TAP_REMOTE_ALLMULTI);
 }
 
-
 static void
 tap_mac_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr)
 {
 	struct pmd_internals *pmd = dev->data->dev_private;
+	enum ioctl_mode mode = LOCAL_ONLY;
 	struct ifreq ifr;
 
 	if (is_zero_ether_addr(mac_addr)) {
@@ -718,15 +718,20 @@ tap_mac_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr)
 		return;
 	}
 	/* Check the actual current MAC address on the tap netdevice */
-	if (tap_ioctl(pmd, SIOCGIFHWADDR, &ifr, 0, LOCAL_ONLY) != 0)
+	if (tap_ioctl(pmd, SIOCGIFHWADDR, &ifr, 0, LOCAL_ONLY) < 0)
 		return;
 	if (is_same_ether_addr((struct ether_addr *)&ifr.ifr_hwaddr.sa_data,
 			       mac_addr))
 		return;
-
+	/* Check the current MAC address on the remote */
+	if (tap_ioctl(pmd, SIOCGIFHWADDR, &ifr, 0, REMOTE_ONLY) < 0)
+		return;
+	if (!is_same_ether_addr((struct ether_addr *)&ifr.ifr_hwaddr.sa_data,
+			       mac_addr))
+		mode = LOCAL_AND_REMOTE;
 	ifr.ifr_hwaddr.sa_family = AF_LOCAL;
 	rte_memcpy(ifr.ifr_hwaddr.sa_data, mac_addr, ETHER_ADDR_LEN);
-	if (tap_ioctl(pmd, SIOCSIFHWADDR, &ifr, 1, LOCAL_AND_REMOTE) < 0)
+	if (tap_ioctl(pmd, SIOCSIFHWADDR, &ifr, 1, mode) < 0)
 		return;
 	rte_memcpy(&pmd->eth_addr, mac_addr, ETHER_ADDR_LEN);
 	if (pmd->remote_if_index) {
-- 
2.12.0.306.g4a9b9b3

Re: [PATCH v3 0/5] improve tap behavior

[Ferruh Yigit]

On 5/12/2017 2:01 PM, Pascal Mazon wrote:
> The tap does not behave properly in some cases.
> 
> It is generally expected that a real device should be available once the
> probing has been done.
> 
> It is also better to check if an operation (here, setting MAC) is
> mandatory before performing it. Typically in cases where the remote
> netdevice is a VF with limited capabilities.
> 
> This series ensures that the tap works more logically.
> 
> v3 changes:
>   - rebase on top of next-net/master
> 
> v2 changes:
>   - fix uninitialized fd variable
> 
> Pascal Mazon (5):
>   net/tap: add debug messages
>   net/tap: remove unnecessary functions
>   net/tap: drop unnecessary nested block
>   net/tap: create netdevice during probing
>   net/tap: do not set remote MAC if not necessary

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