Re: [PATCH v6] eal: add bus cleanup to eal cleanup

[Kevin Laatz <kevin.laatz@intel.com>]

On 02/06/2022 03:06, lihuisong (C) wrote:
> 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?

v7 sent with this change added, thanks.

/Kevin