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

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

On 24/05/2022 10:38, Bruce Richardson wrote:
> On Tue, May 24, 2022 at 10:25:01AM +0100, Kevin Laatz wrote:
>> 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>
>>
> Thanks for the non-RFC versions. Some comments inline.
>
> /Bruce
>
>> ---
>> v2:
>> * change log level from INFO to DEBUG for PCI cleanup
>> * add abignore entries for rte_bus related false positives
>>
>> v3:
>> * add vdev bus cleanup
> Missing v4 update note.
> Please reverse the order here, so it goes from newest to oldest, so those
> of us tracking the patch can just look at top entry.
Ack
>> ---
>>   devtools/libabigail.abignore    |  9 +++++++++
>>   drivers/bus/pci/pci_common.c    | 32 ++++++++++++++++++++++++++++++++
>>   drivers/bus/vdev/vdev.c         | 23 +++++++++++++++++++++++
>>   lib/eal/common/eal_common_bus.c | 18 ++++++++++++++++++
>>   lib/eal/include/rte_bus.h       | 23 +++++++++++++++++++++++
>>   lib/eal/linux/eal.c             |  1 +
>>   6 files changed, 106 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..7d0c49f073 100644
>> --- a/drivers/bus/pci/pci_common.c
>> +++ b/drivers/bus/pci/pci_common.c
>> @@ -394,6 +394,37 @@ pci_probe(void)
>>   	return (probed && probed == failed) ? -1 : 0;
>>   }
>>   
>> +static int
>> +pci_cleanup(void)
>> +{
>> +	struct rte_pci_device *dev = NULL;
>> +	int ret = 0;
>> +
>> +	FOREACH_DEVICE_ON_PCIBUS(dev) {
>> +		struct rte_pci_addr *loc = &dev->addr;
>> +		struct rte_pci_driver *drv = dev->driver;
>> +
>> +		if (loc == NULL || drv == NULL)
>> +			continue;
>> +
>> +		RTE_LOG(DEBUG, 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);
>> +
>> +		ret = drv->remove(dev);
>> +		if (ret < 0) {
>> +			RTE_LOG(ERR, EAL, "Cleanup for device "PCI_PRI_FMT" failed\n",
>> +					dev->addr.domain, dev->addr.bus, dev->addr.devid,
>> +					dev->addr.function);
>> +			rte_errno = errno;
>> +		}
>> +	}
>> +
>> +	return ret;
>> +}
> This function returns the status of the last remove call only, which is
> probably not what we want. Better to make "ret" a local variable inside the
> loop and have a new variable in function scope called "error", initialized
> to zero. Then where you assign rte_errno you can also assign error to -1
> and return error from the function at end.

Will fix for v5.


>> +
>>   /* dump one device */
>>   static int
>>   pci_dump_one_device(FILE *f, struct rte_pci_device *dev)
>> @@ -813,6 +844,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..eca1a3d536 100644
>> --- a/drivers/bus/vdev/vdev.c
>> +++ b/drivers/bus/vdev/vdev.c
>> @@ -569,6 +569,28 @@ vdev_probe(void)
>>   	return ret;
>>   }
>>   
>> +static int
>> +vdev_cleanup(void)
>> +{
>> +	struct rte_vdev_device *dev;
>> +	int ret = 0;
>> +
>> +	TAILQ_FOREACH(dev, &vdev_device_list, next) {
>> +		const char *name;
>> +		struct rte_vdev_driver *drv;
>> +
>> +		name = rte_vdev_device_name(dev);
>> +		if (vdev_parse(name, &drv))
>> +			continue;
>> +
>> +		ret = drv->remove(dev);
>> +		if (ret < 0)
>> +			VDEV_LOG(ERR, "Cleanup for device %s failed\n", rte_vdev_device_name(dev));
>> +	}
>> +
>> +	return ret;
>> +}
> Same comment for ret as above.
>
>> +
>>   struct rte_device *
>>   rte_vdev_find_device(const struct rte_device *start, rte_dev_cmp_t cmp,
>>   		     const void *data)
>> @@ -627,6 +649,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..046a06a2bf 100644
>> --- a/lib/eal/common/eal_common_bus.c
>> +++ b/lib/eal/common/eal_common_bus.c
>> @@ -85,6 +85,24 @@ rte_bus_probe(void)
>>   	return 0;
>>   }
>>   
>> +/* Clean up all devices of all buses */
>> +int
>> +rte_bus_cleanup(void)
>> +{
>> +	int ret;
>> +	struct rte_bus *bus;
>> +
>> +	TAILQ_FOREACH(bus, &rte_bus_list, next) {
>> +		if (bus->cleanup == NULL)
>> +			continue;
>> +		ret = bus->cleanup();
>> +		if (ret)
>> +			RTE_LOG(ERR, EAL, "Bus (%s) cleanup failed.\n", bus->name);
> Is this error message needed, if the individual buses all print out their
> own failure logs? Probably harmless enough.
>
>> +	}
>> +
>> +	return 0;
> Do you want to pass back up the fact that a bus cleanup failed rather than
> always returning 0?

Will change this and review the need for the logging.


>
>> +}
>> +
>>   /* Dump information of a single bus */
>>   static int
>>   bus_dump_one(FILE *f, struct rte_bus *bus)
>> diff --git a/lib/eal/include/rte_bus.h b/lib/eal/include/rte_bus.h
>> index bbbb6efd28..42da38730f 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 */
>>   
>>   };
>>   
>> @@ -317,6 +330,16 @@ int rte_bus_scan(void);
>>    */
>>   int rte_bus_probe(void);
>>   
>> +/**
>> + * For each device on the buses, perform a driver 'match' and call the
>> + * driver-specific function for device cleanup.
>> + *
>> + * @return
>> + * 0 for successful match/cleanup
>> + * !0 otherwise
>> + */
>> +int rte_bus_cleanup(void);
>> +
> This is in a public header file, so it's visible to users, but it's not in
> the version.map file, so not actually usable by anything other than EAL. We
> need to decide whether to make this function explicitly public (in which
> case it probably needs to be marked as experimental and added to
> version.map), or to decide its for EAL use only, in which case move the
> defintion to a private header file e.g. eal_private.h.

The intention is to only call this in eal_cleanup() since all 
applications should call it when closing, so I think its reasonable to 
move it to a private header.

Will move in v5. Thanks for reviewing.


>
>>   /**
>>    * Dump information of all the buses registered with EAL.
>>    *
>> diff --git a/lib/eal/linux/eal.c b/lib/eal/linux/eal.c
>> index 1ef263434a..27014fdc27 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();
>> +	rte_bus_cleanup();
>>   	/* after this point, any DPDK pointers will become dangling */
>>   	rte_eal_memory_detach();
>>   	eal_mp_dev_hotplug_cleanup();
>> -- 
>> 2.31.1
>>