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

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