> From: Kevin Laatz [mailto:kevin.la...@intel.com] > Sent: Friday, 22 April 2022 11.18 > > On 20/04/2022 07:55, Morten Brørup wrote: > >> From: Kevin Laatz [mailto:kevin.la...@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.la...@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 <m...@smartsharesystems.com> > >
