Skip to contentSkip to conversation

improve tap behavior

PATCH Superseded Pascal Mazon 2 participants 21 messages Apr 14, 2017 – May 12, 2017 Network drivers

lore
Share & tools
lore

Pascal Mazon

author
#0 lore raw

cover improve tap behavior

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(-)
Signature · 1 line
2.12.0.306.g4a9b9b3
author
#1 lore raw

PATCH 1/5 net/tap: add debug messages

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

Signed-off-by Pascal Mazon <[email protected]>
---
 drivers/net/tap/rte_eth_tap.c | 37 ++++++++++++++++++++++++-------------
 1 file changed, 24 insertions(+), 13 deletions(-)
drivers/net/tap/rte_eth_tap.c +24 -13
index b75663838844..0c89b63a2357 100644
@@ -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);
 	}
Signature · 1 line
2.12.0.306.g4a9b9b3
author
#2 lore raw

PATCH 2/5 net/tap: remove unnecessary functions

In reply to Pascal Mazon

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

Signed-off-by Pascal Mazon <[email protected]>
---
 drivers/net/tap/rte_eth_tap.c | 26 ++++----------------------
 1 file changed, 4 insertions(+), 22 deletions(-)
drivers/net/tap/rte_eth_tap.c +4 -22
index 0c89b63a2357..c9d107c25ac2 100644
@@ -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;
Signature · 1 line
2.12.0.306.g4a9b9b3
author
#3 lore raw

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

In reply to Pascal Mazon

This is cosmetic; the code is functionally equivalent.

Signed-off-by Pascal Mazon <[email protected]>
---
 drivers/net/tap/rte_eth_tap.c | 32 ++++++++++++++------------------
 1 file changed, 14 insertions(+), 18 deletions(-)
drivers/net/tap/rte_eth_tap.c +14 -18
index c9d107c25ac2..770f08ce12f5 100644
@@ -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;
-				}
-			}
 		}
 	}
Signature · 1 line
2.12.0.306.g4a9b9b3
author
#4 lore raw

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

In reply to 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 <[email protected]>
---
 drivers/net/tap/rte_eth_tap.c | 217 ++++++++++++++++++++++--------------------
 1 file changed, 115 insertions(+), 102 deletions(-)
drivers/net/tap/rte_eth_tap.c +115 -102
index 770f08ce12f5..52bce08780ce 100644
@@ -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);
Signature · 1 line
2.12.0.306.g4a9b9b3
author
#5 lore raw

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

In reply to 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 <[email protected]>
---
 drivers/net/tap/rte_eth_tap.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)
drivers/net/tap/rte_eth_tap.c +9 -4
index 52bce08780ce..41276eacfec3 100644
@@ -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) {
Signature · 1 line
2.12.0.306.g4a9b9b3
#6 lore raw
In reply to Pascal Mazon
Quoted 9 lines · 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.

Quoted 8 lines

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

author
#7 lore raw

cover improve tap behavior

In reply to 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(-)
Signature · 1 line
2.12.0.306.g4a9b9b3
author
#8 lore raw

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

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

Signed-off-by Pascal Mazon <[email protected]>
---
 drivers/net/tap/rte_eth_tap.c | 37 ++++++++++++++++++++++++-------------
 1 file changed, 24 insertions(+), 13 deletions(-)
drivers/net/tap/rte_eth_tap.c +24 -13
index b75663838844..0c89b63a2357 100644
@@ -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);
 	}
Signature · 1 line
2.12.0.306.g4a9b9b3
author
#9 lore raw

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

In reply to Pascal Mazon

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

Signed-off-by Pascal Mazon <[email protected]>
---
 drivers/net/tap/rte_eth_tap.c | 26 ++++----------------------
 1 file changed, 4 insertions(+), 22 deletions(-)
drivers/net/tap/rte_eth_tap.c +4 -22
index 0c89b63a2357..c9d107c25ac2 100644
@@ -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;
Signature · 1 line
2.12.0.306.g4a9b9b3
author
#10 lore raw

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

In reply to Pascal Mazon

This is cosmetic; the code is functionally equivalent.

Signed-off-by Pascal Mazon <[email protected]>
---
 drivers/net/tap/rte_eth_tap.c | 34 +++++++++++++++-------------------
 1 file changed, 15 insertions(+), 19 deletions(-)
drivers/net/tap/rte_eth_tap.c +15 -19
index c9d107c25ac2..403aca9f7307 100644
@@ -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;
-				}
-			}
 		}
 	}
Signature · 1 line
2.12.0.306.g4a9b9b3
author
#11 lore raw

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

In reply to 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 <[email protected]>
---
 drivers/net/tap/rte_eth_tap.c | 217 ++++++++++++++++++++++--------------------
 1 file changed, 115 insertions(+), 102 deletions(-)
drivers/net/tap/rte_eth_tap.c +115 -102
index 403aca9f7307..24ad9ead3b4c 100644
@@ -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);
Signature · 1 line
2.12.0.306.g4a9b9b3
author
#12 lore raw

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

In reply to 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 <[email protected]>
---
 drivers/net/tap/rte_eth_tap.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)
drivers/net/tap/rte_eth_tap.c +9 -4
index 24ad9ead3b4c..58c1b4cb3ddf 100644
@@ -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) {
Signature · 1 line
2.12.0.306.g4a9b9b3
#13 lore raw
In reply to Pascal Mazon
Quoted 14 lines · 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

Quoted 6 lines

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

author
#14 lore raw

v3 cover improve tap behavior

In reply to 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(-)
Signature · 1 line
2.12.0.306.g4a9b9b3
author
#15 lore raw

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

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

Signed-off-by Pascal Mazon <[email protected]>
---
 drivers/net/tap/rte_eth_tap.c | 37 ++++++++++++++++++++++++-------------
 1 file changed, 24 insertions(+), 13 deletions(-)
drivers/net/tap/rte_eth_tap.c +24 -13
index f1bb55e9769b..3d08ef2ca4d4 100644
@@ -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);
 	}
Signature · 1 line
2.12.0.306.g4a9b9b3
author
#16 lore raw

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

In reply to Pascal Mazon

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

Signed-off-by Pascal Mazon <[email protected]>
---
 drivers/net/tap/rte_eth_tap.c | 26 ++++----------------------
 1 file changed, 4 insertions(+), 22 deletions(-)
drivers/net/tap/rte_eth_tap.c +4 -22
index 3d08ef2ca4d4..5b99a812fda0 100644
@@ -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;
Signature · 1 line
2.12.0.306.g4a9b9b3
author
#17 lore raw

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

In reply to Pascal Mazon

This is cosmetic; the code is functionally equivalent.

Signed-off-by Pascal Mazon <[email protected]>
---
 drivers/net/tap/rte_eth_tap.c | 34 +++++++++++++++-------------------
 1 file changed, 15 insertions(+), 19 deletions(-)
drivers/net/tap/rte_eth_tap.c +15 -19
index 5b99a812fda0..91a957edb333 100644
@@ -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;
-				}
-			}
 		}
 	}
Signature · 1 line
2.12.0.306.g4a9b9b3
author
#18 lore raw

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

In reply to 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 <[email protected]>
---
 drivers/net/tap/rte_eth_tap.c | 217 ++++++++++++++++++++++--------------------
 1 file changed, 115 insertions(+), 102 deletions(-)
drivers/net/tap/rte_eth_tap.c +115 -102
index 91a957edb333..26a7f84d4f6b 100644
@@ -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);
Signature · 1 line
2.12.0.306.g4a9b9b3
author
#19 lore raw

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

In reply to 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 <[email protected]>
---
 drivers/net/tap/rte_eth_tap.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)
drivers/net/tap/rte_eth_tap.c +9 -4
index 26a7f84d4f6b..49549b4f1822 100644
@@ -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) {
Signature · 1 line
2.12.0.306.g4a9b9b3
#20 lore raw
In reply to Pascal Mazon
Quoted 17 lines · 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

Quoted 6 lines

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.