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

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

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

Fixes: 613ce6691c0d ("examples/l3fwd-power: implement proper shutdown")

Signed-off-by: Huisong Li <lihuisong@huawei.com>
---
 examples/l3fwd-power/main.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/examples/l3fwd-power/main.c b/examples/l3fwd-power/main.c
index aa7b8db44a..6498b5225e 100644
--- a/examples/l3fwd-power/main.c
+++ b/examples/l3fwd-power/main.c
@@ -2512,6 +2512,8 @@ main(int argc, char **argv)
 	struct lcore_conf *qconf;
 	struct rte_eth_dev_info dev_info;
 	struct rte_eth_txconf *txconf;
+	struct rte_eth_dev *eth_dev;
+	struct rte_device *rte_dev;
 	int ret;
 	uint16_t nb_ports;
 	uint16_t queueid;
@@ -2910,7 +2912,14 @@ main(int argc, char **argv)
 			RTE_LOG(ERR, L3FWD_POWER, "rte_eth_dev_stop: err=%d, port=%u\n",
 				ret, portid);
 
+		/* Retrieve device address in eth device before closing it. */
+		eth_dev = &rte_eth_devices[portid];
+		rte_dev = eth_dev->device;
 		rte_eth_dev_close(portid);
+		ret = rte_dev_remove(rte_dev);
+		if (ret != 0)
+			RTE_LOG(ERR, L3FWD_POWER, "rte_dev_remove: err=%d, port=%u\n",
+				ret, portid);
 	}
 
 	if (app_mode == APP_MODE_EMPTY_POLL)
-- 
2.33.0

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

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

> +		/* Retrieve device address in eth device before closing it. */
> +		eth_dev = &rte_eth_devices[portid];

You should not access this array, considered internal.

> +		rte_dev = eth_dev->device;
>  		rte_eth_dev_close(portid);
> +		ret = rte_dev_remove(rte_dev);

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

[Huisong Li]

Hi, Thomas

在 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.

As far as I know, most apps or examples in the DPDK project have only 
one port for a pci device.

When the port is closed, the rte_pci_device should be freed. But none of 
the apps seem to do this.

>
>> +		/* Retrieve device address in eth device before closing it. */
>> +		eth_dev = &rte_eth_devices[portid];
> You should not access this array, considered internal.

We have to save the address of rte_device to free rte_pci_device before 
closing eth device.

Because the the device address in rte_eth_dev struct will be set to a 
NULL after closing eth device.

It's also handled in OVS in this way.

>
>> +		rte_dev = eth_dev->device;
>>   		rte_eth_dev_close(portid);
>> +		ret = rte_dev_remove(rte_dev);
>
>
> .

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

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

> 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.

> >> +		/* Retrieve device address in eth device before closing it. */
> >> +		eth_dev = &rte_eth_devices[portid];
> > You should not access this array, considered internal.
> 
> We have to save the address of rte_device to free rte_pci_device before 
> closing eth device.
> 
> Because the the device address in rte_eth_dev struct will be set to a 
> NULL after closing eth device.
> 
> It's also handled in OVS in this way.

No you don't have to call rte_dev_remove at all from an app.

> >> +		rte_dev = eth_dev->device;
> >>   		rte_eth_dev_close(portid);
> >> +		ret = rte_dev_remove(rte_dev);

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

[Huisong Li]

Hi, Thomas

The new comments are as follows, and look forward to your reply. Thanks!


在 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.

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.

>
>> 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.

Because rte_pci_device cannot be released in dev_close() of PMD, and is 
managed by framework layer.


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.

Above "automatic", I means that it doesn't involve apps or PMDs.

>
>>>> +		/* Retrieve device address in eth device before closing it. */
>>>> +		eth_dev = &rte_eth_devices[portid];
>>> You should not access this array, considered internal.
>> We have to save the address of rte_device to free rte_pci_device before
>> closing eth device.
>>
>> Because the the device address in rte_eth_dev struct will be set to a
>> NULL after closing eth device.
>>
>> It's also handled in OVS in this way.
> No you don't have to call rte_dev_remove at all from an app.
>
>>>> +		rte_dev = eth_dev->device;
>>>>    		rte_eth_dev_close(portid);
>>>> +		ret = rte_dev_remove(rte_dev);
>
>
> .

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

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

> 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.
If there is no port using the PCI device, it should be released.

> >> 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 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.

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

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

>
>> 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().
> 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.
>
>> 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?

How should PMD free it? What should we do? Any good suggestions?

Would it be more appropriate to do this in rte_eal_cleanup() if it 
cann't be done in the API above?
>
>
>
> .

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

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

> >> 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.

> > 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.

> >> 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.

> 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.

> 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.

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

[Huisong Li]

在 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.
> .

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

[Thomas Monjalon]

18/09/2021 05:24, Huisong Li:
> 在 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.

What is not reasonable, is to not free a device which is closed.

> 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.

Same for mbuf in mempool: allocation is done by the app,
free is done by the PMD.
Not everything is symmetrical.

> 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.

Yes it requires to free on both primary and secondary.

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

A PCI device can have multiple physical or representor ports.

> >> 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.

Yes, but if a device is closed while the rest of the app keep running,
we should not wait to free it.

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

[Huisong Li]

在 2021/9/18 16:46, Thomas Monjalon 写道:
> 18/09/2021 05:24, Huisong Li:
>> 在 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.
> What is not reasonable, is to not free a device which is closed.
>
>> 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.
> Same for mbuf in mempool: allocation is done by the app,
> free is done by the PMD.

That doesn't seem to be the case. In the rx direction, the mbuf is 
allocated by PMD.

So it should be freed by the PMD.

> Not everything is symmetrical.
>
>> 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.
> Yes it requires to free on both primary and secondary.
>
>> Is this for a scenario where there are multiple representor ports under
>> the same PCI address in the same processe?
> A PCI device can have multiple physical or representor ports.
Got it.
>
>>>> 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.
> Yes, but if a device is closed while the rest of the app keep running,
> we should not wait to free it.

 From this point of view, it seems to make sense. However, according to 
the OVS-DPDK

usage, it calls dev_close() first, and then check whether all ports 
under the PCI address are

closed to free rte_pci_device by calling rte_dev_remove().


If we do not want the user to be aware of this, and we want 
rte_pci_device to be freed

in a timely manner. Can we add a code logic calculating the number of 
ports under a PCI address

and calling rte_dev_remove() to rte_eth_dev_close() to free 
rte_pci_device and delete it from rte_pci_bus?

If we do, we may need to make some extra work, otherwise some 
applications, such as OVS-DPDK, will

fail due to a second call to rte_dev_remove().

>
>
> .

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

[Thomas Monjalon]

26/09/2021 14:20, Huisong Li:
> 在 2021/9/18 16:46, Thomas Monjalon 写道:
> > 18/09/2021 05:24, Huisong Li:
> >> 在 2021/9/17 20:50, Thomas Monjalon 写道:
> >>> 17/09/2021 04:13, Huisong Li:
> >>>> 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.
> > Yes it requires to free on both primary and secondary.
> >
> >> Is this for a scenario where there are multiple representor ports under
> >> the same PCI address in the same processe?
> > A PCI device can have multiple physical or representor ports.
> Got it.
> >
> >>>> 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.
> > Yes, but if a device is closed while the rest of the app keep running,
> > we should not wait to free it.
> 
>  From this point of view, it seems to make sense. However, according to 
> the OVS-DPDK
> 
> usage, it calls dev_close() first, and then check whether all ports 
> under the PCI address are
> 
> closed to free rte_pci_device by calling rte_dev_remove().
> 
> 
> If we do not want the user to be aware of this, and we want 
> rte_pci_device to be freed
> 
> in a timely manner. Can we add a code logic calculating the number of 
> ports under a PCI address
> 
> and calling rte_dev_remove() to rte_eth_dev_close() to free 
> rte_pci_device and delete it from rte_pci_bus?
> 
> If we do, we may need to make some extra work, otherwise some 
> applications, such as OVS-DPDK, will
> 
> fail due to a second call to rte_dev_remove().

I don't understand the proposal.
Please could explain again the code path?
It may deserve a separate mail thread.

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

[Huisong Li]

在 2021/9/27 3:16, Thomas Monjalon 写道:
> 26/09/2021 14:20, Huisong Li:
>> 在 2021/9/18 16:46, Thomas Monjalon 写道:
>>> 18/09/2021 05:24, Huisong Li:
>>>> 在 2021/9/17 20:50, Thomas Monjalon 写道:
>>>>> 17/09/2021 04:13, Huisong Li:
>>>>>> 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.
>>> Yes it requires to free on both primary and secondary.
>>>
>>>> Is this for a scenario where there are multiple representor ports under
>>>> the same PCI address in the same processe?
>>> A PCI device can have multiple physical or representor ports.
>> Got it.
>>>>>> 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.
>>> Yes, but if a device is closed while the rest of the app keep running,
>>> we should not wait to free it.
>>   From this point of view, it seems to make sense. However, according to
>> the OVS-DPDK
>>
>> usage, it calls dev_close() first, and then check whether all ports
>> under the PCI address are
>>
>> closed to free rte_pci_device by calling rte_dev_remove().
>>
>>
>> If we do not want the user to be aware of this, and we want
>> rte_pci_device to be freed
>>
>> in a timely manner. Can we add a code logic calculating the number of
>> ports under a PCI address
>>
>> and calling rte_dev_remove() to rte_eth_dev_close() to free
>> rte_pci_device and delete it from rte_pci_bus?
>>
>> If we do, we may need to make some extra work, otherwise some
>> applications, such as OVS-DPDK, will
>>
>> fail due to a second call to rte_dev_remove().
> I don't understand the proposal.
> Please could explain again the code path?

1. This RFC patch intended to free rte_pci_device in DPDK app by calling

rte_dev_remove() after calling dev_close().

2. For the above-mentioned usage in OVS-DPDK, please see function

netdev_dpdk_destruct() in lib/netdev-dpdk.c.

3. Later, you suggest that the release of rte_pci_device should be done

in the dev_close() API, not in the rte_eal_init() which is not real-time.

To sum up, the above proposal comes out.

> It may deserve a separate mail thread.
>
>
> .

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

[Huisong Li]

Hi. Thomas

I've summed up our previous discussion.

Can you look at the final proposal again?

Do you think we should deal with the problem better?


在 2021/9/27 9:44, Huisong Li 写道:
>
> 在 2021/9/27 3:16, Thomas Monjalon 写道:
>> 26/09/2021 14:20, Huisong Li:
>>> 在 2021/9/18 16:46, Thomas Monjalon 写道:
>>>> 18/09/2021 05:24, Huisong Li:
>>>>> 在 2021/9/17 20:50, Thomas Monjalon 写道:
>>>>>> 17/09/2021 04:13, Huisong Li:
>>>>>>> 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.
>>>> Yes it requires to free on both primary and secondary.
>>>>
>>>>> Is this for a scenario where there are multiple representor ports 
>>>>> under
>>>>> the same PCI address in the same processe?
>>>> A PCI device can have multiple physical or representor ports.
>>> Got it.
>>>>>>> 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.
>>>> Yes, but if a device is closed while the rest of the app keep running,
>>>> we should not wait to free it.
>>>   From this point of view, it seems to make sense. However, 
>>> according to
>>> the OVS-DPDK
>>>
>>> usage, it calls dev_close() first, and then check whether all ports
>>> under the PCI address are
>>>
>>> closed to free rte_pci_device by calling rte_dev_remove().
>>>
>>>
>>> If we do not want the user to be aware of this, and we want
>>> rte_pci_device to be freed
>>>
>>> in a timely manner. Can we add a code logic calculating the number of
>>> ports under a PCI address
>>>
>>> and calling rte_dev_remove() to rte_eth_dev_close() to free
>>> rte_pci_device and delete it from rte_pci_bus?
>>>
>>> If we do, we may need to make some extra work, otherwise some
>>> applications, such as OVS-DPDK, will
>>>
>>> fail due to a second call to rte_dev_remove().
>> I don't understand the proposal.
>> Please could explain again the code path?
>
> 1. This RFC patch intended to free rte_pci_device in DPDK app by calling
>
> rte_dev_remove() after calling dev_close().
>
> 2. For the above-mentioned usage in OVS-DPDK, please see function
>
> netdev_dpdk_destruct() in lib/netdev-dpdk.c.
>
> 3. Later, you suggest that the release of rte_pci_device should be done
>
> in the dev_close() API, not in the rte_eal_init() which is not real-time.
>
> To sum up, the above proposal comes out.
>
>> It may deserve a separate mail thread.
>>
>>
>> .
> .

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

[Thomas Monjalon]

30/09/2021 08:28, Huisong Li:
> Hi. Thomas
> 
> I've summed up our previous discussion.
> 
> Can you look at the final proposal again?
> 
> Do you think we should deal with the problem better?

I don't understand what is the final proposal.


> 在 2021/9/27 9:44, Huisong Li 写道:
> >
> > 在 2021/9/27 3:16, Thomas Monjalon 写道:
> >> 26/09/2021 14:20, Huisong Li:
> >>> 在 2021/9/18 16:46, Thomas Monjalon 写道:
> >>>> 18/09/2021 05:24, Huisong Li:
> >>>>> 在 2021/9/17 20:50, Thomas Monjalon 写道:
> >>>>>> 17/09/2021 04:13, Huisong Li:
> >>>>>>> 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.
> >>>> Yes it requires to free on both primary and secondary.
> >>>>
> >>>>> Is this for a scenario where there are multiple representor ports 
> >>>>> under
> >>>>> the same PCI address in the same processe?
> >>>> A PCI device can have multiple physical or representor ports.
> >>> Got it.
> >>>>>>> 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.
> >>>> Yes, but if a device is closed while the rest of the app keep running,
> >>>> we should not wait to free it.
> >>>   From this point of view, it seems to make sense. However, 
> >>> according to
> >>> the OVS-DPDK
> >>>
> >>> usage, it calls dev_close() first, and then check whether all ports
> >>> under the PCI address are
> >>>
> >>> closed to free rte_pci_device by calling rte_dev_remove().
> >>>
> >>>
> >>> If we do not want the user to be aware of this, and we want
> >>> rte_pci_device to be freed
> >>>
> >>> in a timely manner. Can we add a code logic calculating the number of
> >>> ports under a PCI address
> >>>
> >>> and calling rte_dev_remove() to rte_eth_dev_close() to free
> >>> rte_pci_device and delete it from rte_pci_bus?
> >>>
> >>> If we do, we may need to make some extra work, otherwise some
> >>> applications, such as OVS-DPDK, will
> >>>
> >>> fail due to a second call to rte_dev_remove().
> >> I don't understand the proposal.
> >> Please could explain again the code path?
> >
> > 1. This RFC patch intended to free rte_pci_device in DPDK app by calling
> >
> > rte_dev_remove() after calling dev_close().
> >
> > 2. For the above-mentioned usage in OVS-DPDK, please see function
> >
> > netdev_dpdk_destruct() in lib/netdev-dpdk.c.
> >
> > 3. Later, you suggest that the release of rte_pci_device should be done
> >
> > in the dev_close() API, not in the rte_eal_init() which is not real-time.
> >
> > To sum up, the above proposal comes out.
> >
> >> It may deserve a separate mail thread.
> >>
> >>
> >> .
> > .
> 

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

[lihuisong (C)]

在 2021/9/30 15:50, Thomas Monjalon 写道:
> 30/09/2021 08:28, Huisong Li:
>> Hi. Thomas
>>
>> I've summed up our previous discussion.
>>
>> Can you look at the final proposal again?
>>
>> Do you think we should deal with the problem better?
> I don't understand what is the final proposal.
Sorry.

The last idea we discussed was:

As you mentioned, if we do not want the user to free rte_pci_device and 
we want rte_pci_device

to be freed in time. Can we add a code logic calculating the number of 
ports under a PCI address

and calling rte_dev_remove() in rte_eth_dev_close() to free 
rte_pci_device and delete it from rte_pci_bus?

If we do, we may need to make some extra work, otherwise some 
applications, such as OVS-DPDK, will

fail due to a second call to rte_dev_remove().


The method of releasing rte_pci_device in OVS-DPDK is as follows:

It calls dev_close() first, and then check whether all ports under the 
PCI address are closed

to free rte_pci_device by calling rte_dev_remove().

If it's not clear enough, please take a look at the discussion in our 
email line. Thanks.😁
>
>
>> 在 2021/9/27 9:44, Huisong Li 写道:
>>> 在 2021/9/27 3:16, Thomas Monjalon 写道:
>>>> 26/09/2021 14:20, Huisong Li:
>>>>> 在 2021/9/18 16:46, Thomas Monjalon 写道:
>>>>>> 18/09/2021 05:24, Huisong Li:
>>>>>>> 在 2021/9/17 20:50, Thomas Monjalon 写道:
>>>>>>>> 17/09/2021 04:13, Huisong Li:
>>>>>>>>> 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.
>>>>>> Yes it requires to free on both primary and secondary.
>>>>>>
>>>>>>> Is this for a scenario where there are multiple representor ports
>>>>>>> under
>>>>>>> the same PCI address in the same processe?
>>>>>> A PCI device can have multiple physical or representor ports.
>>>>> Got it.
>>>>>>>>> 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.
>>>>>> Yes, but if a device is closed while the rest of the app keep running,
>>>>>> we should not wait to free it.
>>>>>    From this point of view, it seems to make sense. However,
>>>>> according to
>>>>> the OVS-DPDK
>>>>>
>>>>> usage, it calls dev_close() first, and then check whether all ports
>>>>> under the PCI address are
>>>>>
>>>>> closed to free rte_pci_device by calling rte_dev_remove().
>>>>>
>>>>>
>>>>> If we do not want the user to be aware of this, and we want
>>>>> rte_pci_device to be freed
>>>>>
>>>>> in a timely manner. Can we add a code logic calculating the number of
>>>>> ports under a PCI address
>>>>>
>>>>> and calling rte_dev_remove() to rte_eth_dev_close() to free
>>>>> rte_pci_device and delete it from rte_pci_bus?
>>>>>
>>>>> If we do, we may need to make some extra work, otherwise some
>>>>> applications, such as OVS-DPDK, will
>>>>>
>>>>> fail due to a second call to rte_dev_remove().
>>>> I don't understand the proposal.
>>>> Please could explain again the code path?
>>> 1. This RFC patch intended to free rte_pci_device in DPDK app by calling
>>>
>>> rte_dev_remove() after calling dev_close().
>>>
>>> 2. For the above-mentioned usage in OVS-DPDK, please see function
>>>
>>> netdev_dpdk_destruct() in lib/netdev-dpdk.c.
>>>
>>> 3. Later, you suggest that the release of rte_pci_device should be done
>>>
>>> in the dev_close() API, not in the rte_eal_init() which is not real-time.
>>>
>>> To sum up, the above proposal comes out.
>>>
>>>> It may deserve a separate mail thread.
>>>>
>>>>
>>>> .
>>> .
>
>
>
>
> .
>

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

[Thomas Monjalon]

08/10/2021 08:26, lihuisong (C):
> As you mentioned, if we do not want the user to free rte_pci_device and 
> we want rte_pci_device
> to be freed in time. Can we add a code logic calculating the number of 
> ports under a PCI address
> and calling rte_dev_remove() in rte_eth_dev_close() to free 
> rte_pci_device and delete it from rte_pci_bus?

Yes that's the idea.
But it cannot be done in ethdev lib,
it should be the responsibility of the driver.
Only the driver knows exactly what to free.