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 09BFEA0548; Thu, 2 Jun 2022 04:06:32 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 4974E40694; Thu, 2 Jun 2022 04:06:31 +0200 (CEST) Received: from szxga08-in.huawei.com (szxga08-in.huawei.com [45.249.212.255]) by mails.dpdk.org (Postfix) with ESMTP id D98054021E for ; Thu, 2 Jun 2022 04:06:29 +0200 (CEST) Received: from kwepemi100004.china.huawei.com (unknown [172.30.72.56]) by szxga08-in.huawei.com (SkyGuard) with ESMTP id 4LD8Tq0X5Lz1K98n; Thu, 2 Jun 2022 10:04:47 +0800 (CST) Received: from kwepemm600004.china.huawei.com (7.193.23.242) by kwepemi100004.china.huawei.com (7.221.188.70) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2375.24; Thu, 2 Jun 2022 10:06:27 +0800 Received: from [10.67.103.231] (10.67.103.231) by kwepemm600004.china.huawei.com (7.193.23.242) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2375.24; Thu, 2 Jun 2022 10:06:27 +0800 Message-ID: Date: Thu, 2 Jun 2022 10:06:26 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.2.0 Subject: Re: [PATCH v6] eal: add bus cleanup to eal cleanup To: Kevin Laatz , CC: =?UTF-8?Q?Morten_Br=c3=b8rup?= References: <20220419161438.1837860-1-kevin.laatz@intel.com> <20220601170234.11100-1-kevin.laatz@intel.com> From: "lihuisong (C)" In-Reply-To: <20220601170234.11100-1-kevin.laatz@intel.com> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.67.103.231] X-ClientProxiedBy: dggems701-chm.china.huawei.com (10.3.19.178) To kwepemm600004.china.huawei.com (7.193.23.242) X-CFilter-Loop: Reflected 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 Hi Kevin, 在 2022/6/2 1:02, Kevin Laatz 写道: > 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. > > 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. > > This patch 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. > > Contained in this patch are the changes required to perform cleanup for > devices on the PCI bus and VDEV bus during eal_cleanup(). There would be an > ask for bus maintainers to add the relevant cleanup for their buses since > they have the domain expertise. > > Signed-off-by: Kevin Laatz > Acked-by: Morten Brørup > > --- > v6: > * add bus_cleanup to eal_cleanup for FreeBSD > * add bus_cleanup to eal_cleanup for Windows > * remove bus cleanup function to remove rte_ prefix > * other minor fixes > > v5: > * remove unnecessary logs > * move rte_bus_cleanup() definition to eal_private.h > * fix return values for vdev_cleanup and pci_cleanup > > v4: > * rebase > > v3: > * add vdev bus cleanup > > v2: > * change log level from INFO to DEBUG for PCI cleanup > * add abignore entries for rte_bus related false positives > > --- > devtools/libabigail.abignore | 9 +++++++++ > drivers/bus/pci/pci_common.c | 24 ++++++++++++++++++++++++ > drivers/bus/vdev/vdev.c | 24 ++++++++++++++++++++++++ > lib/eal/common/eal_common_bus.c | 17 +++++++++++++++++ > lib/eal/common/eal_private.h | 10 ++++++++++ > lib/eal/freebsd/eal.c | 1 + > lib/eal/include/rte_bus.h | 13 +++++++++++++ > lib/eal/linux/eal.c | 1 + > lib/eal/windows/eal.c | 1 + > 9 files changed, 100 insertions(+) > > diff --git a/devtools/libabigail.abignore b/devtools/libabigail.abignore > index 79ff15dc4e..3e519ee42a 100644 > --- a/devtools/libabigail.abignore > +++ b/devtools/libabigail.abignore > @@ -56,3 +56,12 @@ > ; Ignore libabigail false-positive in clang builds, after moving code. > [suppress_function] > name = rte_eal_remote_launch > + > +; Ignore field inserted to rte_bus, adding cleanup function > +[suppress_type] > + name = rte_bus > + has_data_member_inserted_at = end > + > +; Ignore changes to internally used structs containing rte_bus > +[suppress_type] > + name = rte_pci_bus, rte_vmbus_bus, rte_vdev_bus > diff --git a/drivers/bus/pci/pci_common.c b/drivers/bus/pci/pci_common.c > index 37ab879779..8b132ce5fc 100644 > --- a/drivers/bus/pci/pci_common.c > +++ b/drivers/bus/pci/pci_common.c > @@ -394,6 +394,29 @@ pci_probe(void) > return (probed && probed == failed) ? -1 : 0; > } > > +static int > +pci_cleanup(void) > +{ > + struct rte_pci_device *dev = NULL; > + int error = 0; > + > + FOREACH_DEVICE_ON_PCIBUS(dev) { > + struct rte_pci_driver *drv = dev->driver; > + int ret = 0; > + > + if (drv == NULL || drv->remove == NULL) > + continue; > + > + ret = drv->remove(dev); All devices, such as, compressdev, ethdev and dmadev, on the bus are released here. However, the rte_pci_device or rte_vdev_device on the bus allocated during EAL init are not yet released. Why not free these devices here? > + if (ret < 0) { > + rte_errno = errno; > + error = -1; > + } > + } > + > + return error; > +} > + > /* dump one device */ > static int > pci_dump_one_device(FILE *f, struct rte_pci_device *dev) > @@ -813,6 +836,7 @@ struct rte_pci_bus rte_pci_bus = { > .bus = { > .scan = rte_pci_scan, > .probe = pci_probe, > + .cleanup = pci_cleanup, > .find_device = pci_find_device, > .plug = pci_plug, > .unplug = pci_unplug, > diff --git a/drivers/bus/vdev/vdev.c b/drivers/bus/vdev/vdev.c > index a8d8b2327e..3c54f53e19 100644 > --- a/drivers/bus/vdev/vdev.c > +++ b/drivers/bus/vdev/vdev.c > @@ -569,6 +569,29 @@ vdev_probe(void) > return ret; > } > > +static int > +vdev_cleanup(void) > +{ > + struct rte_vdev_device *dev; > + int error = 0; > + > + TAILQ_FOREACH(dev, &vdev_device_list, next) { > + const struct rte_vdev_driver *drv; > + int ret = 0; > + > + drv = container_of(dev->device.driver, const struct rte_vdev_driver, driver); > + > + if (drv == NULL || drv->remove == NULL) > + continue; > + > + ret = drv->remove(dev); > + if (ret < 0) > + error = -1; > + } > + > + return error; > +} > + > struct rte_device * > rte_vdev_find_device(const struct rte_device *start, rte_dev_cmp_t cmp, > const void *data) > @@ -627,6 +650,7 @@ vdev_get_iommu_class(void) > static struct rte_bus rte_vdev_bus = { > .scan = vdev_scan, > .probe = vdev_probe, > + .cleanup = vdev_cleanup, > .find_device = rte_vdev_find_device, > .plug = vdev_plug, > .unplug = vdev_unplug, > diff --git a/lib/eal/common/eal_common_bus.c b/lib/eal/common/eal_common_bus.c > index baa5b532af..3fe67af0ba 100644 > --- a/lib/eal/common/eal_common_bus.c > +++ b/lib/eal/common/eal_common_bus.c > @@ -85,6 +85,23 @@ rte_bus_probe(void) > return 0; > } > > +/* Clean up all devices of all buses */ > +int > +eal_bus_cleanup(void) > +{ > + int ret = 0; > + struct rte_bus *bus; > + > + TAILQ_FOREACH(bus, &rte_bus_list, next) { > + if (bus->cleanup == NULL) > + continue; > + if (bus->cleanup() != 0) > + ret = -1; > + } > + > + return ret; > +} > + > /* Dump information of a single bus */ > static int > bus_dump_one(FILE *f, struct rte_bus *bus) > diff --git a/lib/eal/common/eal_private.h b/lib/eal/common/eal_private.h > index 44d14241f0..eea4749af4 100644 > --- a/lib/eal/common/eal_private.h > +++ b/lib/eal/common/eal_private.h > @@ -441,6 +441,16 @@ int rte_eal_memory_detach(void); > */ > struct rte_bus *rte_bus_find_by_device_name(const char *str); > > +/** > + * For each device on the buses, call the driver-specific function for > + * device cleanup. > + * > + * @return > + * 0 for successful cleanup > + * !0 otherwise > + */ > +int eal_bus_cleanup(void); > + > /** > * Create the unix channel for primary/secondary communication. > * > diff --git a/lib/eal/freebsd/eal.c b/lib/eal/freebsd/eal.c > index a6b20960f2..97ed2c4678 100644 > --- a/lib/eal/freebsd/eal.c > +++ b/lib/eal/freebsd/eal.c > @@ -893,6 +893,7 @@ rte_eal_cleanup(void) > eal_get_internal_configuration(); > rte_service_finalize(); > rte_mp_channel_cleanup(); > + eal_bus_cleanup(); > /* after this point, any DPDK pointers will become dangling */ > rte_eal_memory_detach(); > rte_eal_alarm_cleanup(); > diff --git a/lib/eal/include/rte_bus.h b/lib/eal/include/rte_bus.h > index bbbb6efd28..9908a013f6 100644 > --- a/lib/eal/include/rte_bus.h > +++ b/lib/eal/include/rte_bus.h > @@ -66,6 +66,18 @@ typedef int (*rte_bus_scan_t)(void); > */ > typedef int (*rte_bus_probe_t)(void); > > +/** > + * Implementation specific cleanup function which is responsible for cleaning up > + * devices on that bus with applicable drivers. > + * > + * This is called while iterating over each registered bus. > + * > + * @return > + * 0 for successful cleanup > + * !0 for any error during cleanup > + */ > +typedef int (*rte_bus_cleanup_t)(void); > + > /** > * Device iterator to find a device on a bus. > * > @@ -277,6 +289,7 @@ struct rte_bus { > /**< handle hot-unplug failure on the bus */ > rte_bus_sigbus_handler_t sigbus_handler; > /**< handle sigbus error on the bus */ > + rte_bus_cleanup_t cleanup; /**< Cleanup devices on bus */ > > }; > > diff --git a/lib/eal/linux/eal.c b/lib/eal/linux/eal.c > index 1ef263434a..9b32265ef5 100644 > --- a/lib/eal/linux/eal.c > +++ b/lib/eal/linux/eal.c > @@ -1266,6 +1266,7 @@ rte_eal_cleanup(void) > vfio_mp_sync_cleanup(); > #endif > rte_mp_channel_cleanup(); > + eal_bus_cleanup(); > /* after this point, any DPDK pointers will become dangling */ > rte_eal_memory_detach(); > eal_mp_dev_hotplug_cleanup(); > diff --git a/lib/eal/windows/eal.c b/lib/eal/windows/eal.c > index 122de2a319..fedd6c971a 100644 > --- a/lib/eal/windows/eal.c > +++ b/lib/eal/windows/eal.c > @@ -262,6 +262,7 @@ rte_eal_cleanup(void) > > eal_intr_thread_cancel(); > eal_mem_virt2iova_cleanup(); > + eal_bus_cleanup(); > /* after this point, any DPDK pointers will become dangling */ > rte_eal_memory_detach(); > eal_cleanup_config(internal_conf);