From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id CC11DA04A5; Wed, 9 Feb 2022 14:21:05 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 93E8F41143; Wed, 9 Feb 2022 14:21:05 +0100 (CET) Received: from new2-smtp.messagingengine.com (new2-smtp.messagingengine.com [66.111.4.224]) by mails.dpdk.org (Postfix) with ESMTP id 2740740140 for ; Wed, 9 Feb 2022 14:21:04 +0100 (CET) Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailnew.nyi.internal (Postfix) with ESMTP id 8B2855800E3; Wed, 9 Feb 2022 08:21:02 -0500 (EST) Received: from mailfrontend2 ([10.202.2.163]) by compute1.internal (MEProxy); Wed, 09 Feb 2022 08:21:02 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h= cc:cc:content-transfer-encoding:content-type:date:date:from:from :in-reply-to:in-reply-to:message-id:mime-version:references :reply-to:sender:subject:subject:to:to; s=fm3; bh=uNoCADxmZiXBFi /mlNFM0mHKyKJvtihVCKjk6A5NY64=; b=WsClvq4moVHSeL0JyIx7RBp6sGVW9L VYZ3tKtuuGP42g+K4N7rLVHoQpG8gtSbRQxJzQFYQWQP4xRH+07Zr3pI7lS6BndL 9dp2Ao7+856DDrxcwHrP/bt59VRHK1WxRcccNetoXaHvEQ6E8gKwOmyF9mpbbWna RImuMDIKE1OTtxgjoMC48UsWImumaSt8AnAfV/krEU2hVo43o7OwStMmlWOWRIVG 9vP9thxHH4n+Sr+xYWnXLW1PN6cq0y2NTCtbprVc/a6JCNMT0X0zes7L2CQfursI w+8WtigEXcf5RjSlgv9YfaaejWGODLNZYO+aLFo9ML7H3jQvrIUiDVYQ== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-transfer-encoding :content-type:date:date:from:from:in-reply-to:in-reply-to :message-id:mime-version:references:reply-to:sender:subject :subject:to:to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender :x-sasl-enc; s=fm2; bh=uNoCADxmZiXBFi/mlNFM0mHKyKJvtihVCKjk6A5NY 64=; b=aOOG4F+qUm7txd9dZBD3rlgOTIpF+NGmX/QMArnJUKhhQtqfNnh1kdnbK 4H60w52iMqZREE2XLFsEitmI63jwppr1+bRn1jlizgYVDYyPsYELvfCPg5Ym2N1z KRmUeQE7IYhY7nCrZSfLqJwWbsLPmJuv0b0UA0YGFnKm6c35PSPAHJMSAwgonuIR NJKF/AYYTboJSkR6piiDFHpX/RnkgIJjaB8Ac6J7e0DVH+WEAM3dfRjv4jIBLKyT 8LA13UVblKwj/P7m3OudukTXq63wG0Lpey6EdyGPO9fAUVvbNh2g53ogxYGtEAN2 qH8EvurCB/8Hx96iSXgEcYw7iCozg== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvvddrheelgdehtdcutefuodetggdotefrodftvf curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu uegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenuc fjughrpefhvffufffkjghfggfgtgesthfuredttddtvdenucfhrhhomhepvfhhohhmrghs ucfoohhnjhgrlhhonhcuoehthhhomhgrshesmhhonhhjrghlohhnrdhnvghtqeenucggtf frrghtthgvrhhnpedugefgvdefudfftdefgeelgffhueekgfffhfeujedtteeutdejueei iedvffegheenucevlhhushhtvghrufhiiigvpedtnecurfgrrhgrmhepmhgrihhlfhhroh hmpehthhhomhgrshesmhhonhhjrghlohhnrdhnvght X-ME-Proxy: Received: by mail.messagingengine.com (Postfix) with ESMTPA; Wed, 9 Feb 2022 08:20:59 -0500 (EST) From: Thomas Monjalon To: Rohit Raj Cc: dev@dpdk.org, Bruce Richardson , Ray Kinsella , Dmitry Kozlyuk , Narcisa Ana Maria Vasile , Dmitry Malloy , Pallavi Kadam , dev , Nipun Gupta , Sachin Saxena , Hemant Agrawal , David Marchand Subject: Re: [PATCH v5 1/2] eal: add API for bus close Date: Wed, 09 Feb 2022 14:20:57 +0100 Message-ID: <4782321.0VBMTVartN@thomas> In-Reply-To: References: <20201008153048.19369-1-rohit.raj@nxp.com> <20220110052627.22577-1-rohit.raj@nxp.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org 09/02/2022 12:04, David Marchand: > On Mon, Jan 10, 2022 at 6:26 AM wrote: > > > > From: Rohit Raj > > > > As per the current code we have API for bus probe, but the > > bus close API is missing. This breaks the multi process > > scenarios as objects are not cleaned while terminating the > > secondary processes. > > After an application crash, how does this bus resets the associated resources? > > > This patch adds a new API rte_bus_close() for cleanup of > > bus objects which were acquired during probe. > > The patch in its current form breaks the ABI on rte_bus object. > This can be seen in GHA, or calling DPDK_ABI_REF_VERSION=v21.11 > ./devtools/test-meson-builds.sh. [...] > > +/* Close all devices of all buses */ > > +int > > +rte_bus_close(void) > > +{ > > + int ret; > > + struct rte_bus *bus, *vbus = NULL; > > + > > + TAILQ_FOREACH(bus, &rte_bus_list, next) { > > + if (!strcmp(bus->name, "vdev")) { Please do an explicit comparison "== 0". > > + vbus = bus; > > + continue; > > + } > > + > > + if (bus->close) { Please do an explicit comparison with "!= NULL". We can also completely remove this check and implement the callback in all buses. It should loop in all remaining devices and remove them. > > + ret = bus->close(); > > + if (ret) > > + RTE_LOG(ERR, EAL, "Bus (%s) close failed.\n", > > + bus->name); > > + } > > + } > > + > > + if (vbus && vbus->close) { > > + ret = vbus->close(); > > + if (ret) > > + RTE_LOG(ERR, EAL, "Bus (%s) close failed.\n", > > + vbus->name); > > + } > > The vdev bus is special in that some drivers can reference objects > from other buses (see f4ce209a8ce5 ("eal: postpone vdev > initialization") and da76cc02342b ("eal: probe new virtual bus after > other bus devices")). > For this reason, I would expect that the vdev bus is closed before the > other buses. Yes, good catch. We don't have to expose this function as API. This can be an internal function called only in rte_eal_cleanup(). Instead, it would be more useful to have a public function to close a single bus by its name. > > @@ -263,6 +280,7 @@ struct rte_bus { > > const char *name; /**< Name of the bus */ > > rte_bus_scan_t scan; /**< Scan for devices attached to bus */ > > rte_bus_probe_t probe; /**< Probe devices on bus */ > > + rte_bus_close_t close; /**< Close devices on bus */ As David said, it is breaking the ABI. [...] > > @@ -1362,6 +1362,14 @@ rte_eal_cleanup(void) > > > > if (rte_eal_process_type() == RTE_PROC_PRIMARY) > > rte_memseg_walk(mark_freeable, NULL); > > + > > + /* Close all the buses and devices/drivers on them */ > > + if (rte_bus_close()) { > > + rte_eal_init_alert("Cannot close devices"); > > You can't call rte_eal_*init*_alert in rte_eal_cleanup. > > There is not much to do if the bus close fails, I'd rather leave the > cleanup continue. +1, just log and save the error code for return at the end.