From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
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 <dev@dpdk.org>; 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: <a577db9c-5777-876f-e4df-f6f27e171bb6@huawei.com>
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 <kevin.laatz@intel.com>, <dev@dpdk.org>
CC: =?UTF-8?Q?Morten_Br=c3=b8rup?= <mb@smartsharesystems.com>
References: <20220419161438.1837860-1-kevin.laatz@intel.com>
<20220601170234.11100-1-kevin.laatz@intel.com>
From: "lihuisong (C)" <lihuisong@huawei.com>
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 <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=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 <kevin.laatz@intel.com>
> Acked-by: Morten Brørup <mb@smartsharesystems.com>
>
> ---
> 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);