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 96755A00BE; Wed, 20 Apr 2022 08:55:59 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 28DE34068E; Wed, 20 Apr 2022 08:55:59 +0200 (CEST) Received: from smartserver.smartsharesystems.com (smartserver.smartsharesystems.com [77.243.40.215]) by mails.dpdk.org (Postfix) with ESMTP id C408640687 for ; Wed, 20 Apr 2022 08:55:57 +0200 (CEST) X-MimeOLE: Produced By Microsoft Exchange V6.5 Content-class: urn:content-classes:message MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Subject: RE: [RFC] eal: add bus cleanup to eal cleanup Date: Wed, 20 Apr 2022 08:55:54 +0200 Message-ID: <98CBD80474FA8B44BF855DF32C47DC35D86FE1@smartserver.smartshare.dk> In-Reply-To: <20220419161438.1837860-1-kevin.laatz@intel.com> X-MS-Has-Attach: X-MS-TNEF-Correlator: Thread-Topic: [RFC] eal: add bus cleanup to eal cleanup Thread-Index: AdhUCJnfc3LUjTnDQzak1TBAme83PAAdqCHg References: <20220419161438.1837860-1-kevin.laatz@intel.com> From: =?iso-8859-1?Q?Morten_Br=F8rup?= To: "Kevin Laatz" , 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 > From: Kevin Laatz [mailto:kevin.laatz@intel.com] > Sent: Tuesday, 19 April 2022 18.15 >=20 > During EAL init, all buses are probed and the devices found are > initialized. On eal_cleanup(), the inverse does not happen, meaning = any > allocated memory and other configuration will not be cleaned up > appropriately on exit. >=20 > Currently, in order for device cleanup to take place, applications = must > call the driver-relevant functions to ensure proper cleanup is done > before > the application exits. Since initialization occurs for all devices on > the > bus, not just the devices used by an application, it requires a) > application awareness of all bus devices that could have been probed = on > the > system, and b) code duplication across applications to ensure cleanup > is > performed. An example of this is rte_eth_dev_close() which is commonly > used > across the example applications. >=20 > This RFC proposes adding bus cleanup to the eal_cleanup() to make = EAL's > init/exit more symmetrical, ensuring all bus devices are cleaned up > appropriately without the application needing to be aware of all bus > types > that may have been probed during initialization. >=20 > Contained in this RFC are the changes required to perform cleanup for > devices on the PCI bus during eal_cleanup(). This can be expanded in > subsequent versions if these changes are desired. There would be an = ask > for > bus maintainers to add the relevant cleanup for their buses since they > have > the domain expertise. >=20 > Signed-off-by: Kevin Laatz > --- [...] > + RTE_LOG(INFO, EAL, > + "Clean up PCI driver: %s (%x:%x) device: > "PCI_PRI_FMT" (socket %i)\n", > + drv->driver.name, dev->id.vendor_id, dev- > >id.device_id, > + loc->domain, loc->bus, loc->devid, loc- > >function, > + dev->device.numa_node); I agree with Stephen, this message might as well be DEBUG level. You = could argue for symmetry: If the "alloc" message during startup is INFO = level, it makes sense using INFO level for the "free" message during = cleanup too. However, the message probably has far lower information = value during cleanup (because this driver cleanup is expected to = happen), so I would degrade it to DEBUG level. Symmetry is not always = the strongest argument. I have no strong preference, so I'll leave it up = to you, Kevin. [...] > @@ -263,6 +275,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_cleanup_t cleanup; /**< Cleanup devices on bus */ > rte_bus_find_device_t find_device; /**< Find a device on the bus > */ > rte_bus_plug_t plug; /**< Probe single device for drivers > */ > rte_bus_unplug_t unplug; /**< Remove single device from > driver */ Have you considered if modifying the rte_bus structure in = /lib/eal/include/rte_bus.h breaks the ABI or not? Overall, this patch is certainly a good idea! On the condition that modifying the rte_bus structure does not break the = ABI... Acked-by: Morten Br=F8rup