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

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

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!