* [dpdk-dev] [PATCH v2 1/5] net/tap: add debug messages
2017-04-18 8:17 ` Pascal Mazon
@ 2017-04-18 8:17 ` Pascal Mazon
2017-04-18 8:17 ` [dpdk-dev] [PATCH v2 2/5] net/tap: remove unnecessary functions Pascal Mazon
` (5 subsequent siblings)
6 siblings, 0 replies; 21+ messages in thread
From: Pascal Mazon @ 2017-04-18 8:17 UTC (permalink / raw)
To: dev; +Cc: 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
^ permalink raw reply [flat|nested] 21+ messages in thread* [dpdk-dev] [PATCH v2 2/5] net/tap: remove unnecessary functions
2017-04-18 8:17 ` Pascal Mazon
2017-04-18 8:17 ` [dpdk-dev] [PATCH v2 1/5] net/tap: add debug messages Pascal Mazon
@ 2017-04-18 8:17 ` Pascal Mazon
2017-04-18 8:17 ` [dpdk-dev] [PATCH v2 3/5] net/tap: drop unnecessary nested block Pascal Mazon
` (4 subsequent siblings)
6 siblings, 0 replies; 21+ messages in thread
From: Pascal Mazon @ 2017-04-18 8:17 UTC (permalink / raw)
To: dev; +Cc: 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
^ permalink raw reply [flat|nested] 21+ messages in thread* [dpdk-dev] [PATCH v2 3/5] net/tap: drop unnecessary nested block
2017-04-18 8:17 ` Pascal Mazon
2017-04-18 8:17 ` [dpdk-dev] [PATCH v2 1/5] net/tap: add debug messages Pascal Mazon
2017-04-18 8:17 ` [dpdk-dev] [PATCH v2 2/5] net/tap: remove unnecessary functions Pascal Mazon
@ 2017-04-18 8:17 ` Pascal Mazon
2017-04-18 8:17 ` [dpdk-dev] [PATCH v2 4/5] net/tap: create netdevice during probing Pascal Mazon
` (3 subsequent siblings)
6 siblings, 0 replies; 21+ messages in thread
From: Pascal Mazon @ 2017-04-18 8:17 UTC (permalink / raw)
To: dev; +Cc: 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
^ permalink raw reply [flat|nested] 21+ messages in thread* [dpdk-dev] [PATCH v2 4/5] net/tap: create netdevice during probing
2017-04-18 8:17 ` Pascal Mazon
` (2 preceding siblings ...)
2017-04-18 8:17 ` [dpdk-dev] [PATCH v2 3/5] net/tap: drop unnecessary nested block Pascal Mazon
@ 2017-04-18 8:17 ` Pascal Mazon
2017-04-18 8:17 ` [dpdk-dev] [PATCH v2 5/5] net/tap: do not set remote MAC if not necessary Pascal Mazon
` (2 subsequent siblings)
6 siblings, 0 replies; 21+ messages in thread
From: Pascal Mazon @ 2017-04-18 8:17 UTC (permalink / raw)
To: dev; +Cc: 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
^ permalink raw reply [flat|nested] 21+ messages in thread* [dpdk-dev] [PATCH v2 5/5] net/tap: do not set remote MAC if not necessary
2017-04-18 8:17 ` Pascal Mazon
` (3 preceding siblings ...)
2017-04-18 8:17 ` [dpdk-dev] [PATCH v2 4/5] net/tap: create netdevice during probing Pascal Mazon
@ 2017-04-18 8:17 ` Pascal Mazon
2017-05-12 12:29 ` [dpdk-dev] [PATCH 0/5] improve tap behavior Ferruh Yigit
2017-05-12 13:01 ` [dpdk-dev] [PATCH v3 " Pascal Mazon
6 siblings, 0 replies; 21+ messages in thread
From: Pascal Mazon @ 2017-04-18 8:17 UTC (permalink / raw)
To: dev; +Cc: 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
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [dpdk-dev] [PATCH 0/5] improve tap behavior
2017-04-18 8:17 ` Pascal Mazon
` (4 preceding siblings ...)
2017-04-18 8:17 ` [dpdk-dev] [PATCH v2 5/5] net/tap: do not set remote MAC if not necessary Pascal Mazon
@ 2017-05-12 12:29 ` Ferruh Yigit
2017-05-12 13:01 ` [dpdk-dev] [PATCH v3 " Pascal Mazon
6 siblings, 0 replies; 21+ messages in thread
From: Ferruh Yigit @ 2017-05-12 12:29 UTC (permalink / raw)
To: Pascal Mazon, dev
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
^ permalink raw reply [flat|nested] 21+ messages in thread* [dpdk-dev] [PATCH v3 0/5] improve tap behavior
2017-04-18 8:17 ` Pascal Mazon
` (5 preceding siblings ...)
2017-05-12 12:29 ` [dpdk-dev] [PATCH 0/5] improve tap behavior Ferruh Yigit
@ 2017-05-12 13:01 ` Pascal Mazon
2017-05-12 13:01 ` [dpdk-dev] [PATCH v3 1/5] net/tap: add debug messages Pascal Mazon
` (5 more replies)
6 siblings, 6 replies; 21+ messages in thread
From: Pascal Mazon @ 2017-05-12 13:01 UTC (permalink / raw)
To: ferruh.yigit, dev; +Cc: 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
^ permalink raw reply [flat|nested] 21+ messages in thread* [dpdk-dev] [PATCH v3 1/5] net/tap: add debug messages
2017-05-12 13:01 ` [dpdk-dev] [PATCH v3 " Pascal Mazon
@ 2017-05-12 13:01 ` Pascal Mazon
2017-05-12 13:01 ` [dpdk-dev] [PATCH v3 2/5] net/tap: remove unnecessary functions Pascal Mazon
` (4 subsequent siblings)
5 siblings, 0 replies; 21+ messages in thread
From: Pascal Mazon @ 2017-05-12 13:01 UTC (permalink / raw)
To: ferruh.yigit, dev; +Cc: 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
^ permalink raw reply [flat|nested] 21+ messages in thread* [dpdk-dev] [PATCH v3 2/5] net/tap: remove unnecessary functions
2017-05-12 13:01 ` [dpdk-dev] [PATCH v3 " Pascal Mazon
2017-05-12 13:01 ` [dpdk-dev] [PATCH v3 1/5] net/tap: add debug messages Pascal Mazon
@ 2017-05-12 13:01 ` Pascal Mazon
2017-05-12 13:01 ` [dpdk-dev] [PATCH v3 3/5] net/tap: drop unnecessary nested block Pascal Mazon
` (3 subsequent siblings)
5 siblings, 0 replies; 21+ messages in thread
From: Pascal Mazon @ 2017-05-12 13:01 UTC (permalink / raw)
To: ferruh.yigit, dev; +Cc: 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
^ permalink raw reply [flat|nested] 21+ messages in thread* [dpdk-dev] [PATCH v3 3/5] net/tap: drop unnecessary nested block
2017-05-12 13:01 ` [dpdk-dev] [PATCH v3 " Pascal Mazon
2017-05-12 13:01 ` [dpdk-dev] [PATCH v3 1/5] net/tap: add debug messages Pascal Mazon
2017-05-12 13:01 ` [dpdk-dev] [PATCH v3 2/5] net/tap: remove unnecessary functions Pascal Mazon
@ 2017-05-12 13:01 ` Pascal Mazon
2017-05-12 13:01 ` [dpdk-dev] [PATCH v3 4/5] net/tap: create netdevice during probing Pascal Mazon
` (2 subsequent siblings)
5 siblings, 0 replies; 21+ messages in thread
From: Pascal Mazon @ 2017-05-12 13:01 UTC (permalink / raw)
To: ferruh.yigit, dev; +Cc: 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
^ permalink raw reply [flat|nested] 21+ messages in thread* [dpdk-dev] [PATCH v3 4/5] net/tap: create netdevice during probing
2017-05-12 13:01 ` [dpdk-dev] [PATCH v3 " Pascal Mazon
` (2 preceding siblings ...)
2017-05-12 13:01 ` [dpdk-dev] [PATCH v3 3/5] net/tap: drop unnecessary nested block Pascal Mazon
@ 2017-05-12 13:01 ` Pascal Mazon
2017-05-12 13:01 ` [dpdk-dev] [PATCH v3 5/5] net/tap: do not set remote MAC if not necessary Pascal Mazon
2017-05-12 14:04 ` [dpdk-dev] [PATCH v3 0/5] improve tap behavior Ferruh Yigit
5 siblings, 0 replies; 21+ messages in thread
From: Pascal Mazon @ 2017-05-12 13:01 UTC (permalink / raw)
To: ferruh.yigit, dev; +Cc: 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
^ permalink raw reply [flat|nested] 21+ messages in thread* [dpdk-dev] [PATCH v3 5/5] net/tap: do not set remote MAC if not necessary
2017-05-12 13:01 ` [dpdk-dev] [PATCH v3 " Pascal Mazon
` (3 preceding siblings ...)
2017-05-12 13:01 ` [dpdk-dev] [PATCH v3 4/5] net/tap: create netdevice during probing Pascal Mazon
@ 2017-05-12 13:01 ` Pascal Mazon
2017-05-12 14:04 ` [dpdk-dev] [PATCH v3 0/5] improve tap behavior Ferruh Yigit
5 siblings, 0 replies; 21+ messages in thread
From: Pascal Mazon @ 2017-05-12 13:01 UTC (permalink / raw)
To: ferruh.yigit, dev; +Cc: 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
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [dpdk-dev] [PATCH v3 0/5] improve tap behavior
2017-05-12 13:01 ` [dpdk-dev] [PATCH v3 " Pascal Mazon
` (4 preceding siblings ...)
2017-05-12 13:01 ` [dpdk-dev] [PATCH v3 5/5] net/tap: do not set remote MAC if not necessary Pascal Mazon
@ 2017-05-12 14:04 ` Ferruh Yigit
5 siblings, 0 replies; 21+ messages in thread
From: Ferruh Yigit @ 2017-05-12 14:04 UTC (permalink / raw)
To: Pascal Mazon, dev
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.
^ permalink raw reply [flat|nested] 21+ messages in thread