From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from EUR01-HE1-obe.outbound.protection.outlook.com (mail-he1eur01on0040.outbound.protection.outlook.com [104.47.0.40]) by dpdk.org (Postfix) with ESMTP id 8409E25D9; Tue, 22 May 2018 14:09:53 +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=ZHRVYQ0m85tZO0HJH5MRibK+I3jDag/j35puOhp9gRE=; b=Y9h9OQLdQlsuR83yXY6NWvjQ3RlPWWAUdJCl84y81IkG3hylUlI5sESdB8vepBUn1rVouiJIoefUEZ3PW4jOy9nbxbLMudMPDdTnxWXORDbrMyzHqfH8+G1JzPyOYv+Rhd1PF/3oBUdG9zEzXsKBfhoxTpxLEW0+vTKky1yeebA= Received: from VI1PR0501MB2608.eurprd05.prod.outlook.com (10.168.137.20) by VI1PR0501MB1966.eurprd05.prod.outlook.com (10.166.45.9) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384_P256) id 15.20.776.11; Tue, 22 May 2018 12:09:52 +0000 Received: from VI1PR0501MB2608.eurprd05.prod.outlook.com ([fe80::1035:58f9:b94c:2180]) by VI1PR0501MB2608.eurprd05.prod.outlook.com ([fe80::1035:58f9:b94c:2180%18]) with mapi id 15.20.0776.015; Tue, 22 May 2018 12:09:52 +0000 From: Matan Azrad To: =?iso-8859-1?Q?Ga=EBtan_Rivet?= CC: "dev@dpdk.org" , Ophir Munk , "stable@dpdk.org" Thread-Topic: [PATCH v2 1/2] net/failsafe: fix removed sub-device cleanup Thread-Index: AQHT8armzudxZEqf3U6TKovCIcr1G6Q7hjJAgAAdXgCAAARskA== Date: Tue, 22 May 2018 12:09:52 +0000 Message-ID: References: <1526583136-21680-1-git-send-email-matan@mellanox.com> <1526932084-1120-1-git-send-email-matan@mellanox.com> <20180522085656.bx3r3e4c6lz4xwlp@bidouze.vm.6wind.com> <20180522115308.by3cvodasyy4psqt@bidouze.vm.6wind.com> In-Reply-To: <20180522115308.by3cvodasyy4psqt@bidouze.vm.6wind.com> 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-microsoft-exchange-diagnostics: 1; VI1PR0501MB1966; 7:xrKE6V0HeguxqJba/potgSbMBwWNUyOXPtYoVHvDLaEpJFkzSzBUj0U1Fl6u/ZJ7B4NUJL79koIp4Sng3I8oLURVLyCBkTIWiHYxYw62llMftqPW8Dpl1MDY4Vs//rHmaw9886AiROWOb4v2c9+cH+hL9RmB0HXixXV2XxzRzjDKZagyiuLCUWUam2zmJhdjZ7uXuxWN0YgzNI2QYgsDw09YzsvRBpxT5hK8/dZco9ccC1bL3S5YkE3iLs7FAef2 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)(48565401081)(4534165)(4627221)(201703031133081)(201702281549075)(2017052603328)(7153060)(7193020); SRVR:VI1PR0501MB1966; x-ms-traffictypediagnostic: VI1PR0501MB1966: x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:; x-ms-exchange-senderadcheck: 1 x-exchange-antispam-report-cfa-test: BCL:0; PCL:0; RULEID:(8211001083)(6040522)(2401047)(5005006)(8121501046)(3002001)(3231254)(944501410)(52105095)(93006095)(93001095)(10201501046)(6055026)(149027)(150027)(6041310)(20161123564045)(20161123560045)(20161123558120)(20161123562045)(201703131423095)(201702281528075)(20161123555045)(201703061421075)(201703061406153)(6072148)(201708071742011)(7699016); SRVR:VI1PR0501MB1966; BCL:0; PCL:0; RULEID:; SRVR:VI1PR0501MB1966; x-forefront-prvs: 0680FADD48 x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(376002)(39860400002)(346002)(39380400002)(366004)(396003)(199004)(189003)(54906003)(9686003)(86362001)(3846002)(7736002)(6116002)(8676002)(486006)(81156014)(81166006)(6506007)(2906002)(59450400001)(25786009)(106356001)(66066001)(102836004)(7696005)(105586002)(76176011)(99286004)(305945005)(55016002)(33656002)(53936002)(93886005)(97736004)(11346002)(476003)(6436002)(446003)(68736007)(8936002)(316002)(229853002)(4326008)(5660300001)(14454004)(3660700001)(74316002)(6916009)(6246003)(2900100001)(186003)(3280700002)(5250100002)(478600001)(26005); DIR:OUT; SFP:1101; SCL:1; SRVR:VI1PR0501MB1966; H:VI1PR0501MB2608.eurprd05.prod.outlook.com; FPR:; SPF:None; LANG:en; PTR:InfoNoRecords; A:1; MX:1; received-spf: None (protection.outlook.com: mellanox.com does not designate permitted sender hosts) x-microsoft-antispam-message-info: mZTdyqYtqMEd/N8LO6LbCVv++rVHZLP8QHi9Q0SIpK1oePhmyfWWBrr02MBReD/1wglPGTeSCZzsGsJZYrVQtkvGGVnYhku0wF45lRzYBeH2o3SWm84SbPL+Pymeqc3qVb1r9VWWvnFV6EpL5+H3yJVyeS49DBsawiYAa14cn1t9AcVSNaYm26jS12g2+9on spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-MS-Office365-Filtering-Correlation-Id: 4c885ca1-fe3d-4ce1-f2da-08d5bfdcec6a X-OriginatorOrg: Mellanox.com X-MS-Exchange-CrossTenant-Network-Message-Id: 4c885ca1-fe3d-4ce1-f2da-08d5bfdcec6a X-MS-Exchange-CrossTenant-originalarrivaltime: 22 May 2018 12:09:52.1685 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: a652971c-7d2e-4d9b-a6a4-d149256f461b X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR0501MB1966 Subject: Re: [dpdk-dev] [PATCH v2 1/2] net/failsafe: fix removed sub-device cleanup 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: Tue, 22 May 2018 12:09:53 -0000 From: Ga=EBtan Rivet > On Tue, May 22, 2018 at 10:19:14AM +0000, Matan Azrad wrote: > > Hi Gaetan > > > > From: Ga=EBtan Rivet > > > Hello Matan, > > > > > > On Mon, May 21, 2018 at 07:48:03PM +0000, Matan Azrad wrote: > > > > The fail-safe PMD registers to RMV event for each removable > > > > sub-device port in order to cleanup the sub-device resources and > > > > switch the Tx sub-device directly when it is plugged-out. > > > > > > > > During removal time, the fail-safe PMD stops and closes the > > > > sub-device but it doesn't unregister the LSC and RMV callbacks of > > > > the sub-device port. > > > > > > > > It can lead the callbacks to be called for a port which is no more > > > > associated with the fail-safe sub-device, because there is not a > > > > guarantee that a sub-device gets the same port ID for each plug-in > > > > process. This port, for example, may belong to another sub-device > > > > of a different fail-safe device. > > > > > > > > Unregister the LSC and RMV callbacks for sub-devices which are not > > > > used. > > > > > > > > Fixes: 598fb8aec6f6 ("net/failsafe: support device removal") > > > > Cc: stable@dpdk.org > > > > > > > > Signed-off-by: Matan Azrad > > > > --- > > > > drivers/net/failsafe/failsafe_ether.c | 22 +++++++++++++++++++++= + > > > > drivers/net/failsafe/failsafe_ops.c | 5 +++++ > > > > drivers/net/failsafe/failsafe_private.h | 5 +++++ > > > > 3 files changed, 32 insertions(+) > > > > > > > > V2: > > > > Improve the commit log and add code comments for the new sub-dev > > > > fields > > > (Ophir suggestion). > > > > > > > > > > > > diff --git a/drivers/net/failsafe/failsafe_ether.c > > > > b/drivers/net/failsafe/failsafe_ether.c > > > > index 733e95d..2bbee82 100644 > > > > --- a/drivers/net/failsafe/failsafe_ether.c > > > > +++ b/drivers/net/failsafe/failsafe_ether.c > > > > @@ -260,6 +260,7 @@ > > > > sdev->state =3D DEV_ACTIVE; > > > > /* fallthrough */ > > > > case DEV_ACTIVE: > > > > + failsafe_eth_dev_unregister_callbacks(sdev); > > > > rte_eth_dev_close(PORT_ID(sdev)); > > > > sdev->state =3D DEV_PROBED; > > > > /* fallthrough */ > > > > @@ -321,6 +322,27 @@ > > > > } > > > > > > > > void > > > > +failsafe_eth_dev_unregister_callbacks(struct sub_device *sdev) { > > > > + if (sdev =3D=3D NULL) > > > > + return; > > > > + if (sdev->rmv_callback) { > > > > + rte_eth_dev_callback_unregister(PORT_ID(sdev), > > > > + RTE_ETH_EVENT_INTR_RMV, > > > > + failsafe_eth_rmv_event_callback, > > > > + sdev); > > > > + sdev->rmv_callback =3D 0; > > > > > > I agree with Ophir here, either the return value should not be > > > ignored, and rmv_callback should only be set to 0 on success, or a > > > proper justification (and an accompanying comment) should be given. > > > > > > The issue I could see is that even on error, there won't be a > > > process to try again unregistering the callback. > > > > > > Maybe this could be added in failsafe_dev_remove()? Something like > > > > > > FOREACH_SUBDEV(sdev, i, dev) { > > > if (sdev->rmv_callback && sdev->state <=3D DEV_PROBED) > > > if (rte_eth_dev_callback_unregister(...) =3D=3D 0) > > > sdev->rmv_callback =3D 0; > > > /* same for lsc_callback */ > > > } > > > > > > Does it make sense to you? Do you think this is necessary, or should > > > we ignore this? > > > > The RMV\LSC event callbacks are called from the host thread and also th= e > removal process is running from the host thread so I think EAGAIN is not > expected in the removal time. > > Other error (EINVAL) may return again every attempt and probably points= to > another critical issue. > > > > Is a code comment for the above enough? Or you think we still need to c= heck > it? > > > > >=20 > Ok, that makes sense. >=20 > If EINVAL is possible however, I think a warning would be helpful for the= user to > be aware of the issue. The callback flag would then be meaningless anyway= . Ok, thanks, V3 is coming. >=20 > -- > Ga=EBtan Rivet > 6WIND