From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f43.google.com (mail-wm0-f43.google.com [74.125.82.43]) by dpdk.org (Postfix) with ESMTP id 138BE3B5 for ; Mon, 6 Feb 2017 16:45:47 +0100 (CET) Received: by mail-wm0-f43.google.com with SMTP id t18so35228792wmt.0 for ; Mon, 06 Feb 2017 07:45:47 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:in-reply-to:references :organization:mime-version:content-transfer-encoding; bh=pDeOhUj+dinoh79ppOdzTkZXop25mZSz8TPP/kab0fM=; b=yjQCtJb9hG08PT8AMa5PXIv7pGPXjM34iBrlkT27/iu8587qkDC8YAmk26XJulPtDU Tm94tI2dF2UF3crCbhjHdOVYZLgL5AXp3R98thAJba3+bWvk1ccl5V6bOV5hQ2K/Cz07 kJi2FfkkHY1rWxMWny0QYaXyp/HVogWVcO70SvLzNmtf9PVTDs34KVrXPoggUGig80aL KQQBFYI0OoAA0ZB889YrCmyUq5XEsH4JsF0s68tN3OdtQG5aocMhk9Z59Mzqt6YFsehi 6NItTA+Buo8Yprut6h6KsMsWAa/Joisin32M0L+u9e1jbzJb5OcAj+RHKle3GRJslAXC GDnw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to :references:organization:mime-version:content-transfer-encoding; bh=pDeOhUj+dinoh79ppOdzTkZXop25mZSz8TPP/kab0fM=; b=hczg72tnSChkqaQ/nkhIYvGfBEqLqaAVaGtt4JE0UkqEBYC5Ft6BVJIDQ7+e9FwsGk fOtcszIs3orxYk6m81PFuL6ma7XHv87OvkTQG7Jd6OiLbu3082teBSzrRJFlQpiaGPYY inF0Im611WH/vjGhCpxnzznwFVkKeZHMjhsG6/av7GgSDqDFn9kUwmFaV2ZS04Kg0he4 Bs+up7R6XkWbbQ7Pw2OZt4yPYODejidHYPBlL0xzWcjKQyKvEQUpyxtoNS51jBNjZoys zCLx/aldgWrJ2A5WSmIUzPIeU3+J2s3KaFLBEP6KRxLGhhO2Fy1+pqrqIVDFjDlAdsQU 9ktw== X-Gm-Message-State: AIkVDXLy3+tqgpmDzZ4bqieDqx0/Wr37G7tUgZhF0sB3SjB7t7eO6/7QLtZwxW1PPNpQN0Dr X-Received: by 10.223.151.99 with SMTP id r90mr9407475wrb.183.1486395946596; Mon, 06 Feb 2017 07:45:46 -0800 (PST) Received: from paques.dev.6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id u198sm13570205wmf.9.2017.02.06.07.45.46 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 06 Feb 2017 07:45:46 -0800 (PST) Date: Mon, 6 Feb 2017 16:45:37 +0100 From: Pascal Mazon To: Keith Wiles Cc: dev@dpdk.org, ferruh.yigit@intel.com Message-ID: <20170206164537.7ccf79db@paques.dev.6wind.com> In-Reply-To: <20170205160509.88530-4-keith.wiles@intel.com> References: <20170205160509.88530-1-keith.wiles@intel.com> <20170205160509.88530-4-keith.wiles@intel.com> Organization: 6WIND X-Mailer: Claws Mail 3.11.1 (GTK+ 2.24.25; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v2 4/6] net/tap: fix multi-queue support X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 06 Feb 2017 15:45:47 -0000 On Sun, 5 Feb 2017 10:05:07 -0600 Keith Wiles wrote: > At the same time remove the code which created the first device queue > at probe time. Now all queues are created during queue setup calls. > > Signed-off-by: Keith Wiles > --- > drivers/net/tap/rte_eth_tap.c | 104 > ++++++++++++++---------------------------- 1 file changed, 34 insertions(+), > 70 deletions(-) > > diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c > index 61659bc..7c923a2 100644 > --- a/drivers/net/tap/rte_eth_tap.c > +++ b/drivers/net/tap/rte_eth_tap.c > @@ -57,7 +57,11 @@ > #define ETH_TAP_IFACE_ARG "iface" > #define ETH_TAP_SPEED_ARG "speed" > > +#ifdef IFF_MULTI_QUEUE > #define RTE_PMD_TAP_MAX_QUEUES 16 > +#else > +#define RTE_PMD_TAP_MAX_QUEUES 1 > +#endif > > static struct rte_vdev_driver pmd_tap_drv; > > @@ -114,17 +118,20 @@ struct pmd_internals { > * supplied name. > */ > static int > -tun_alloc(char *name) > +tun_alloc(struct pmd_internals *pmd, uint16_t qid) > { > struct ifreq ifr; > +#ifdef IFF_MULTI_QUEUE > unsigned int features; > +#endif > int fd; > > memset(&ifr, 0, sizeof(struct ifreq)); > > ifr.ifr_flags = IFF_TAP | IFF_NO_PI; > - if (name && name[0]) > - strncpy(ifr.ifr_name, name, IFNAMSIZ); > + strncpy(ifr.ifr_name, pmd->name, IFNAMSIZ); > + > + RTE_LOG(DEBUG, PMD, "ifr_name '%s'\n", ifr.ifr_name); > > fd = open(TUN_TAP_DEV_PATH, O_RDWR); > if (fd < 0) { > @@ -132,37 +139,26 @@ tun_alloc(char *name) > goto error; > } > > - /* Grab the TUN features to verify we can work */ > +#ifdef IFF_MULTI_QUEUE > + /* Grab the TUN features to verify we can work multi-queue */ > if (ioctl(fd, TUNGETFEATURES, &features) < 0) { > - RTE_LOG(ERR, PMD, "Unable to get TUN/TAP features\n"); > + RTE_LOG(ERR, PMD, "TAP unable to get TUN/TAP features\n"); > goto error; > } > - RTE_LOG(DEBUG, PMD, "TUN/TAP Features %08x\n", features); > + RTE_LOG(DEBUG, PMD, " TAP Features %08x\n", features); > > -#ifdef IFF_MULTI_QUEUE > - if (!(features & IFF_MULTI_QUEUE) && (RTE_PMD_TAP_MAX_QUEUES > 1)) { > - RTE_LOG(DEBUG, PMD, "TUN/TAP device only one queue\n"); > - goto error; > - } else if ((features & IFF_ONE_QUEUE) && > - (RTE_PMD_TAP_MAX_QUEUES == 1)) { > - ifr.ifr_flags |= IFF_ONE_QUEUE; > - RTE_LOG(DEBUG, PMD, "Single queue only support\n"); > - } else { > - ifr.ifr_flags |= IFF_MULTI_QUEUE; > - RTE_LOG(DEBUG, PMD, "Multi-queue support for %d queues\n", > + if (features & IFF_MULTI_QUEUE) { > + RTE_LOG(DEBUG, PMD, " Multi-queue support for %d queues\n", > RTE_PMD_TAP_MAX_QUEUES); > - } > -#else > - if (RTE_PMD_TAP_MAX_QUEUES > 1) { > - RTE_LOG(DEBUG, PMD, "TUN/TAP device only one queue\n"); > - goto error; > - } else { > + ifr.ifr_flags |= IFF_MULTI_QUEUE; > + } else > +#endif > + { > ifr.ifr_flags |= IFF_ONE_QUEUE; > RTE_LOG(DEBUG, PMD, "Single queue only support\n"); > } > -#endif > > - /* Set the TUN/TAP configuration and get the name if needed */ > + /* Set the TUN/TAP configuration and set the name if needed */ > if (ioctl(fd, TUNSETIFF, (void *)&ifr) < 0) { > RTE_LOG(ERR, PMD, "Unable to set TUNSETIFF for %s\n", > ifr.ifr_name); > @@ -177,9 +173,15 @@ tun_alloc(char *name) > goto error; > } > > - /* If the name is different that new name as default */ > - if (name && strcmp(name, ifr.ifr_name)) > - snprintf(name, RTE_ETH_NAME_MAX_LEN - 1, "%s", ifr.ifr_name); > + if (qid == 0) { > + if (ioctl(fd, SIOCGIFHWADDR, &ifr) == -1) { > + RTE_LOG(ERR, PMD, "ioctl failed (SIOCGIFHWADDR) > (%s)\n", > + ifr.ifr_name); > + goto error; > + } > + > + rte_memcpy(&pmd->eth_addr, ifr.ifr_hwaddr.sa_data, ETH_ALEN); > + } > > return fd; > > @@ -507,7 +509,7 @@ tap_setup_queue(struct rte_eth_dev *dev, > if (fd < 0) { > RTE_LOG(INFO, PMD, "Add queue to TAP %s for qid > %d\n", pmd->name, qid); > - fd = tun_alloc(pmd->name); > + fd = tun_alloc(pmd, qid); > if (fd < 0) { > RTE_LOG(ERR, PMD, "tun_alloc(%s) failed\n", > pmd->name); > @@ -629,34 +631,13 @@ static const struct eth_dev_ops ops = { > }; > > static int > -pmd_mac_address(int fd, struct ether_addr *addr) > -{ > - struct ifreq ifr; > - > - if ((fd <= 0) || !addr) > - return -1; > - > - memset(&ifr, 0, sizeof(ifr)); > - > - if (ioctl(fd, SIOCGIFHWADDR, &ifr) == -1) { > - RTE_LOG(ERR, PMD, "ioctl failed (SIOCGIFHWADDR) (%s)\n", > - ifr.ifr_name); > - return -1; > - } > - > - rte_memcpy(addr, ifr.ifr_hwaddr.sa_data, ETH_ALEN); > - > - return 0; > -} > - > -static int > eth_dev_tap_create(const char *name, char *tap_name) > { > int numa_node = rte_socket_id(); > struct rte_eth_dev *dev = NULL; > struct pmd_internals *pmd = NULL; > struct rte_eth_dev_data *data = NULL; > - int i, fd = -1; > + int i; > > RTE_LOG(INFO, PMD, > "%s: Create TAP Ethernet device with %d queues on numa %u\n", > @@ -674,7 +655,6 @@ eth_dev_tap_create(const char *name, char *tap_name) > goto error_exit; > } > > - /* Use the name and not the tap_name */ > dev = rte_eth_dev_allocate(tap_name); > if (!dev) { > RTE_LOG(ERR, PMD, "Unable to allocate device struct\n"); > @@ -705,28 +685,12 @@ eth_dev_tap_create(const char *name, char *tap_name) > dev->tx_pkt_burst = pmd_tx_burst; > snprintf(dev->data->name, sizeof(dev->data->name), "%s", name); > > - /* Create the first Tap device */ > - fd = tun_alloc(tap_name); > - if (fd < 0) { > - RTE_LOG(ERR, PMD, "tun_alloc() failed\n"); > - goto error_exit; > - } > - > - /* Presetup the fds to -1 as being not working */ > - for (i = 1; i < RTE_PMD_TAP_MAX_QUEUES; i++) { > + /* Presetup the fds to -1 as being not valid */ > + for (i = 0; i < RTE_PMD_TAP_MAX_QUEUES; i++) { > pmd->rxq[i].fd = -1; > pmd->txq[i].fd = -1; > } > > - /* Take the TUN/TAP fd and place in the first location */ > - pmd->rxq[0].fd = fd; > - pmd->txq[0].fd = fd; > - > - if (pmd_mac_address(fd, &pmd->eth_addr) < 0) { > - RTE_LOG(ERR, PMD, "Unable to get MAC address\n"); > - goto error_exit; > - } > - > return 0; > > error_exit: I like that you use #ifdef IFF_MULTI_QUEUE, it's a good thing. Something bothers me, though. I don't think you should close all fds in the dev_stop() function, but rather in the dev_close() function. After a dev_close(), we don't expect the user to re-use the device, so we can safely remove the queues. However, after a dev_stop(), the user can definitely do a dev_start(), which in your case would fail because it will try to set the link up on an inexisting device. dev_stop() should act as dev_start(), in symmetry, and dev_close() should close the fd/queues. The rest of the code looks OK to me. Best regards, Pascal