Re: [dpdk-dev] [RFC V1] examples/l3fwd-power: fix memory leak for rte_pci_device

[Huisong Li <lihuisong@huawei.com>]

在 2021/9/17 20:50, Thomas Monjalon 写道:
> 17/09/2021 04:13, Huisong Li:
>> 在 2021/9/16 18:36, Thomas Monjalon 写道:
>>> 16/09/2021 10:01, Huisong Li:
>>>> 在 2021/9/8 15:20, Thomas Monjalon 写道:
>>>>> 08/09/2021 04:01, Huisong Li:
>>>>>> 在 2021/9/7 16:53, Thomas Monjalon 写道:
>>>>>>> 07/09/2021 05:41, Huisong Li:
>>>>>>>> Calling rte_eth_dev_close() will release resources of eth device and close
>>>>>>>> it. But rte_pci_device struct isn't released when app exit, which will lead
>>>>>>>> to memory leak.
>>>>>>> That's a PMD issue.
>>>>>>> When the last port of a PCI device is closed, the device should be freed.
>>>>>> Why is this a PMD problem? I don't understand.
>>>>> In the PMD close function, freeing of PCI device must be managed,
>>>>> so the app doesn't have to bother.
>>>> I know what you mean. Currently, there are two ways to close PMD device
>>>> (rte_eth_dev_close() and rte_dev_remove()).
>>>>
>>>> For rte_dev_remove(), eth device can be closed and rte_pci_device also
>>>> can be freed, so it can make app not care about that.
>>>>
>>>> But dev_close() is only used to close eth device, and nothing about
>>>> rte_pci_device is involved in the framework layer
>>>>
>>>> call stack of dev_close(). The rte_pci_device is allocated and
>>>> initialized when the rte_pci_bus scans "/sys/bus/pci/devices" directory.
>>>>
>>>> Generally, the PMD of eth devices operates on the basis of eth devices,
>>>> and rarely on rte_pci_device.
>>> No. The PMD is doing the relation between the PCI device and the ethdev port.
>> It seems that the ethdev layer can create eth devices based on
>> rte_pci_device, but does not release rte_pci_device.
> No, the ethdev layer does not manage any bus.
> Only the PMD does that.

I don't mean that the ethdev layer manages the bus.

I mean, it neither allocate rte_pci_device nor free it.

>>>> And the rte_pci_device corresponding to the eth devices managed and
>>>> processed by rte_pci_bus.
>>>>
>>>> So, PMD is closed only based on the port ID of the eth device, whilch
>>>> only shuts down eth devices, not frees rte_pci_device
>>>> and remove it from rte_pci_bus.
>>> Not really.
>> I do not see any PMD driver releasing rte_pci_device in dev_close().
> Maybe not but we should.

I'm sure.

As far as I know, the PMD does not free rte_pci_device for devices under 
the PCI bus, whether ethdev or dmadev.

>
>>> If there is no port using the PCI device, it should be released.
>> Yes.
>>>>>> As far as I know, most apps or examples in the DPDK project have only
>>>>>> one port for a pci device.
>>>>> The number of ports per PCI device is driver-specific.
>>>>>
>>>>>> When the port is closed, the rte_pci_device should be freed. But none of
>>>>>> the apps seem to do this.
>>>>> That's because from the app point of view, only ports should be managed.
>>>>> The hardware device is managed by the PMD.
>>>>> Only drivers (PMDs) have to do the relation between class ports
>>>>> and hardware devices.
>>>> Yes. But the current app only closes the port to disable the PMD, and
>>>> the rte_pci_device cannot be freed.
>>> Why not?
>> Because most apps in DPDK call dev_close() to close the eth device
>> corresponding to a port.
> You don't say why the underlying PCI device could not be freed.
 From the current implementation, rte_eth_dev_close() in ethdev layer 
and dev_close() in PMD both do not free it.
>
>>>> Because rte_pci_device cannot be released in dev_close() of PMD, and is
>>>> managed by framework layer.
>>> No
>>>
>>>> Btw. Excluding rte_dev_probe() and rte_dev_remove(),  it seems that the
>>>> DPDK framework only automatically
>>>> scans PCI devices, but does not automatically release PCI devices when
>>>> the process exits.
>>> Indeed, because such freeing is the responsibility of the PMD.
>> Do you mean to free rte_pci_device in the dev_close() API?
> I mean free the PCI device in the PMD implementation of dev_close.

I don't think it's reasonable.

In the normal process, the rte_pci_device is allocated rte_eal_init() 
when pci bus scan "/sys/bus/pci/devices"

by calling rte_bus_scan() and insert to rte_pci_bus.device_list.

Then, calling rte_bus_probe() in rte_eal_init to match rte_pci_device 
and rte_pci_driver registered under rte_pci_bus

to generate an eth device.

 From this point of view, the rte_pci_device should be managed and 
released by the rte_pci_bus.

Generally, the uninstallation operation should be reversed. Release the 
eth device first and then release the rte_pci_device.

Therefore the rte_pci_device  does not be freed in the PMD 
implementation of dev_close.

>
>> How should PMD free it? What should we do? Any good suggestions?
> Check that there is no other port sharing the same PCI device,
> then call the PMD callback for rte_pci_remove_t.

For primary and secondary processes, their rte_pci_device is independent.

Is this for a scenario where there are multiple representor ports under 
the same PCI address in the same processe?

>> Would it be more appropriate to do this in rte_eal_cleanup() if it
>> cann't be done in the API above?
> rte_eal_cleanup is a last cleanup for what was not done earlier.
> We could do that but first we should properly free devices when closed.
>
Totally, it is appropriate that rte_eal_cleanup is responsible for 
releasing devices under the pci bus.
> .