[RFC] eal: add bus cleanup to eal cleanup

[RFC] eal: add bus cleanup to eal cleanup

[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 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.

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.

Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
---
 drivers/bus/pci/pci_common.c    | 29 +++++++++++++++++++++++++++++
 lib/eal/common/eal_common_bus.c | 18 ++++++++++++++++++
 lib/eal/include/rte_bus.h       | 23 +++++++++++++++++++++++
 lib/eal/linux/eal.c             |  1 +
 4 files changed, 71 insertions(+)

diff --git a/drivers/bus/pci/pci_common.c b/drivers/bus/pci/pci_common.c
index 37ab879779..ee6cce8fc6 100644
--- a/drivers/bus/pci/pci_common.c
+++ b/drivers/bus/pci/pci_common.c
@@ -394,6 +394,34 @@ 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;
+
+		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);
+
+		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;
+}
+
 /* dump one device */
 static int
 pci_dump_one_device(FILE *f, struct rte_pci_device *dev)
@@ -813,6 +841,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/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);
+	}
+
+	return 0;
+}
+
 /* 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..7dbc398408 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.
  *
@@ -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 */
@@ -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);
+
 /**
  * Dump information of all the buses registered with EAL.
  *
diff --git a/lib/eal/linux/eal.c b/lib/eal/linux/eal.c
index 025e5cc10d..37983b98c0 100644
--- a/lib/eal/linux/eal.c
+++ b/lib/eal/linux/eal.c
@@ -1268,6 +1268,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

Re: [RFC] eal: add bus cleanup to eal cleanup

[Stephen Hemminger]

On Tue, 19 Apr 2022 17:14:38 +0100
Kevin Laatz <kevin.laatz@intel.com> wrote:

> +		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);
> +

Looks like a debug message, do we really need more log spam?

RE: [RFC] eal: add bus cleanup to eal cleanup

[Morten Brørup]

> From: Kevin Laatz [mailto:kevin.laatz@intel.com]
> Sent: Tuesday, 19 April 2022 18.15
> 
> 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 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.
> 
> 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.
> 
> Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
> ---

[...]

> +		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ørup <mb@smartsharesystems.com>

Re: [RFC] eal: add bus cleanup to eal cleanup

[Kevin Laatz]

On 20/04/2022 07:55, Morten Brørup wrote:
>> From: Kevin Laatz [mailto:kevin.laatz@intel.com]
>> Sent: Tuesday, 19 April 2022 18.15
>>
>> 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 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.
>>
>> 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.
>>
>> Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
>> ---
> [...]
>
>> +		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.

Thanks for the feedback.

+1, will change to debug for v2.


>
> [...]
>
>> @@ -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?

I've looked into this and have run test-meson-builds with ABI checks 
enabled.

The output of those checks flagged some potential breaks, however I 
believe these are false positives. The output indicated 2 potential 
breaks (in multiple places, but the root is the same)

1. Member has been added to the rte_bus struct. This is flagged as a 
sub-type change, however since rte_bus is only ever reference by 
pointer, it is not a break.

2. Offset of members changes in 'rte_pci_bus' and 'rte_vmbus_bus' 
structs. These structs are only used internally so also do no break ABI.


Since the ABI checks do flag the addition, I will add an entry to the 
abignore for the v2.


>
>
> 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ørup <mb@smartsharesystems.com>
>

RE: [RFC] eal: add bus cleanup to eal cleanup

[Morten Brørup]

> From: Kevin Laatz [mailto:kevin.laatz@intel.com]
> Sent: Friday, 22 April 2022 11.18
> 
> On 20/04/2022 07:55, Morten Brørup wrote:
> >> From: Kevin Laatz [mailto:kevin.laatz@intel.com]
> >> Sent: Tuesday, 19 April 2022 18.15
> >>
> >> 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 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.
> >>
> >> 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.
> >>
> >> Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
> >> ---
> > [...]
> >
> >> +		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.
> 
> Thanks for the feedback.
> 
> +1, will change to debug for v2.
> 
> 
> >
> > [...]
> >
> >> @@ -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?
> 
> I've looked into this and have run test-meson-builds with ABI checks
> enabled.
> 
> The output of those checks flagged some potential breaks, however I
> believe these are false positives. The output indicated 2 potential
> breaks (in multiple places, but the root is the same)
> 
> 1. Member has been added to the rte_bus struct. This is flagged as a
> sub-type change, however since rte_bus is only ever reference by
> pointer, it is not a break.
> 
> 2. Offset of members changes in 'rte_pci_bus' and 'rte_vmbus_bus'
> structs. These structs are only used internally so also do no break
> ABI.
> 

Sounds good! Then there should be no more worries. :-)

> 
> Since the ABI checks do flag the addition, I will add an entry to the
> abignore for the v2.
> 
> 
> >
> >
> > 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ørup <mb@smartsharesystems.com>
> >

[RFC v2] eal: add bus cleanup to eal cleanup

[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 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.

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.

Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>

---
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    | 29 +++++++++++++++++++++++++++++
 lib/eal/common/eal_common_bus.c | 18 ++++++++++++++++++
 lib/eal/include/rte_bus.h       | 23 +++++++++++++++++++++++
 lib/eal/linux/eal.c             |  1 +
 5 files changed, 80 insertions(+)

diff --git a/devtools/libabigail.abignore b/devtools/libabigail.abignore
index c618f20032..3ff5d4db7c 100644
--- a/devtools/libabigail.abignore
+++ b/devtools/libabigail.abignore
@@ -40,3 +40,12 @@
 ; Ignore visibility fix of local functions in experimental gpudev library
 [suppress_file]
         soname_regexp = ^librte_gpudev\.
+
+; 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
diff --git a/drivers/bus/pci/pci_common.c b/drivers/bus/pci/pci_common.c
index 37ab879779..1bee8e8201 100644
--- a/drivers/bus/pci/pci_common.c
+++ b/drivers/bus/pci/pci_common.c
@@ -394,6 +394,34 @@ 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;
+
+		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;
+}
+
 /* dump one device */
 static int
 pci_dump_one_device(FILE *f, struct rte_pci_device *dev)
@@ -813,6 +841,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/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);
+	}
+
+	return 0;
+}
+
 /* 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);
+
 /**
  * Dump information of all the buses registered with EAL.
  *
diff --git a/lib/eal/linux/eal.c b/lib/eal/linux/eal.c
index 025e5cc10d..37983b98c0 100644
--- a/lib/eal/linux/eal.c
+++ b/lib/eal/linux/eal.c
@@ -1268,6 +1268,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

[PATCH v3] eal: add bus cleanup to eal cleanup

[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>

---
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
---
 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 c618f20032..2ff482c086 100644
--- a/devtools/libabigail.abignore
+++ b/devtools/libabigail.abignore
@@ -40,3 +40,12 @@
 ; Ignore visibility fix of local functions in experimental gpudev library
 [suppress_file]
         soname_regexp = ^librte_gpudev\.
+
+; 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;
+}
+
 /* 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;
+}
+
 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);
+	}
+
+	return 0;
+}
+
 /* 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);
+
 /**
  * Dump information of all the buses registered with EAL.
  *
diff --git a/lib/eal/linux/eal.c b/lib/eal/linux/eal.c
index 025e5cc10d..37983b98c0 100644
--- a/lib/eal/linux/eal.c
+++ b/lib/eal/linux/eal.c
@@ -1268,6 +1268,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

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

[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>

---
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
---
 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;
+}
+
 /* 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;
+}
+
 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);
+	}
+
+	return 0;
+}
+
 /* 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);
+
 /**
  * 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

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

[Bruce Richardson]

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. 

> ---
>  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.

> +
>  /* 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?

> +}
> +
>  /* 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.

>  /**
>   * 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
> 

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

[Kevin Laatz]

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
>>

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

[Stephen Hemminger]

On Tue, 24 May 2022 10:25:01 +0100
Kevin Laatz <kevin.laatz@intel.com> wrote:

> +
> +		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);
> +

No message is needed here.>

> +			RTE_LOG(ERR, EAL, "Cleanup for device "PCI_PRI_FMT" failed\n",
> +					dev->addr.domain, dev->addr.bus, dev->addr.devid,
> +					dev->addr.function);

As Bruce said, the driver probably already logged something.
So no message, or make it a DEBUG message.

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

[Kevin Laatz]

On 24/05/2022 15:48, Stephen Hemminger wrote:
> On Tue, 24 May 2022 10:25:01 +0100
> Kevin Laatz <kevin.laatz@intel.com> wrote:
>
>> +
>> +		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);
>> +
> No message is needed here.>
>
>> +			RTE_LOG(ERR, EAL, "Cleanup for device "PCI_PRI_FMT" failed\n",
>> +					dev->addr.domain, dev->addr.bus, dev->addr.devid,
>> +					dev->addr.function);
> As Bruce said, the driver probably already logged something.
> So no message, or make it a DEBUG message.
Ack, will remove unnecessary logging in v5.

[PATCH v5] eal: add bus cleanup to eal cleanup

[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>

---
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/include/rte_bus.h       | 13 +++++++++++++
 lib/eal/linux/eal.c             |  1 +
 7 files changed, 98 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..5dd486019f 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)
+			continue;
+
+		ret = drv->remove(dev);
+		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..78d6e75b89 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 char *name;
+		struct rte_vdev_driver *drv;
+		int ret = 0;
+
+		name = rte_vdev_device_name(dev);
+		if (vdev_parse(name, &drv))
+			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..be62776a2d 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
+rte_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())
+			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..df190702a3 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, 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);
+
 /**
  * Create the unix channel for primary/secondary communication.
  *
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..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

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

[Bruce Richardson]

On Wed, May 25, 2022 at 11:39:53AM +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>
> 
> ---
> v5:
> * remove unnecessary logs
> * move rte_bus_cleanup() definition to eal_private.h
> * fix return values for vdev_cleanup and pci_cleanup
> 

Hi Kevin,

few more comments inline below.

Also, with this change, does it mean that we can remove the cleanup done at
the end of testpmd and some other apps? If so, perhaps that should be done
in a separate patch to make this a multi-patch series.

/Bruce

> 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/include/rte_bus.h       | 13 +++++++++++++
>  lib/eal/linux/eal.c             |  1 +
>  7 files changed, 98 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..5dd486019f 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)
> +			continue;
> +
> +		ret = drv->remove(dev);
> +		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..78d6e75b89 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 char *name;
> +		struct rte_vdev_driver *drv;
> +		int ret = 0;
> +
> +		name = rte_vdev_device_name(dev);
> +		if (vdev_parse(name, &drv))
> +			continue;
> +

The vdev_device struct contains an rte_device struct which contains a
pointer to the driver. Can that not be used rather than calling vdev_parse?

> +		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..be62776a2d 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
> +rte_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())
> +			ret = -1;

Make comparison explicit, I think, i.e. add "!= 0" or "< 0" in if
statement.

> +	}
> +
> +	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..df190702a3 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, 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);
> +

Since this is now a private function, remove the rte_ prefix. Suggest
"eal_bus_cleanup" as a function name.

>  /**
>   * Create the unix channel for primary/secondary communication.
>   *
> 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..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();

Is this change not also applicable for FreeBSD and windows?

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

[Kevin Laatz]

On 25/05/2022 12:12, Bruce Richardson wrote:
> On Wed, May 25, 2022 at 11:39:53AM +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>
>>
>> ---
>> v5:
>> * remove unnecessary logs
>> * move rte_bus_cleanup() definition to eal_private.h
>> * fix return values for vdev_cleanup and pci_cleanup
>>
> Hi Kevin,
>
> few more comments inline below.
>
> Also, with this change, does it mean that we can remove the cleanup done at
> the end of testpmd and some other apps? If so, perhaps that should be done
> in a separate patch to make this a multi-patch series.

With these changes there will be some overlapping cleanup.

I'll look into removing any dupliation, however depending on the app 
there may be other cleanup that still needs to stay in place.


> /Bruce
>
>> 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/include/rte_bus.h       | 13 +++++++++++++
>>   lib/eal/linux/eal.c             |  1 +
>>   7 files changed, 98 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..5dd486019f 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)
>> +			continue;
>> +
>> +		ret = drv->remove(dev);
>> +		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..78d6e75b89 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 char *name;
>> +		struct rte_vdev_driver *drv;
>> +		int ret = 0;
>> +
>> +		name = rte_vdev_device_name(dev);
>> +		if (vdev_parse(name, &drv))
>> +			continue;
>> +
> The vdev_device struct contains an rte_device struct which contains a
> pointer to the driver. Can that not be used rather than calling vdev_parse?

Ack


>
>> +		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..be62776a2d 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
>> +rte_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())
>> +			ret = -1;
> Make comparison explicit, I think, i.e. add "!= 0" or "< 0" in if
> statement.

Ack


>
>> +	}
>> +
>> +	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..df190702a3 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, 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);
>> +
> Since this is now a private function, remove the rte_ prefix. Suggest
> "eal_bus_cleanup" as a function name.

Yes, makes sense


>
>>   /**
>>    * Create the unix channel for primary/secondary communication.
>>    *
>> 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..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();
> Is this change not also applicable for FreeBSD and windows?

Will add this to both, thanks.

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

[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);
+		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);
-- 
2.31.1

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

[Bruce Richardson]

On Wed, Jun 01, 2022 at 06:02:34PM +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>
>
Reviewed-by: Bruce Richardson <bruce.richardson@intel.com> 

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

[lihuisong (C)]

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);

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

[Kevin Laatz]

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

[PATCH v7] eal: add bus cleanup to eal cleanup

[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>
Reviewed-by: Bruce Richardson <bruce.richardson@intel.com>

---
v7:
* free rte_pci_device structs during cleanup
* free rte_vdev_device structs during cleanup

v6:
* fix units in doc API descriptions

v5:
* add doc updates for new APIs

v4:
* fix return value when scaling_freq_max is not set
* fix mismatching comments

v3:
* move setters from arg parse function to init
* consider 0 as 'not set' for scaling_freq_max
* other minor fixes

v2:
* add doc update for l3fwd-power
* order version.map additions alphabetically
---
 devtools/libabigail.abignore    |  9 +++++++++
 drivers/bus/pci/pci_common.c    | 26 ++++++++++++++++++++++++++
 drivers/bus/vdev/vdev.c         | 26 ++++++++++++++++++++++++++
 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, 104 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..75b312eef1 100644
--- a/drivers/bus/pci/pci_common.c
+++ b/drivers/bus/pci/pci_common.c
@@ -25,6 +25,7 @@
 #include <rte_common.h>
 #include <rte_devargs.h>
 #include <rte_vfio.h>
+#include <rte_tailq.h>
 
 #include "private.h"
 
@@ -394,6 +395,30 @@ pci_probe(void)
 	return (probed && probed == failed) ? -1 : 0;
 }
 
+static int
+pci_cleanup(void)
+{
+	struct rte_pci_device *dev, *tmp_dev;
+	int error = 0;
+
+	RTE_TAILQ_FOREACH_SAFE(dev, &rte_pci_bus.device_list, next, tmp_dev) {
+		struct rte_pci_driver *drv = dev->driver;
+		int ret = 0;
+
+		if (drv == NULL || drv->remove == NULL)
+			continue;
+
+		ret = drv->remove(dev);
+		if (ret < 0) {
+			rte_errno = errno;
+			error = -1;
+		}
+		free(dev);
+	}
+
+	return error;
+}
+
 /* dump one device */
 static int
 pci_dump_one_device(FILE *f, struct rte_pci_device *dev)
@@ -813,6 +838,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..707ea1bbb5 100644
--- a/drivers/bus/vdev/vdev.c
+++ b/drivers/bus/vdev/vdev.c
@@ -569,6 +569,31 @@ vdev_probe(void)
 	return ret;
 }
 
+static int
+vdev_cleanup(void)
+{
+	struct rte_vdev_device *dev, *tmp_dev;
+	int error = 0;
+
+	RTE_TAILQ_FOREACH_SAFE(dev, &vdev_device_list, next, tmp_dev) {
+		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;
+
+		free(dev);
+	}
+
+	return error;
+}
+
 struct rte_device *
 rte_vdev_find_device(const struct rte_device *start, rte_dev_cmp_t cmp,
 		     const void *data)
@@ -627,6 +652,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);
-- 
2.31.1

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

[Stephen Hemminger]

On Fri,  3 Jun 2022 15:36:01 +0100
Kevin Laatz <kevin.laatz@intel.com> wrote:

> +/* 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;
> +}
> +

This is an internal  function, and all users of it
look like they don't use the return value.

Why not make the function void eal_bus_cleanup()
and simplify back up the call chain?

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

[Bruce Richardson]

On Fri, Jun 03, 2022 at 08:11:54AM -0700, Stephen Hemminger wrote:
> On Fri,  3 Jun 2022 15:36:01 +0100
> Kevin Laatz <kevin.laatz@intel.com> wrote:
> 
> > +/* 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;
> > +}
> > +
> 
> This is an internal  function, and all users of it
> look like they don't use the return value.
> 
> Why not make the function void eal_bus_cleanup()
> and simplify back up the call chain?

Is there really that much difference in doing so? My own slight preference
would be to have the error codes available for future use in case we want
them, so long as the overhead of them is not great (which it should not
be). However, if others all feel that having these functions return void is
best, I'm happy enough with that too.

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

[lihuisong (C)]

在 2022/6/3 22:36, 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>
> Reviewed-by: Bruce Richardson <bruce.richardson@intel.com>
>
> ---
> v7:
> * free rte_pci_device structs during cleanup
> * free rte_vdev_device structs during cleanup
>
> v6:
> * fix units in doc API descriptions
>
> v5:
> * add doc updates for new APIs
>
> v4:
> * fix return value when scaling_freq_max is not set
> * fix mismatching comments
>
> v3:
> * move setters from arg parse function to init
> * consider 0 as 'not set' for scaling_freq_max
> * other minor fixes
>
> v2:
> * add doc update for l3fwd-power
> * order version.map additions alphabetically
> ---
>   devtools/libabigail.abignore    |  9 +++++++++
>   drivers/bus/pci/pci_common.c    | 26 ++++++++++++++++++++++++++
>   drivers/bus/vdev/vdev.c         | 26 ++++++++++++++++++++++++++
>   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, 104 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..75b312eef1 100644
> --- a/drivers/bus/pci/pci_common.c
> +++ b/drivers/bus/pci/pci_common.c
> @@ -25,6 +25,7 @@
>   #include <rte_common.h>
>   #include <rte_devargs.h>
>   #include <rte_vfio.h>
> +#include <rte_tailq.h>
>   
>   #include "private.h"
>   
> @@ -394,6 +395,30 @@ pci_probe(void)
>   	return (probed && probed == failed) ? -1 : 0;
>   }
>   
> +static int
> +pci_cleanup(void)
> +{
> +	struct rte_pci_device *dev, *tmp_dev;
> +	int error = 0;
> +
> +	RTE_TAILQ_FOREACH_SAFE(dev, &rte_pci_bus.device_list, next, tmp_dev) {
> +		struct rte_pci_driver *drv = dev->driver;
> +		int ret = 0;
> +
> +		if (drv == NULL || drv->remove == NULL)
> +			continue;
It seems that 'dev->driver' still points to the 'rte_pci_driver' or 
'rte_vdev_driver'
if the device has been closed by 'dev_close()'. Logically, there is a 
risk of removing
a device twice. Do you want to guarantee through the 'remove()' API itself?
> +
> +		ret = drv->remove(dev);
> +		if (ret < 0) {
> +			rte_errno = errno;
> +			error = -1;
> +		}
> +		free(dev);
Can I use the 'local_dev_remove()' to remove the device and 
rte_pci/vdev_device on the bus?
Because there may be other resources that need to be released, like, 
some release operations
in the 'rte_pci_detach_dev()'.
> +	}
> +
> +	return error;
> +}
> +
>   /* dump one device */
>   static int
>   pci_dump_one_device(FILE *f, struct rte_pci_device *dev)
> @@ -813,6 +838,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..707ea1bbb5 100644
> --- a/drivers/bus/vdev/vdev.c
> +++ b/drivers/bus/vdev/vdev.c
> @@ -569,6 +569,31 @@ vdev_probe(void)
>   	return ret;
>   }
>   
> +static int
> +vdev_cleanup(void)
> +{
> +	struct rte_vdev_device *dev, *tmp_dev;
> +	int error = 0;
> +
> +	RTE_TAILQ_FOREACH_SAFE(dev, &vdev_device_list, next, tmp_dev) {
> +		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;
> +
> +		free(dev);
> +	}
> +
> +	return error;
> +}
> +
>   struct rte_device *
>   rte_vdev_find_device(const struct rte_device *start, rte_dev_cmp_t cmp,
>   		     const void *data)
> @@ -627,6 +652,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);

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

[Thomas Monjalon]

03/06/2022 16:36, 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.
[...]
> --- 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

I'm not sure we can safely consider these structs as internal.
The right process is to send a deprecation notice,
and then remove them from the public API.

For info, Li has sent a patch for the bus cleanup
which is not updating the bus code:
https://patches.dpdk.org/project/dpdk/patch/20220606114650.209612-3-lizh@nvidia.com/
It may be a temporary solution before the deprecation.

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

[David Marchand]

On Tue, Jun 7, 2022 at 1:09 PM Thomas Monjalon <thomas@monjalon.net> wrote:
>
> 03/06/2022 16:36, 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.
> [...]
> > --- 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

(This change is strange as there is no rte_vdev_bus type, but I won't
investigate the relevance of this rule for now).

>
> I'm not sure we can safely consider these structs as internal.
> The right process is to send a deprecation notice,
> and then remove them from the public API.

Same for me, I don't think we can safely ignore.

A rte_bus struct is embedded in rte_pci_bus (resp. rte_vmbus_bus).
If we make it grow, any inlined access (like walk in device_list or
driver_list) after the rte_bus object is broken for code accessing it
out of DPDK.
Such code might exist out there, since we expose
FOREACH_DEVICE_ON_PCIBUS, for example.


>
> For info, Li has sent a patch for the bus cleanup
> which is not updating the bus code:
> https://patches.dpdk.org/project/dpdk/patch/20220606114650.209612-3-lizh@nvidia.com/
> It may be a temporary solution before the deprecation.

On the principle, that's probably the best, there is no question about
unclear frontier of the ABI.
(In practice though, the mentionned patch is triggering segfaults in
two CI, for pdump).

Hiding rte_bus object should be straightforward in v22.11, I had some
patches, but never finished the work.

It would be great too, to look into rte_driver and rte_device which
are exposed important types, but that's another story.


-- 
David Marchand

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

[Bruce Richardson]

On Tue, Jun 07, 2022 at 05:12:02PM +0200, David Marchand wrote:
> On Tue, Jun 7, 2022 at 1:09 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> >
> > 03/06/2022 16:36, 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.
> > [...]
> > > --- 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
> 
> (This change is strange as there is no rte_vdev_bus type, but I won't
> investigate the relevance of this rule for now).
> 
> >
> > I'm not sure we can safely consider these structs as internal.
> > The right process is to send a deprecation notice,
> > and then remove them from the public API.
> 
> Same for me, I don't think we can safely ignore.
> 
> A rte_bus struct is embedded in rte_pci_bus (resp. rte_vmbus_bus).
> If we make it grow, any inlined access (like walk in device_list or
> driver_list) after the rte_bus object is broken for code accessing it
> out of DPDK.
> Such code might exist out there, since we expose
> FOREACH_DEVICE_ON_PCIBUS, for example.
> 
> 
> >
> > For info, Li has sent a patch for the bus cleanup
> > which is not updating the bus code:
> > https://patches.dpdk.org/project/dpdk/patch/20220606114650.209612-3-lizh@nvidia.com/
> > It may be a temporary solution before the deprecation.
> 
> On the principle, that's probably the best, there is no question about
> unclear frontier of the ABI.
> (In practice though, the mentionned patch is triggering segfaults in
> two CI, for pdump).
> 
> Hiding rte_bus object should be straightforward in v22.11, I had some
> patches, but never finished the work.
> 
> It would be great too, to look into rte_driver and rte_device which
> are exposed important types, but that's another story.
> 
Agreed, we need to look into all this for 22.11 release, let's defer this
patch until we get proper deprecation process. Temporary patch looks fine
as a fix too.

/Bruce

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

[David Marchand]

Hello Bruce, Kevin,

On Mon, Jun 13, 2022 at 5:59 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
> > > For info, Li has sent a patch for the bus cleanup
> > > which is not updating the bus code:
> > > https://patches.dpdk.org/project/dpdk/patch/20220606114650.209612-3-lizh@nvidia.com/
> > > It may be a temporary solution before the deprecation.
> >
> > On the principle, that's probably the best, there is no question about
> > unclear frontier of the ABI.
> > (In practice though, the mentionned patch is triggering segfaults in
> > two CI, for pdump).
> >
> > Hiding rte_bus object should be straightforward in v22.11, I had some
> > patches, but never finished the work.
> >
> > It would be great too, to look into rte_driver and rte_device which
> > are exposed important types, but that's another story.
> >
> Agreed, we need to look into all this for 22.11 release, let's defer this
> patch until we get proper deprecation process. Temporary patch looks fine
> as a fix too.

The patch needs some rebasing for making it into 22.11.
Can you work on it, this week?


Thanks!

-- 
David Marchand

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

[Kevin Laatz]

Hi David,

On 03/10/2022 13:35, David Marchand wrote:
> Hello Bruce, Kevin,
>
> On Mon, Jun 13, 2022 at 5:59 PM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
>>>> For info, Li has sent a patch for the bus cleanup
>>>> which is not updating the bus code:
>>>> https://patches.dpdk.org/project/dpdk/patch/20220606114650.209612-3-lizh@nvidia.com/
>>>> It may be a temporary solution before the deprecation.
>>> On the principle, that's probably the best, there is no question about
>>> unclear frontier of the ABI.
>>> (In practice though, the mentionned patch is triggering segfaults in
>>> two CI, for pdump).
>>>
>>> Hiding rte_bus object should be straightforward in v22.11, I had some
>>> patches, but never finished the work.
>>>
>>> It would be great too, to look into rte_driver and rte_device which
>>> are exposed important types, but that's another story.
>>>
>> Agreed, we need to look into all this for 22.11 release, let's defer this
>> patch until we get proper deprecation process. Temporary patch looks fine
>> as a fix too.
> The patch needs some rebasing for making it into 22.11.
> Can you work on it, this week?
>
Yes, I'll have a look at it - thanks for your work on the deprecations 
and cleanup!

-Kevin

[PATCH v8] eal: add bus cleanup to eal cleanup

[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>
Reviewed-by: Bruce Richardson <bruce.richardson@intel.com>

---
v8:
* rebase

v7:
* free rte_pci_device structs during cleanup
* free rte_vdev_device structs during cleanup

v6:
* fix units in doc API descriptions

v5:
* add doc updates for new APIs

v4:
* fix return value when scaling_freq_max is not set
* fix mismatching comments

v3:
* move setters from arg parse function to init
* consider 0 as 'not set' for scaling_freq_max
* other minor fixes

v2:
* add doc update for l3fwd-power
* order version.map additions alphabetically
---
 drivers/bus/pci/pci_common.c    | 28 ++++++++++++++++++++++++++++
 drivers/bus/vdev/vdev.c         | 27 +++++++++++++++++++++++++++
 lib/eal/common/eal_common_bus.c | 17 +++++++++++++++++
 lib/eal/common/eal_private.h    | 10 ++++++++++
 lib/eal/freebsd/eal.c           |  1 +
 lib/eal/include/bus_driver.h    | 13 +++++++++++++
 lib/eal/linux/eal.c             |  1 +
 lib/eal/windows/eal.c           |  1 +
 8 files changed, 98 insertions(+)

diff --git a/drivers/bus/pci/pci_common.c b/drivers/bus/pci/pci_common.c
index 5ea72bcf23..fb754e0e0a 100644
--- a/drivers/bus/pci/pci_common.c
+++ b/drivers/bus/pci/pci_common.c
@@ -25,6 +25,7 @@
 #include <rte_common.h>
 #include <rte_devargs.h>
 #include <rte_vfio.h>
+#include <rte_tailq.h>
 
 #include "private.h"
 
@@ -439,6 +440,32 @@ pci_probe(void)
 	return (probed && probed == failed) ? -1 : 0;
 }
 
+static int
+pci_cleanup(void)
+{
+	struct rte_pci_device *dev, *tmp_dev;
+	int error = 0;
+
+	RTE_TAILQ_FOREACH_SAFE(dev, &rte_pci_bus.device_list, next, tmp_dev) {
+		struct rte_pci_driver *drv = dev->driver;
+		int ret = 0;
+
+		if (drv == NULL || drv->remove == NULL)
+			continue;
+
+		ret = drv->remove(dev);
+		if (ret < 0) {
+			rte_errno = errno;
+			error = -1;
+		}
+		dev->driver = NULL;
+		dev->device.driver = NULL;
+		free(dev);
+	}
+
+	return error;
+}
+
 /* dump one device */
 static int
 pci_dump_one_device(FILE *f, struct rte_pci_device *dev)
@@ -856,6 +883,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 b176b658fc..f5b43f1930 100644
--- a/drivers/bus/vdev/vdev.c
+++ b/drivers/bus/vdev/vdev.c
@@ -567,6 +567,32 @@ vdev_probe(void)
 	return ret;
 }
 
+static int
+vdev_cleanup(void)
+{
+	struct rte_vdev_device *dev, *tmp_dev;
+	int error = 0;
+
+	RTE_TAILQ_FOREACH_SAFE(dev, &vdev_device_list, next, tmp_dev) {
+		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;
+
+		dev->device.driver = NULL;
+		free(dev);
+	}
+
+	return error;
+}
+
 struct rte_device *
 rte_vdev_find_device(const struct rte_device *start, rte_dev_cmp_t cmp,
 		     const void *data)
@@ -625,6 +651,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 be64d31b0f..deb9fb8a12 100644
--- a/lib/eal/common/eal_common_bus.c
+++ b/lib/eal/common/eal_common_bus.c
@@ -91,6 +91,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 3ca9ce2ffc..0f4d75bb89 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 a1bb5363b1..b9a7792c19 100644
--- a/lib/eal/freebsd/eal.c
+++ b/lib/eal/freebsd/eal.c
@@ -896,6 +896,7 @@ rte_eal_cleanup(void)
 	rte_mp_channel_cleanup();
 	rte_trace_save();
 	eal_trace_fini();
+	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/bus_driver.h b/lib/eal/include/bus_driver.h
index d2e615a736..7b85a17a09 100644
--- a/lib/eal/include/bus_driver.h
+++ b/lib/eal/include/bus_driver.h
@@ -205,6 +205,18 @@ typedef int (*rte_bus_hot_unplug_handler_t)(struct rte_device *dev);
  */
 typedef int (*rte_bus_sigbus_handler_t)(const void *failure_addr);
 
+/**
+ * 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);
+
 /**
  * Bus scan policies
  */
@@ -256,6 +268,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 9a168b7773..b1d6b5046f 100644
--- a/lib/eal/linux/eal.c
+++ b/lib/eal/linux/eal.c
@@ -1365,6 +1365,7 @@ rte_eal_cleanup(void)
 	rte_mp_channel_cleanup();
 	rte_trace_save();
 	eal_trace_fini();
+	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 79322d2ce9..adb929a014 100644
--- a/lib/eal/windows/eal.c
+++ b/lib/eal/windows/eal.c
@@ -263,6 +263,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);
-- 
2.31.1

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

[David Marchand]

On Tue, Oct 4, 2022 at 3:08 PM Kevin Laatz <kevin.laatz@intel.com> 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>
> Reviewed-by: Bruce Richardson <bruce.richardson@intel.com>
>

Thanks for the rebase.
Most of it lgtm, just one question/comment.

[snip]

> diff --git a/lib/eal/freebsd/eal.c b/lib/eal/freebsd/eal.c
> index a1bb5363b1..b9a7792c19 100644
> --- a/lib/eal/freebsd/eal.c
> +++ b/lib/eal/freebsd/eal.c
> @@ -896,6 +896,7 @@ rte_eal_cleanup(void)
>         rte_mp_channel_cleanup();
>         rte_trace_save();
>         eal_trace_fini();
> +       eal_bus_cleanup();
>         /* after this point, any DPDK pointers will become dangling */
>         rte_eal_memory_detach();
>         rte_eal_alarm_cleanup();

Do you have a reason to put the bus cleanup after the traces are
stored and the trace subsystem is uninitialised?

With the current location for eal_bus_cleanup(), it means that this
function (and any code it calls) is not traceable.
To be fair, I don't think we have any trace points in this code at the
moment, but we might have in the future.


-- 
David Marchand

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

[Kevin Laatz]

On 04/10/2022 16:28, David Marchand wrote:
> On Tue, Oct 4, 2022 at 3:08 PM Kevin Laatz <kevin.laatz@intel.com> 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>
>> Reviewed-by: Bruce Richardson <bruce.richardson@intel.com>
>>
> Thanks for the rebase.
> Most of it lgtm, just one question/comment.
>
> [snip]
>
>> diff --git a/lib/eal/freebsd/eal.c b/lib/eal/freebsd/eal.c
>> index a1bb5363b1..b9a7792c19 100644
>> --- a/lib/eal/freebsd/eal.c
>> +++ b/lib/eal/freebsd/eal.c
>> @@ -896,6 +896,7 @@ rte_eal_cleanup(void)
>>          rte_mp_channel_cleanup();
>>          rte_trace_save();
>>          eal_trace_fini();
>> +       eal_bus_cleanup();
>>          /* after this point, any DPDK pointers will become dangling */
>>          rte_eal_memory_detach();
>>          rte_eal_alarm_cleanup();
> Do you have a reason to put the bus cleanup after the traces are
> stored and the trace subsystem is uninitialised?
>
> With the current location for eal_bus_cleanup(), it means that this
> function (and any code it calls) is not traceable.
> To be fair, I don't think we have any trace points in this code at the
> moment, but we might have in the future.

No reason for doing it after trace un-init. I'll move and resend.

Thanks!

[PATCH v9] eal: add bus cleanup to eal cleanup

[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>
Reviewed-by: Bruce Richardson <bruce.richardson@intel.com>

---
v9:
* move bus cleanup before trace save and uninitialize

v8:
* rebase

v7:
* free rte_pci_device structs during cleanup
* free rte_vdev_device structs during cleanup

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
---
 drivers/bus/pci/pci_common.c    | 28 ++++++++++++++++++++++++++++
 drivers/bus/vdev/vdev.c         | 27 +++++++++++++++++++++++++++
 lib/eal/common/eal_common_bus.c | 17 +++++++++++++++++
 lib/eal/common/eal_private.h    | 10 ++++++++++
 lib/eal/freebsd/eal.c           |  1 +
 lib/eal/include/bus_driver.h    | 13 +++++++++++++
 lib/eal/linux/eal.c             |  1 +
 lib/eal/windows/eal.c           |  1 +
 8 files changed, 98 insertions(+)

diff --git a/drivers/bus/pci/pci_common.c b/drivers/bus/pci/pci_common.c
index 5ea72bcf23..fb754e0e0a 100644
--- a/drivers/bus/pci/pci_common.c
+++ b/drivers/bus/pci/pci_common.c
@@ -25,6 +25,7 @@
 #include <rte_common.h>
 #include <rte_devargs.h>
 #include <rte_vfio.h>
+#include <rte_tailq.h>
 
 #include "private.h"
 
@@ -439,6 +440,32 @@ pci_probe(void)
 	return (probed && probed == failed) ? -1 : 0;
 }
 
+static int
+pci_cleanup(void)
+{
+	struct rte_pci_device *dev, *tmp_dev;
+	int error = 0;
+
+	RTE_TAILQ_FOREACH_SAFE(dev, &rte_pci_bus.device_list, next, tmp_dev) {
+		struct rte_pci_driver *drv = dev->driver;
+		int ret = 0;
+
+		if (drv == NULL || drv->remove == NULL)
+			continue;
+
+		ret = drv->remove(dev);
+		if (ret < 0) {
+			rte_errno = errno;
+			error = -1;
+		}
+		dev->driver = NULL;
+		dev->device.driver = NULL;
+		free(dev);
+	}
+
+	return error;
+}
+
 /* dump one device */
 static int
 pci_dump_one_device(FILE *f, struct rte_pci_device *dev)
@@ -856,6 +883,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 b176b658fc..f5b43f1930 100644
--- a/drivers/bus/vdev/vdev.c
+++ b/drivers/bus/vdev/vdev.c
@@ -567,6 +567,32 @@ vdev_probe(void)
 	return ret;
 }
 
+static int
+vdev_cleanup(void)
+{
+	struct rte_vdev_device *dev, *tmp_dev;
+	int error = 0;
+
+	RTE_TAILQ_FOREACH_SAFE(dev, &vdev_device_list, next, tmp_dev) {
+		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;
+
+		dev->device.driver = NULL;
+		free(dev);
+	}
+
+	return error;
+}
+
 struct rte_device *
 rte_vdev_find_device(const struct rte_device *start, rte_dev_cmp_t cmp,
 		     const void *data)
@@ -625,6 +651,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 be64d31b0f..deb9fb8a12 100644
--- a/lib/eal/common/eal_common_bus.c
+++ b/lib/eal/common/eal_common_bus.c
@@ -91,6 +91,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 3ca9ce2ffc..0f4d75bb89 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 a1bb5363b1..1b58cd3da6 100644
--- a/lib/eal/freebsd/eal.c
+++ b/lib/eal/freebsd/eal.c
@@ -894,6 +894,7 @@ rte_eal_cleanup(void)
 		eal_get_internal_configuration();
 	rte_service_finalize();
 	rte_mp_channel_cleanup();
+	eal_bus_cleanup();
 	rte_trace_save();
 	eal_trace_fini();
 	/* after this point, any DPDK pointers will become dangling */
diff --git a/lib/eal/include/bus_driver.h b/lib/eal/include/bus_driver.h
index d2e615a736..7b85a17a09 100644
--- a/lib/eal/include/bus_driver.h
+++ b/lib/eal/include/bus_driver.h
@@ -205,6 +205,18 @@ typedef int (*rte_bus_hot_unplug_handler_t)(struct rte_device *dev);
  */
 typedef int (*rte_bus_sigbus_handler_t)(const void *failure_addr);
 
+/**
+ * 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);
+
 /**
  * Bus scan policies
  */
@@ -256,6 +268,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 9a168b7773..e74542fc71 100644
--- a/lib/eal/linux/eal.c
+++ b/lib/eal/linux/eal.c
@@ -1363,6 +1363,7 @@ rte_eal_cleanup(void)
 	vfio_mp_sync_cleanup();
 #endif
 	rte_mp_channel_cleanup();
+	eal_bus_cleanup();
 	rte_trace_save();
 	eal_trace_fini();
 	/* after this point, any DPDK pointers will become dangling */
diff --git a/lib/eal/windows/eal.c b/lib/eal/windows/eal.c
index 79322d2ce9..adb929a014 100644
--- a/lib/eal/windows/eal.c
+++ b/lib/eal/windows/eal.c
@@ -263,6 +263,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);
-- 
2.31.1

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

[David Marchand]

On Tue, Oct 4, 2022 at 6:47 PM Kevin Laatz <kevin.laatz@intel.com> 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.

Cc: maintainers for info.


> Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
> Acked-by: Morten Brørup <mb@smartsharesystems.com>
> Reviewed-by: Bruce Richardson <bruce.richardson@intel.com>

Applied, thanks.


-- 
David Marchand

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

[Thomas Monjalon]

05/10/2022 09:45, David Marchand:
> On Tue, Oct 4, 2022 at 6:47 PM Kevin Laatz <kevin.laatz@intel.com> wrote:
> > 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.
> 
> Cc: maintainers for info.

Kevin,
When you don't go to the end of a task, you must ask help to complete it.
Here you assume others will do it,
but you don't even Cc the relevant maintainers.

Please could you track that all buses will get the implementation?
You should open a bugzilla ticket for each bus,
and assign it to the relevant maintainer.
Thanks

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

[Kevin Laatz]

Hi Thomas, All,

On 05/10/2022 10:41, Thomas Monjalon wrote:
> 05/10/2022 09:45, David Marchand:
>> On Tue, Oct 4, 2022 at 6:47 PM Kevin Laatz <kevin.laatz@intel.com> wrote:
>>> 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.
>> Cc: maintainers for info.
> Kevin,
> When you don't go to the end of a task, you must ask help to complete it.
> Here you assume others will do it,
> but you don't even Cc the relevant maintainers.
>
> Please could you track that all buses will get the implementation?
> You should open a bugzilla ticket for each bus,
> and assign it to the relevant maintainer.
> Thanks
>
Bugzilla tickets created:

[auxiliary] https://bugs.dpdk.org/show_bug.cgi?id=1090
[dpaa] https://bugs.dpdk.org/show_bug.cgi?id=1091
[fslmc] https://bugs.dpdk.org/show_bug.cgi?id=1092
[vmbus] https://bugs.dpdk.org/show_bug.cgi?id=1093
[ifpga] https://bugs.dpdk.org/show_bug.cgi?id=1094

I've assigned to maintainers that have a registered account with Bugzilla.

I was not able to assign the iFPGA ticket to Rosen Xu since his email is 
not registered.

--
Kevin

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

[Thomas Monjalon]

05/10/2022 13:03, Kevin Laatz:
> Hi Thomas, All,
> 
> On 05/10/2022 10:41, Thomas Monjalon wrote:
> > 05/10/2022 09:45, David Marchand:
> >> On Tue, Oct 4, 2022 at 6:47 PM Kevin Laatz <kevin.laatz@intel.com> wrote:
> >>> 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.
> >> Cc: maintainers for info.
> > Kevin,
> > When you don't go to the end of a task, you must ask help to complete it.
> > Here you assume others will do it,
> > but you don't even Cc the relevant maintainers.
> >
> > Please could you track that all buses will get the implementation?
> > You should open a bugzilla ticket for each bus,
> > and assign it to the relevant maintainer.
> > Thanks
> >
> Bugzilla tickets created:
> 
> [auxiliary] https://bugs.dpdk.org/show_bug.cgi?id=1090
> [dpaa] https://bugs.dpdk.org/show_bug.cgi?id=1091
> [fslmc] https://bugs.dpdk.org/show_bug.cgi?id=1092
> [vmbus] https://bugs.dpdk.org/show_bug.cgi?id=1093
> [ifpga] https://bugs.dpdk.org/show_bug.cgi?id=1094
> 
> I've assigned to maintainers that have a registered account with Bugzilla.
> 
> I was not able to assign the iFPGA ticket to Rosen Xu since his email is 
> not registered.

Thanks, please don't hesitate to ping again if there is no fast enough progress.