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
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?
> 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>
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> >
> 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> > >
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
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
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
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 >
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.
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 >>
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.
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
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?
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.
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
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>
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);
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
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
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?
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.
在 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);
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.
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
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
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
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
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
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
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!
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
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
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
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
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.