From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id 7E2FF6932 for ; Mon, 6 Feb 2017 16:57:03 +0100 (CET) Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga102.fm.intel.com with ESMTP; 06 Feb 2017 07:57:02 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,342,1477983600"; d="scan'208";a="1122773004" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by fmsmga002.fm.intel.com with ESMTP; 06 Feb 2017 07:57:02 -0800 Received: from fmsmsx112.amr.corp.intel.com (10.18.116.6) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.248.2; Mon, 6 Feb 2017 07:57:02 -0800 Received: from fmsmsx113.amr.corp.intel.com ([169.254.13.230]) by FMSMSX112.amr.corp.intel.com ([169.254.5.137]) with mapi id 14.03.0248.002; Mon, 6 Feb 2017 07:57:02 -0800 From: "Wiles, Keith" To: Pascal Mazon CC: "dev@dpdk.org" , "Yigit, Ferruh" Thread-Topic: [PATCH v2 4/6] net/tap: fix multi-queue support Thread-Index: AQHSgJAiRgj36ukdAUKO3s8F2vtAHqFcqEkA Date: Mon, 6 Feb 2017 15:57:01 +0000 Message-ID: <919B37D8-F45D-4973-88AE-7E0267CEEB5A@intel.com> References: <20170205160509.88530-1-keith.wiles@intel.com> <20170205160509.88530-4-keith.wiles@intel.com> <20170206164537.7ccf79db@paques.dev.6wind.com> In-Reply-To: <20170206164537.7ccf79db@paques.dev.6wind.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.254.19.238] Content-Type: text/plain; charset="us-ascii" Content-ID: <8896CA4997E1934AB1E785591EE5FACF@intel.com> Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 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:57:04 -0000 > On Feb 6, 2017, at 9:45 AM, Pascal Mazon wrote: >=20 > On Sun, 5 Feb 2017 10:05:07 -0600 > Keith Wiles wrote: >=20 >> 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. >>=20 >> Signed-off-by: Keith Wiles >> --- >> drivers/net/tap/rte_eth_tap.c | 104 >> ++++++++++++++---------------------------- 1 file changed, 34 insertions= (+), >> 70 deletions(-) >>=20 >> 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" >>=20 >> +#ifdef IFF_MULTI_QUEUE >> #define RTE_PMD_TAP_MAX_QUEUES 16 >> +#else >> +#define RTE_PMD_TAP_MAX_QUEUES 1 >> +#endif >>=20 >> static struct rte_vdev_driver pmd_tap_drv; >>=20 >> @@ -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; >>=20 >> memset(&ifr, 0, sizeof(struct ifreq)); >>=20 >> ifr.ifr_flags =3D 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); >>=20 >> fd =3D open(TUN_TAP_DEV_PATH, O_RDWR); >> if (fd < 0) { >> @@ -132,37 +139,26 @@ tun_alloc(char *name) >> goto error; >> } >>=20 >> - /* 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); >>=20 >> -#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 =3D=3D 1)) { >> - ifr.ifr_flags |=3D IFF_ONE_QUEUE; >> - RTE_LOG(DEBUG, PMD, "Single queue only support\n"); >> - } else { >> - ifr.ifr_flags |=3D 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 |=3D IFF_MULTI_QUEUE; >> + } else >> +#endif >> + { >> ifr.ifr_flags |=3D IFF_ONE_QUEUE; >> RTE_LOG(DEBUG, PMD, "Single queue only support\n"); >> } >> -#endif >>=20 >> - /* 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; >> } >>=20 >> - /* 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 =3D=3D 0) { >> + if (ioctl(fd, SIOCGIFHWADDR, &ifr) =3D=3D -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); >> + } >>=20 >> return fd; >>=20 >> @@ -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 =3D tun_alloc(pmd->name); >> + fd =3D 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 =3D { >> }; >>=20 >> static int >> -pmd_mac_address(int fd, struct ether_addr *addr) >> -{ >> - struct ifreq ifr; >> - >> - if ((fd <=3D 0) || !addr) >> - return -1; >> - >> - memset(&ifr, 0, sizeof(ifr)); >> - >> - if (ioctl(fd, SIOCGIFHWADDR, &ifr) =3D=3D -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 =3D rte_socket_id(); >> struct rte_eth_dev *dev =3D NULL; >> struct pmd_internals *pmd =3D NULL; >> struct rte_eth_dev_data *data =3D NULL; >> - int i, fd =3D -1; >> + int i; >>=20 >> 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; >> } >>=20 >> - /* Use the name and not the tap_name */ >> dev =3D 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_nam= e) >> dev->tx_pkt_burst =3D pmd_tx_burst; >> snprintf(dev->data->name, sizeof(dev->data->name), "%s", name); >>=20 >> - /* Create the first Tap device */ >> - fd =3D 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 =3D 1; i < RTE_PMD_TAP_MAX_QUEUES; i++) { >> + /* Presetup the fds to -1 as being not valid */ >> + for (i =3D 0; i < RTE_PMD_TAP_MAX_QUEUES; i++) { >> pmd->rxq[i].fd =3D -1; >> pmd->txq[i].fd =3D -1; >> } >>=20 >> - /* Take the TUN/TAP fd and place in the first location */ >> - pmd->rxq[0].fd =3D fd; >> - pmd->txq[0].fd =3D 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; >>=20 >> error_exit: >=20 > I like that you use #ifdef IFF_MULTI_QUEUE, it's a good thing. >=20 > 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. >=20 > dev_stop() should act as dev_start(), in symmetry, and dev_close() > should close the fd/queues. That is a good point should I create v3 or do a patch for that after this r= elease? >=20 > The rest of the code looks OK to me. >=20 > Best regards, > Pascal Regards, Keith