From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id C6B13A0525; Sun, 16 Feb 2020 09:09:07 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 971DA1B952; Sun, 16 Feb 2020 09:09:06 +0100 (CET) Received: from EUR05-DB8-obe.outbound.protection.outlook.com (mail-db8eur05on2089.outbound.protection.outlook.com [40.107.20.89]) by dpdk.org (Postfix) with ESMTP id BEB2E374; Sun, 16 Feb 2020 09:09:04 +0100 (CET) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=G92g9jdRW91wiRnJwLlAJx6wmdXGa2umkz/HS7aknwO9BRXX44QyhGPBPTd+0gcsF/hpBCEzxeLxXB/mUiwUDEfCXUrVWWtpcWIxfpn0MfIqHxfNpeZiDrg5LgaI3r0I2zQsQMM2ZqGljBJx29KeTsHbAhCb9j8DQ9Zsaz/WsC//oHKX9zLsNKN7zTU8yuTDyGShWW+q+j72CQBF5FM82yMpAE1mm+YH62r3ROalCopdWqUuu8SAGHIB85sfIC/gqg+LLAskzpFYNBGapuMj0AOE+tkbRw/9X6RljXlJUU8LYLL+CSNGPkOMKr5fqoPYzRAKLIyoNjM/HpKAH91cgQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=MT+0Ytp8ofVOmkM9uf/6VmVUtQGwJ7ZpiK7dYnN0pnc=; b=IBpDbkL5DscH/EYaCJj49v4+cIxLtvGicVGl/JA0S0WfXaFP/MzRujz3KwFATfb7MPIfyi15VvkzhGxj3x9+FRbAorhKWIkRgDtEXoklTR1V3dqzDFL+7WN58IYl6txbDkB3svyxMIWWgUTzhyj568tFwl9hNEKtbcdtRGzo5jsNPjtmOGRDENmV8ADyiKm9hhWApNWpsyx386mdIk9UhoEZY68ObVGWoW+v58pZyZekZqKPHuBwcJWOG+/kn/5oaPIG6wHxk3RuPW8394qZ5HeyBiImOeXvv6VSXD+ZZ1t4Ese+i42ZAuJ/bwTle5TtQTfnzZ8r4PX6Eyhjfv0Y+w== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=mellanox.com; dmarc=pass action=none header.from=mellanox.com; dkim=pass header.d=mellanox.com; arc=none 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=MT+0Ytp8ofVOmkM9uf/6VmVUtQGwJ7ZpiK7dYnN0pnc=; b=bTp7KvxufJ+p/mxTn1uaeNAWnkhrvOBCyMv79lqUP1PeGSks6/2lSgO3oK6glwPcEyrMuGnFGzJzepjpaDDUpuoYrHCsZFHawGgmuxduLvgKCYtY09ZknZT6mH07tsig79yvWFP/4OeBLwv+m9p2YVa8S8GwSq1hxgc5+jDsMaY= Received: from AM0PR0502MB4019.eurprd05.prod.outlook.com (52.133.39.139) by AM0PR0502MB3828.eurprd05.prod.outlook.com (52.133.47.142) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.2729.25; Sun, 16 Feb 2020 08:09:03 +0000 Received: from AM0PR0502MB4019.eurprd05.prod.outlook.com ([fe80::e495:9258:db09:1de8]) by AM0PR0502MB4019.eurprd05.prod.outlook.com ([fe80::e495:9258:db09:1de8%7]) with mapi id 15.20.2707.031; Sun, 16 Feb 2020 08:09:03 +0000 From: Matan Azrad To: Thomas Monjalon , "dev@dpdk.org" CC: "ferruh.yigit@intel.com" , "stable@dpdk.org" , Wenzhuo Lu , Jingjing Wu , Bernard Iremonger Thread-Topic: [PATCH 2/2] app/testpmd: fix hot-unplug detaching Thread-Index: AQHV4oWezsDo1eyNk0Gy+pjDKgZ/MqgddTuw Date: Sun, 16 Feb 2020 08:09:03 +0000 Message-ID: References: <20200213155226.1024939-1-thomas@monjalon.net> <20200213155226.1024939-3-thomas@monjalon.net> In-Reply-To: <20200213155226.1024939-3-thomas@monjalon.net> Accept-Language: en-US, he-IL Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: spf=none (sender IP is ) smtp.mailfrom=matan@mellanox.com; x-originating-ip: [193.47.165.251] x-ms-publictraffictype: Email x-ms-office365-filtering-ht: Tenant x-ms-office365-filtering-correlation-id: 3240377b-5073-43a3-4227-08d7b2b77c94 x-ms-traffictypediagnostic: AM0PR0502MB3828: x-ld-processed: a652971c-7d2e-4d9b-a6a4-d149256f461b,ExtAddr x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:7219; x-forefront-prvs: 03152A99FF x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(4636009)(39850400004)(136003)(396003)(366004)(376002)(346002)(189003)(199004)(5660300002)(186003)(4326008)(7696005)(26005)(81156014)(8676002)(81166006)(8936002)(2906002)(316002)(110136005)(54906003)(6506007)(66476007)(71200400001)(55016002)(478600001)(66556008)(9686003)(66446008)(64756008)(33656002)(76116006)(52536014)(66946007)(86362001); DIR:OUT; SFP:1101; SCL:1; SRVR:AM0PR0502MB3828; H:AM0PR0502MB4019.eurprd05.prod.outlook.com; FPR:; SPF:None; LANG:en; PTR:InfoNoRecords; MX:1; A:1; received-spf: None (protection.outlook.com: mellanox.com does not designate permitted sender hosts) x-ms-exchange-senderadcheck: 1 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: cnQlllnFJ/rqNn9wOkIrIBQ7/m2uY4UDiqUSzkkItMg2EeLiUf6JEGxGTPiLaPi6FPsoRknL3qNOlZEdKHZq5wa5NboSFntl2en2JrfqngiDbinH0euvX1lerYvSSLdN5SDXMBWizXZWYwfuqB+FZN74FNag7f/QEkDORZwGUXMNcZr4sMm1gZzotbFg+9FZxlrwlZ3/zuPZ3vsi3nJbN14NFcY29DN6xiVP5IJTuFPBDX24pRdWDwnpUlcRvDLhA/bALNQibmimkWf1+oL1jf+LvaA6kvp8w8RuhAQtkxYwozoBx8msqsN81OAbs6ySHHfoW+zcG8KaI+jI866a02ECSk9YCAbDU/g0dWRtPrXIS5e5lCc5KMEKO3QmPdw0bcG+8yFjT23Ry7VWkJCnnY72U4qV1kYXThoUeoh1yky7Yv11Vk+zskKY6+rr9yFO x-ms-exchange-antispam-messagedata: e2sFsmD17e11A8dMdLk+J7q6SI5CWGwQpkTryrqK5eyPePcuuyjLFsHznPGrOD/l0AQbZa9RW61limTY/a9sM1lKtsvaurVHbXQ07XjzCBvnz6nHNtD+m52vCuzJIq78R/rO3/2T73BVN+0uTPVtqA== x-ms-exchange-transport-forked: True Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-OriginatorOrg: Mellanox.com X-MS-Exchange-CrossTenant-Network-Message-Id: 3240377b-5073-43a3-4227-08d7b2b77c94 X-MS-Exchange-CrossTenant-originalarrivaltime: 16 Feb 2020 08:09:03.4385 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: a652971c-7d2e-4d9b-a6a4-d149256f461b X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: WipuUX3fPLi6TprhBPm6YIF9AT3BUkVVyP3ofHVlkFdBBEFBBaQGFs6m7TPogyVEEhGz72nxy0+5qPIEUT7OOA== X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM0PR0502MB3828 Subject: Re: [dpdk-dev] [PATCH 2/2] app/testpmd: fix hot-unplug detaching 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: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hi Thomas Thanks for the patches, I Saw it just now. please see small comment below: From: Thomas Monjalon > There is a possible race condition in the hotplug path in rmv_port_callba= ck(). > If a port is created between > close_port(port_id) and detach_port_device(port_id), then the port_id wil= l > have been reallocated to a different device which will be wrongly detache= d. >=20 > Since a check was added in detach_port_device() for manual detach case, > the hotplug path was even more broken. > It became impossible to run because the new check prevented to run > detach_port_device() after the port is closed. >=20 > The solution for both issues is to not rely on the port_id for detaching = the > rte_device. > The function detach_port_device() is split to allow calling > detach_device() directly with the rte_device pointer, saved before closin= g > the port. >=20 > Fixes: 43d0e304980a ("app/testpmd: fix invalid port detaching") If you fix the race, Don't you think you need to add fixes line for the pat= ch which created the race? > Cc: stable@dpdk.org >=20 > Signed-off-by: Thomas Monjalon > --- > app/test-pmd/testpmd.c | 48 ++++++++++++++++++++++++------------------ > 1 file changed, 28 insertions(+), 20 deletions(-) >=20 > diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index > 11203cb03d..035836adfb 100644 > --- a/app/test-pmd/testpmd.c > +++ b/app/test-pmd/testpmd.c > @@ -2633,32 +2633,17 @@ setup_attached_port(portid_t pi) > printf("Done\n"); > } >=20 > -void > -detach_port_device(portid_t port_id) > +static void > +detach_device(struct rte_device *dev) > { > - struct rte_device *dev; > portid_t sibling; >=20 > - printf("Removing a device...\n"); > - > - if (port_id_is_invalid(port_id, ENABLED_WARN)) > - return; > - > - dev =3D rte_eth_devices[port_id].device; > if (dev =3D=3D NULL) { > printf("Device already removed\n"); > return; > } >=20 > - if (ports[port_id].port_status !=3D RTE_PORT_CLOSED) { > - if (ports[port_id].port_status !=3D RTE_PORT_STOPPED) { > - printf("Port not stopped\n"); > - return; > - } > - printf("Port was not closed\n"); > - if (ports[port_id].flow_list) > - port_flow_flush(port_id); > - } > + printf("Removing a device...\n"); >=20 > if (rte_dev_remove(dev) < 0) { > TESTPMD_LOG(ERR, "Failed to detach device %s\n", dev- > >name); @@ -2676,13 +2661,31 @@ detach_port_device(portid_t port_id) >=20 > remove_invalid_ports(); >=20 > - printf("Device of port %u is detached\n", port_id); > + printf("Device is detached\n"); > printf("Now total ports is %d\n", nb_ports); > printf("Done\n"); > return; > } >=20 > void > +detach_port_device(portid_t port_id) > +{ > + if (port_id_is_invalid(port_id, ENABLED_WARN)) > + return; > + > + if (ports[port_id].port_status !=3D RTE_PORT_CLOSED) { > + if (ports[port_id].port_status !=3D RTE_PORT_STOPPED) { > + printf("Port not stopped\n"); > + return; > + } > + printf("Port was not closed\n"); > + if (ports[port_id].flow_list) > + port_flow_flush(port_id); > + } > + > + detach_device(rte_eth_devices[port_id].device); > +} > + > void > detach_devargs(char *identifier) > { > @@ -2873,6 +2876,7 @@ rmv_port_callback(void *arg) > int need_to_start =3D 0; > int org_no_link_check =3D no_link_check; > portid_t port_id =3D (intptr_t)arg; > + struct rte_device *dev; >=20 > RTE_ETH_VALID_PORTID_OR_RET(port_id); >=20 > @@ -2883,8 +2887,12 @@ rmv_port_callback(void *arg) > no_link_check =3D 1; > stop_port(port_id); > no_link_check =3D org_no_link_check; > + > + /* Save rte_device pointer before closing ethdev port */ > + dev =3D rte_eth_devices[port_id].device; > close_port(port_id); > - detach_port_device(port_id); > + detach_device(dev); /* might be already removed or have more > ports */ > + > if (need_to_start) > start_packet_forwarding(0); > } > -- > 2.25.0