From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from EUR03-AM5-obe.outbound.protection.outlook.com (mail-eopbgr30063.outbound.protection.outlook.com [40.107.3.63]) by dpdk.org (Postfix) with ESMTP id 15A1B4C75; Fri, 18 May 2018 10:38:05 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=Mellanox.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=SRNs/mGdEnK9tC2QPZjzWTCjcXjaqXGNwLbuHuZKWuU=; b=MfOV46HJqIVMu0c2qEvqPwJKR3Bg3PkYRFzdG36iii9rJpwvGuCshOf4Uhcs/Q1nST1owGm0C5gJJz1w2+bDlY7BesQhKeFt81vLakPdh1J6fZklWmIVPPca7Lhg8mpPu3c1q8NLGk1JNH8YLoKjmCaVAb0RnCbL/Gl23sR/kqo= Received: from HE1PR0501MB2314.eurprd05.prod.outlook.com (10.168.34.19) by HE1PR0501MB2442.eurprd05.prod.outlook.com (10.168.126.10) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384_P256) id 15.20.776.11; Fri, 18 May 2018 08:38:01 +0000 Received: from HE1PR0501MB2314.eurprd05.prod.outlook.com ([fe80::1071:70d0:eac1:5d97]) by HE1PR0501MB2314.eurprd05.prod.outlook.com ([fe80::1071:70d0:eac1:5d97%18]) with mapi id 15.20.0776.010; Fri, 18 May 2018 08:38:01 +0000 From: Ophir Munk To: "Wiles, Keith" CC: "dev@dpdk.org" , Pascal Mazon , Thomas Monjalon , Olga Shern , Shahaf Shuler , "stable@dpdk.org" Thread-Topic: [PATCH v2] net/tap: fix device removal when no queues exist Thread-Index: AQHT7cQdH1m4hkYm+ki/osK5rB5mIaQz4mAAgAFGzXA= Date: Fri, 18 May 2018 08:38:01 +0000 Message-ID: References: <1526487019-3737-1-git-send-email-ophirmu@mellanox.com> <1526550455-14072-1-git-send-email-ophirmu@mellanox.com> <1E0EA271-1891-4336-BCB8-A1A6A6F0BDF3@intel.com> In-Reply-To: <1E0EA271-1891-4336-BCB8-A1A6A6F0BDF3@intel.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: spf=none (sender IP is ) smtp.mailfrom=ophirmu@mellanox.com; x-originating-ip: [85.250.111.138] x-ms-publictraffictype: Email x-microsoft-exchange-diagnostics: 1; HE1PR0501MB2442; 7:CdU1GaZKliyq81AIeMbOylrwOYRJ8GcwB3lt3mtk6y7bTJHsVk9LMUTVOoanzoQbIM1sIlxY754SwTK28oo9DdvYSYz6RWtPUQ/GHpQDhbFaLwQuRT3w5SmDJc8qX4k97jrlK4WFZyE7fDKiEYBL65ZRs/RhBV4tQAjRQ/3w/tyFzIpnan75a8vJIwQ2PqC7FAuLpSx2/Y1xA2onNouCas9BYz5EopHwZdgBB12IOpHubh0tYZxNS6yxVVgFbQp0 x-ms-exchange-antispam-srfa-diagnostics: SOS; x-ms-office365-filtering-ht: Tenant x-microsoft-antispam: UriScan:; BCL:0; PCL:0; RULEID:(7020095)(4652020)(5600026)(4534165)(4627221)(201703031133081)(201702281549075)(48565401081)(2017052603328)(7153060)(7193020); SRVR:HE1PR0501MB2442; x-ms-traffictypediagnostic: HE1PR0501MB2442: x-ld-processed: a652971c-7d2e-4d9b-a6a4-d149256f461b,ExtAddr x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:(228905959029699); x-ms-exchange-senderadcheck: 1 x-exchange-antispam-report-cfa-test: BCL:0; PCL:0; RULEID:(8211001083)(6040522)(2401047)(8121501046)(5005006)(3231254)(944501410)(52105095)(93006095)(93001095)(3002001)(10201501046)(6055026)(149027)(150027)(6041310)(20161123558120)(20161123564045)(20161123560045)(20161123562045)(201703131423095)(201702281528075)(20161123555045)(201703061421075)(201703061406153)(6072148)(201708071742011)(7699016); SRVR:HE1PR0501MB2442; BCL:0; PCL:0; RULEID:; SRVR:HE1PR0501MB2442; x-forefront-prvs: 0676F530A9 x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(979002)(39380400002)(346002)(366004)(376002)(396003)(39860400002)(13464003)(199004)(189003)(86362001)(54906003)(6116002)(68736007)(478600001)(14454004)(316002)(3846002)(26005)(229853002)(186003)(66066001)(59450400001)(6436002)(6506007)(53546011)(2906002)(102836004)(76176011)(7696005)(2900100001)(6246003)(55016002)(9686003)(53936002)(4326008)(105586002)(106356001)(6916009)(33656002)(99286004)(25786009)(3280700002)(5250100002)(5660300001)(3660700001)(97736004)(8676002)(486006)(446003)(11346002)(476003)(305945005)(7736002)(74316002)(81156014)(8936002)(81166006)(969003)(989001)(999001)(1009001)(1019001); DIR:OUT; SFP:1101; SCL:1; SRVR:HE1PR0501MB2442; H:HE1PR0501MB2314.eurprd05.prod.outlook.com; FPR:; SPF:None; LANG:en; PTR:InfoNoRecords; A:1; MX:3; received-spf: None (protection.outlook.com: mellanox.com does not designate permitted sender hosts) x-microsoft-antispam-message-info: E4zAg1OxW8rCNzOS3IYBYpJ5I+9Vhn7VAgYIaltkvuw9UKPYX0DZhFA+YqqUw6IRoDSqwFk+voSn4WnxDVy5n8+vpTdHdM5Ix7Vn6y6A88x6qd6DoJecGNVEXSB24zeSelffLS+nXsoazVUfIVcJFoPBu3qfPlu16k4D4bp4QbYtGRl/4NBs7Wz9glK+gvmf spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-MS-Office365-Filtering-Correlation-Id: 3cc905f4-febb-43bb-ed73-08d5bc9aaac5 X-OriginatorOrg: Mellanox.com X-MS-Exchange-CrossTenant-Network-Message-Id: 3cc905f4-febb-43bb-ed73-08d5bc9aaac5 X-MS-Exchange-CrossTenant-originalarrivaltime: 18 May 2018 08:38:01.7783 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: a652971c-7d2e-4d9b-a6a4-d149256f461b X-MS-Exchange-Transport-CrossTenantHeadersStamped: HE1PR0501MB2442 Subject: Re: [dpdk-dev] [PATCH v2] net/tap: fix device removal when no queues exist 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: Fri, 18 May 2018 08:38:05 -0000 Hi Keith, Please find comments inline > -----Original Message----- > From: Wiles, Keith [mailto:keith.wiles@intel.com] > Sent: Thursday, May 17, 2018 4:00 PM > To: Ophir Munk > Cc: dev@dpdk.org; Pascal Mazon ; Thomas > Monjalon ; Olga Shern ; > Shahaf Shuler ; stable@dpdk.org > Subject: Re: [PATCH v2] net/tap: fix device removal when no queues exist >=20 >=20 >=20 > > On May 17, 2018, at 2:47 AM, Ophir Munk > wrote: > > > > TAP device is created following its first queue creation. Multiple > > queues can be added or removed over time. In Linux terminology those > > are file descriptors which are opened or closed over time. As long as > > the number of opened file descriptors is positive - TAP device will > > appear as a Linux device. In case all queues are released (the > > equivalent of all file descriptors being closed) the TAP device will > > be removed. This can lead to abnormalities in different scenarios > > where the TAP device should exist even if all its queues are released. > > In order to make TAP existence independent of its number of queues - > > an extra file descriptor is opened on TAP creation and is closed on > > TAP closure. It's only purpose is to serve as a keep-alive mechanism > > for the TAP device. > > > > Fixes: bf7b7f437b49 ("net/tap: create netdevice during probing") > > Cc: stable@dpdk.org > > > > Signed-off-by: Ophir Munk > > --- > > v1: > > Initial release > > v2: > > Reword commit message (a fixing patch) > > > > drivers/net/tap/rte_eth_tap.c | 31 ++++++++++++++++++++++++------- > > drivers/net/tap/rte_eth_tap.h | 1 + > > 2 files changed, 25 insertions(+), 7 deletions(-) >=20 > I did not see where ka_fd is set to -1 at startup, just in case we fail b= efore the > first open attempt and possible hit the close code in the remove routine.= I did > not look at the complete driver, but I think it maybe reasonable to add t= hat > initial variable setup. >=20 Your concern is in place. However, please note that ka_fd is allocated and = initialized to 0 as part of private device structure initialization during = the call to: dev =3D rte_eth_vdev_allocate(vdev, sizeof(*pmd)); Therefore it is guaranteed to be 0 in case we fail before the first open at= tempt and if we hit the close code we will not be closing anything wrong (c= losing a 0 fd has no effect).=20 While reading the code again I have noticed that tun_alloc() always return = -1 so no need to reassign ka_fd to -1 after calling tun_alloc. Also I have = fixed a typo in the commit message: It's --> Its. I have sent v3 with the above small changes. > > > > diff --git a/drivers/net/tap/rte_eth_tap.c > > b/drivers/net/tap/rte_eth_tap.c index c006d07..6901edc 100644 > > --- a/drivers/net/tap/rte_eth_tap.c > > +++ b/drivers/net/tap/rte_eth_tap.c > > @@ -929,6 +929,15 @@ tap_dev_close(struct rte_eth_dev *dev) > > ioctl(internals->ioctl_sock, SIOCSIFFLAGS, > > &internals->remote_initial_flags); > > } > > + > > + if (internals->ka_fd !=3D -1) { > > + close(internals->ka_fd); > > + internals->ka_fd =3D -1; > > + } > > + /* > > + * Since TUN device has no more opened file descriptors > > + * it will be removed from kernel > > + */ > > } > > > > static void > > @@ -1561,13 +1570,18 @@ eth_dev_tap_create(struct rte_vdev_device > *vdev, char *tap_name, > > rte_memcpy(&pmd->eth_addr, mac_addr, > sizeof(*mac_addr)); > > } > > > > - /* Immediately create the netdevice (this will create the 1st queue). > */ > > - /* rx queue */ > > - if (tap_setup_queue(dev, pmd, 0, 1) =3D=3D -1) > > - goto error_exit; > > - /* tx queue */ > > - if (tap_setup_queue(dev, pmd, 0, 0) =3D=3D -1) > > + /* > > + * Allocate a TUN device keep-alive file descriptor that will only be > > + * closed when the TUN device itself is closed or removed. > > + * This keep-alive file descriptor will guarantee that the TUN device > > + * exists even when all of its queues are closed > > + */ > > + pmd->ka_fd =3D tun_alloc(pmd); > > + if (pmd->ka_fd < 0) { > > + pmd->ka_fd =3D -1; > > + TAP_LOG(ERR, "Unable to create %s interface", > tuntap_name); > > goto error_exit; > > + } > > > > ifr.ifr_mtu =3D dev->data->mtu; > > if (tap_ioctl(pmd, SIOCSIFMTU, &ifr, 1, LOCAL_AND_REMOTE) < 0) > @@ > > -1961,9 +1975,12 @@ rte_pmd_tap_remove(struct rte_vdev_device *dev) > > > > close(internals->ioctl_sock); > > rte_free(eth_dev->data->dev_private); > > - > > rte_eth_dev_release_port(eth_dev); > > > > + if (internals->ka_fd !=3D -1) { > > + close(internals->ka_fd); > > + internals->ka_fd =3D -1; > > + } > > return 0; > > } > > > > diff --git a/drivers/net/tap/rte_eth_tap.h > > b/drivers/net/tap/rte_eth_tap.h index babe42d..575dce4 100644 > > --- a/drivers/net/tap/rte_eth_tap.h > > +++ b/drivers/net/tap/rte_eth_tap.h > > @@ -81,6 +81,7 @@ struct pmd_internals { > > struct rx_queue rxq[RTE_PMD_TAP_MAX_QUEUES]; /* List of RX > queues */ > > struct tx_queue txq[RTE_PMD_TAP_MAX_QUEUES]; /* List of TX > queues */ > > struct rte_intr_handle intr_handle; /* LSC interrupt handle. = */ > > + int ka_fd; /* keep-alive file descriptor */ > > }; > > > > /* tap_intr.c */ > > -- > > 2.7.4 > > >=20 > Regards, > Keith