[dpdk-dev] [PATCH v2] librte_ether: release memory in uninit function.
On 7/9/2015 11:32 AM, Tetsuya Mukawa wrote: > On 2015/07/08 18:59, Thomas Monjalon wrote: >> 2015-07-08 09:49, Iremonger, Bernard: >>> From: Tetsuya Mukawa [mailto:mukawa at igel.co.jp] On 2015/07/07 19:53, Iremonger, Bernard wrote: > From: Tetsuya Mukawa [mailto:mukawa at igel.co.jp] >> On 2015/07/06 20:35, Qiu, Michael wrote: >>> Hi, all >>> >>> As we has gap on the memory release action to be done in which step, >>> I appreciate all your comments on this patch. >>> >>> Currently, the correct quit sequence for testpmd is stop() ---> >>> port_stop() --> port_close() --> quit(). This will lead lots of >>> memory not released by default, like queues. [.../...] >> It appears that the API is not defined. >> None of these assumptions is written in rte_ethdev.h. >> Please continue the discussion by submitting a patch fixing the doxygen >> comments of these functions. >> How an application developper is supposed to use these functions without >> having a clear explanation of their roles in doxygen? > Yes, this assumption is only written in DPDK user's guide. > So we should add description to doxygen. I will take care of it. > > Anyway, I will add description to doxygen like user's guide. > (A) stop() and close() must be called before detach(). > (B) close() releases all most all resources. > Probably Bernard and Michael patches are fit for above restrictions. > > It may be good to add below in DPDK-2.2, though supporting below feature > will be more user friendly. > (C) Even if stop() and close() are not called, detach() will take care > of it. > > Probably, it's not so much time left before releasing DPDK-2.1. > So how about keeping current restriction written in user's guide, then > add a new feature in DPDK-2.2. Yes, agree. Thanks, Michael > Regards, > Tetsuya >
[dpdk-dev] [PATCH v2] librte_ether: release memory in uninit function.
On 2015/07/08 18:59, Thomas Monjalon wrote: > 2015-07-08 09:49, Iremonger, Bernard: >> From: Tetsuya Mukawa [mailto:mukawa at igel.co.jp] >>> On 2015/07/07 19:53, Iremonger, Bernard wrote: From: Tetsuya Mukawa [mailto:mukawa at igel.co.jp] > On 2015/07/06 20:35, Qiu, Michael wrote: >> Hi, all >> >> As we has gap on the memory release action to be done in which step, >> I appreciate all your comments on this patch. >> >> Currently, the correct quit sequence for testpmd is stop() ---> >> port_stop() --> port_close() --> quit(). This will lead lots of >> memory not released by default, like queues. There is also the scenario (outside of testpmd) where an application can call >>> rte_eth_dev_close() or rte_eth_dev_detach() which calls >>> rte_eth_dev_uninit() directly from an application. In these cases the memory allocated in the EAL layer should be released in >>> both rte_eth_dev_close() and rte_eth_dev_detach(). >>> >>> Hi Bernard, >>> >>> All DPDK applications that uses hotpluggingmust call rte_eth_dev_stop() and >>> rte_eth_dev_close()APIs before detaching ports. >>> (This is described in DPDK documentation) Could we ignore applications that >>> only calls rte_eth_dev_detach()? >>> This patch is intended to address the rte_eth_dev_detach() case only >>> (hotplug case). Perhaps there should be a separate patch to address the >>> rte_eth_dev_close() case. >>> >>> We all needs to have consensus about how to implement finalization code in >>> close() and uninit(). >>> Probably one of options will be implementing finalization code in >>> close() as much as possible. >>> >>> Tetsuya, >> Hi Tetsuya, >> Testpmd enforces the dev_stop(), dev_close() and dev_detach() sequence. >> There is no reason why an application cannot call dev_detach() directly. >> >> During internal review of PMD dev_uninit() functions it was decided to >> ensure that this sequence was adhered to by calling dev_close() which calls >> dev_stop() from the dev_uninit() function. >> In the PMD hotplug patches the following calling sequence is implemented: >> dev_uninit() calls dev_close() calls dev_stop(). >> At present most of the finalization code is implemented in dev_close() and >> dev_stop(). >> The dev_uninit() functions implement what is left of the finalization code. > It appears that the API is not defined. > None of these assumptions is written in rte_ethdev.h. > Please continue the discussion by submitting a patch fixing the doxygen > comments of these functions. > How an application developper is supposed to use these functions without > having a clear explanation of their roles in doxygen? Yes, this assumption is only written in DPDK user's guide. So we should add description to doxygen. I will take care of it. Anyway, I will add description to doxygen like user's guide. (A) stop() and close() must be called before detach(). (B) close() releases all most all resources. Probably Bernard and Michael patches are fit for above restrictions. It may be good to add below in DPDK-2.2, though supporting below feature will be more user friendly. (C) Even if stop() and close() are not called, detach() will take care of it. Probably, it's not so much time left before releasing DPDK-2.1. So how about keeping current restriction written in user's guide, then add a new feature in DPDK-2.2. Regards, Tetsuya
[dpdk-dev] [PATCH v2] librte_ether: release memory in uninit function.
On 2015/07/07 19:53, Iremonger, Bernard wrote: >> -Original Message- >> From: Tetsuya Mukawa [mailto:mukawa at igel.co.jp] >> Sent: Tuesday, July 7, 2015 4:38 AM >> To: dev at dpdk.org >> Cc: Qiu, Michael; Iremonger, Bernard; thomas.monjalon at 6wind.com; >> Ananyev, Konstantin; Stephen Hemminger; Zhang, Helin; Chen, Jing D; Neil >> Horman >> Subject: Re: [dpdk-dev] [PATCH v2] librte_ether: release memory in uninit >> function. >> >> On 2015/07/06 20:35, Qiu, Michael wrote: >>> Hi, all >>> >>> As we has gap on the memory release action to be done in which step, I >>> appreciate all your comments on this patch. >>> >>> Currently, the correct quit sequence for testpmd is stop() ---> >>> port_stop() --> port_close() --> quit(). This will lead lots of memory >>> not released by default, like queues. > There is also the scenario (outside of testpmd) where an application can call > rte_eth_dev_close() or rte_eth_dev_detach() which calls rte_eth_dev_uninit() > directly from an application. > In these cases the memory allocated in the EAL layer should be released in > both rte_eth_dev_close() and rte_eth_dev_detach(). Hi Bernard, All DPDK applications that uses hotpluggingmust call rte_eth_dev_stop() and rte_eth_dev_close()APIs before detaching ports. (This is described in DPDK documentation) Could we ignore applications that only calls rte_eth_dev_detach()? > This patch is intended to address the rte_eth_dev_detach() case only (hotplug > case). > Perhaps there should be a separate patch to address the rte_eth_dev_close() > case. We all needs to have consensus about how to implement finalization code in close() and uninit(). Probably one of options will be implementing finalization code in close() as much as possible. Tetsuya, > Regards, > > Bernard. > >>> We have 3 potential proposals(normal case means without hotplug): >>> >>> 1. Those memory release in close() other left in uninit() >>> This will work fine for both hotplug case and normal case. >> +1 >> >> I assume that once close() is called, we cannot start the port again without >> hotplugging. >> This is my premise. >> >> It might be good that we move finalization code to close() as much as >> possible, because of followings. >> 1. Anyway, once close() is called, we cannot start the port again. So it >> should >> be ok to free resources in close(). >> 2. Non-hotplugging applications can release resources if we implement >> finalization code to close(). >> >> Also I guess Stephen's suggestion is important to keep code clean. >> It may be good that 'dev->data->rx/tx_queues[queue_id]' are freed in >> rx/tx_queue_release() of each PMD, and rx/tx_queue_release() will be >> called by rte_eth_dev_close(). Also 'dev->data->rx/tx_queues[]' should be >> freed in ethdev.c. >> >>> 3. Combine close() and uninit(), only one will be left. >>> This will work fine for both hotplug case and normal case. But it >>> may change a lot, such as logic. >> I guess this will be second option. But I agree we need to change a lot. >> Also >> after enabling hotplug by default, when someone only wants to close the >> port, it will be impossible. >> >> Regards, >> Tetsuya
[dpdk-dev] [PATCH v2] librte_ether: release memory in uninit function.
2015-07-08 09:49, Iremonger, Bernard: > From: Tetsuya Mukawa [mailto:mukawa at igel.co.jp] > > On 2015/07/07 19:53, Iremonger, Bernard wrote: > > > From: Tetsuya Mukawa [mailto:mukawa at igel.co.jp] > > >> On 2015/07/06 20:35, Qiu, Michael wrote: > > >>> Hi, all > > >>> > > >>> As we has gap on the memory release action to be done in which step, > > >>> I appreciate all your comments on this patch. > > >>> > > >>> Currently, the correct quit sequence for testpmd is stop() ---> > > >>> port_stop() --> port_close() --> quit(). This will lead lots of > > >>> memory not released by default, like queues. > > > There is also the scenario (outside of testpmd) where an application can > > > call > > rte_eth_dev_close() or rte_eth_dev_detach() which calls > > rte_eth_dev_uninit() directly from an application. > > > In these cases the memory allocated in the EAL layer should be released in > > both rte_eth_dev_close() and rte_eth_dev_detach(). > > > > Hi Bernard, > > > > All DPDK applications that uses hotpluggingmust call rte_eth_dev_stop() and > > rte_eth_dev_close()APIs before detaching ports. > > (This is described in DPDK documentation) Could we ignore applications that > > only calls rte_eth_dev_detach()? > > > > > This patch is intended to address the rte_eth_dev_detach() case only > > (hotplug case). > > > Perhaps there should be a separate patch to address the > > rte_eth_dev_close() case. > > > > We all needs to have consensus about how to implement finalization code in > > close() and uninit(). > > Probably one of options will be implementing finalization code in > > close() as much as possible. > > > > Tetsuya, > > Hi Tetsuya, > Testpmd enforces the dev_stop(), dev_close() and dev_detach() sequence. > There is no reason why an application cannot call dev_detach() directly. > > During internal review of PMD dev_uninit() functions it was decided to > ensure that this sequence was adhered to by calling dev_close() which calls > dev_stop() from the dev_uninit() function. > In the PMD hotplug patches the following calling sequence is implemented: > dev_uninit() calls dev_close() calls dev_stop(). > At present most of the finalization code is implemented in dev_close() and > dev_stop(). > The dev_uninit() functions implement what is left of the finalization code. It appears that the API is not defined. None of these assumptions is written in rte_ethdev.h. Please continue the discussion by submitting a patch fixing the doxygen comments of these functions. How an application developper is supposed to use these functions without having a clear explanation of their roles in doxygen?
[dpdk-dev] [PATCH v2] librte_ether: release memory in uninit function.
> -Original Message- > From: Tetsuya Mukawa [mailto:mukawa at igel.co.jp] > Sent: Wednesday, July 8, 2015 4:48 AM > To: Iremonger, Bernard; dev at dpdk.org > Cc: Qiu, Michael; thomas.monjalon at 6wind.com; Ananyev, Konstantin; > Stephen Hemminger; Zhang, Helin; Chen, Jing D; Neil Horman > Subject: Re: [dpdk-dev] [PATCH v2] librte_ether: release memory in uninit > function. > > On 2015/07/07 19:53, Iremonger, Bernard wrote: > >> -Original Message- > >> From: Tetsuya Mukawa [mailto:mukawa at igel.co.jp] > >> Sent: Tuesday, July 7, 2015 4:38 AM > >> To: dev at dpdk.org > >> Cc: Qiu, Michael; Iremonger, Bernard; thomas.monjalon at 6wind.com; > >> Ananyev, Konstantin; Stephen Hemminger; Zhang, Helin; Chen, Jing D; > >> Neil Horman > >> Subject: Re: [dpdk-dev] [PATCH v2] librte_ether: release memory in > >> uninit function. > >> > >> On 2015/07/06 20:35, Qiu, Michael wrote: > >>> Hi, all > >>> > >>> As we has gap on the memory release action to be done in which step, > >>> I appreciate all your comments on this patch. > >>> > >>> Currently, the correct quit sequence for testpmd is stop() ---> > >>> port_stop() --> port_close() --> quit(). This will lead lots of > >>> memory not released by default, like queues. > > There is also the scenario (outside of testpmd) where an application can > > call > rte_eth_dev_close() or rte_eth_dev_detach() which calls > rte_eth_dev_uninit() directly from an application. > > In these cases the memory allocated in the EAL layer should be released in > both rte_eth_dev_close() and rte_eth_dev_detach(). > > Hi Bernard, > > All DPDK applications that uses hotpluggingmust call rte_eth_dev_stop() and > rte_eth_dev_close()APIs before detaching ports. > (This is described in DPDK documentation) Could we ignore applications that > only calls rte_eth_dev_detach()? > > > This patch is intended to address the rte_eth_dev_detach() case only > (hotplug case). > > Perhaps there should be a separate patch to address the > rte_eth_dev_close() case. > > We all needs to have consensus about how to implement finalization code in > close() and uninit(). > Probably one of options will be implementing finalization code in > close() as much as possible. > > Tetsuya, Hi Tetsuya, Testpmd enforces the dev_stop(), dev_close() and dev_detach() sequence. There is no reason why an application cannot call dev_detach() directly. During internal review of PMD dev_uninit() functions it was decided to ensure that this sequence was adhered to by calling dev_close() which calls dev_stop() from the dev_uninit() function. In the PMD hotplug patches the following calling sequence is implemented: dev_uninit() calls dev_close() calls dev_stop(). At present most of the finalization code is implemented in dev_close() and dev_stop(). The dev_uninit() functions implement what is left of the finalization code. Regards, Bernard. > > > Regards, > > > > Bernard. > > > >>> We have 3 potential proposals(normal case means without hotplug): > >>> > >>> 1. Those memory release in close() other left in uninit() > >>> This will work fine for both hotplug case and normal case. > >> +1 > >> > >> I assume that once close() is called, we cannot start the port again > >> without hotplugging. > >> This is my premise. > >> > >> It might be good that we move finalization code to close() as much as > >> possible, because of followings. > >> 1. Anyway, once close() is called, we cannot start the port again. So > >> it should be ok to free resources in close(). > >> 2. Non-hotplugging applications can release resources if we implement > >> finalization code to close(). > >> > >> Also I guess Stephen's suggestion is important to keep code clean. > >> It may be good that 'dev->data->rx/tx_queues[queue_id]' are freed in > >> rx/tx_queue_release() of each PMD, and rx/tx_queue_release() will be > >> called by rte_eth_dev_close(). Also 'dev->data->rx/tx_queues[]' > >> should be freed in ethdev.c. > >> > >>> 3. Combine close() and uninit(), only one will be left. > >>> This will work fine for both hotplug case and normal case. But > >>> it may change a lot, such as logic. > >> I guess this will be second option. But I agree we need to change a > >> lot. Also after enabling hotplug by default, when someone only wants > >> to close the port, it will be impossible. > >> > >> Regards, > >> Tetsuya
[dpdk-dev] [PATCH v2] librte_ether: release memory in uninit function.
On 2015/07/06 20:35, Qiu, Michael wrote: > Hi, all > > As we has gap on the memory release action to be done in which step, I > appreciate all your comments on this patch. > > Currently, the correct quit sequence for testpmd is stop() ---> > port_stop() --> port_close() --> quit(). This will lead lots of memory > not released by default, like queues. > > We have 3 potential proposals(normal case means without hotplug): > > 1. Those memory release in close() other left in uninit() > This will work fine for both hotplug case and normal case. +1 I assume that once close() is called, we cannot start the port again without hotplugging. This is my premise. It might be good that we move finalization code to close() as much as possible, because of followings. 1. Anyway, once close() is called, we cannot start the port again. So it should be ok to free resources in close(). 2. Non-hotplugging applications can release resources if we implement finalization code to close(). Also I guess Stephen's suggestion is important to keep code clean. It may be good that 'dev->data->rx/tx_queues[queue_id]' are freed in rx/tx_queue_release() of each PMD, and rx/tx_queue_release() will be called by rte_eth_dev_close(). Also 'dev->data->rx/tx_queues[]' should be freed in ethdev.c. > 3. Combine close() and uninit(), only one will be left. > This will work fine for both hotplug case and normal case. But it > may change a lot, such as logic. I guess this will be second option. But I agree we need to change a lot. Also after enabling hotplug by default, when someone only wants to close the port, it will be impossible. Regards, Tetsuya
[dpdk-dev] [PATCH v2] librte_ether: release memory in uninit function.
> -Original Message- > From: Tetsuya Mukawa [mailto:mukawa at igel.co.jp] > Sent: Tuesday, July 7, 2015 4:38 AM > To: dev at dpdk.org > Cc: Qiu, Michael; Iremonger, Bernard; thomas.monjalon at 6wind.com; > Ananyev, Konstantin; Stephen Hemminger; Zhang, Helin; Chen, Jing D; Neil > Horman > Subject: Re: [dpdk-dev] [PATCH v2] librte_ether: release memory in uninit > function. > > On 2015/07/06 20:35, Qiu, Michael wrote: > > Hi, all > > > > As we has gap on the memory release action to be done in which step, I > > appreciate all your comments on this patch. > > > > Currently, the correct quit sequence for testpmd is stop() ---> > > port_stop() --> port_close() --> quit(). This will lead lots of memory > > not released by default, like queues. There is also the scenario (outside of testpmd) where an application can call rte_eth_dev_close() or rte_eth_dev_detach() which calls rte_eth_dev_uninit() directly from an application. In these cases the memory allocated in the EAL layer should be released in both rte_eth_dev_close() and rte_eth_dev_detach(). This patch is intended to address the rte_eth_dev_detach() case only (hotplug case). Perhaps there should be a separate patch to address the rte_eth_dev_close() case. Regards, Bernard. > > > > We have 3 potential proposals(normal case means without hotplug): > > > > 1. Those memory release in close() other left in uninit() > > This will work fine for both hotplug case and normal case. > > +1 > > I assume that once close() is called, we cannot start the port again without > hotplugging. > This is my premise. > > It might be good that we move finalization code to close() as much as > possible, because of followings. > 1. Anyway, once close() is called, we cannot start the port again. So it > should > be ok to free resources in close(). > 2. Non-hotplugging applications can release resources if we implement > finalization code to close(). > > Also I guess Stephen's suggestion is important to keep code clean. > It may be good that 'dev->data->rx/tx_queues[queue_id]' are freed in > rx/tx_queue_release() of each PMD, and rx/tx_queue_release() will be > called by rte_eth_dev_close(). Also 'dev->data->rx/tx_queues[]' should be > freed in ethdev.c. > > > 3. Combine close() and uninit(), only one will be left. > > This will work fine for both hotplug case and normal case. But it > > may change a lot, such as logic. > > I guess this will be second option. But I agree we need to change a lot. Also > after enabling hotplug by default, when someone only wants to close the > port, it will be impossible. > > Regards, > Tetsuya
[dpdk-dev] [PATCH v2] librte_ether: release memory in uninit function.
Hi, all
As we has gap on the memory release action to be done in which step, I
appreciate all your comments on this patch.
Currently, the correct quit sequence for testpmd is stop() --->
port_stop() --> port_close() --> quit(). This will lead lots of memory
not released by default, like queues.
We have 3 potential proposals(normal case means without hotplug):
1. Those memory release in close() other left in uninit()
This will work fine for both hotplug case and normal case.
2. leave close() just as the same before, all be done in uninit()
This will works fine for hotplug, for normal case maybe has
issue(memory not released?).
3. Combine close() and uninit(), only one will be left.
This will work fine for both hotplug case and normal case. But it
may change a lot, such as logic.
4. other.
For more details, you could go though this thread.
Thanks,
Michael
On 6/30/2015 9:32 AM, Qiu, Michael wrote:
> On 6/30/2015 12:42 AM, Iremonger, Bernard wrote:
>>> -Original Message-
>>> From: Qiu, Michael
>>> Sent: Monday, June 29, 2015 4:22 PM
>>> To: Iremonger, Bernard; dev at dpdk.org
>>> Cc: Zhang, Helin; Ananyev, Konstantin; mukawa at igel.co.jp; Stephen
>>> Hemminger
>>> Subject: Re: [PATCH v2] librte_ether: release memory in uninit function.
>>>
>>> On 2015/6/29 18:20, Iremonger, Bernard wrote:
> -Original Message-
> From: Qiu, Michael
> Sent: Monday, June 29, 2015 9:55 AM
> To: Iremonger, Bernard; dev at dpdk.org
> Cc: Zhang, Helin; Ananyev, Konstantin; mukawa at igel.co.jp; Stephen
> Hemminger
> Subject: Re: [PATCH v2] librte_ether: release memory in uninit function.
>
> On 6/26/2015 5:32 PM, Iremonger, Bernard wrote:
>> Changes in v2:
>> do not free mac_addrs and hash_mac_addrs here.
>>
>> Signed-off-by: Bernard Iremonger
>> ---
>> lib/librte_ether/rte_ethdev.c |6 +-
>> 1 files changed, 5 insertions(+), 1 deletions(-)
>>
>> diff --git a/lib/librte_ether/rte_ethdev.c
>> b/lib/librte_ether/rte_ethdev.c index e13fde5..7ae101a 100644
>> --- a/lib/librte_ether/rte_ethdev.c
>> +++ b/lib/librte_ether/rte_ethdev.c
>> @@ -369,8 +369,12 @@ rte_eth_dev_uninit(struct rte_pci_device
> *pci_dev)
>> /* free ether device */
>> rte_eth_dev_release_port(eth_dev);
>>
>> -if (rte_eal_process_type() == RTE_PROC_PRIMARY)
>> +if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
>> +rte_free(eth_dev->data->rx_queues);
>> +rte_free(eth_dev->data->tx_queues);
>> rte_free(eth_dev->data->dev_private);
>> +memset(eth_dev->data, 0, sizeof(struct
> rte_eth_dev_data));
>> +}
>>
>> eth_dev->pci_dev = NULL;
>> eth_dev->driver = NULL;
> Actually, This could be put in rte_eth_dev_close() becasue queues
> should be released when closed.
>
> Also before free dev->data->rx_queues you should make sure
> dev->data->rx_queues[i] has been freed in PMD close() function, So
> dev->data->this
> two should be better done at the same time, ether in
> rte_eth_dev_close() or in PMD close() function. For hotplug in fm10k,
> I put it in PMD close() function.
>
> Thanks,
> Michael
Hi Michael,
The consensus is that the rx_queue and tx_queue memory should not be
>>> released in the PMD as it is not allocated by the PMD. The memory is
>>> allocated in rte_eth_dev_rx_queue_config() and
>>> rte_eth_dev_tx_queue_config(), which are both called from
>>> rte_eth_dev_configure() which is called by the application (for example
>>> test_pmd). So it seems to make sense to free this memory in
>>> rte_eth_dev_uninit().
>>>
>>> It really make sense to free memory in rte_ether level, but when close a
>>> port
>>> with out detached? just as stop --> close() --> quit(), the memory will not
>>> be
>>> released :)
>>>
>> In the above scenario lots of memory will not be released.
>>
>> This is why the detach() and the underlying dev_uninit() functions were
>> introduced.
> First detach is only for hotplug, for *users do not use hotplug*, that
> scenario is the right action. So "lots of memory will not be released"
> is issue need be fixed, actually, in fm10k driver, lots of memory has
> been released.
>
>> The dev_uninit() functions currently call dev_close() which in turn calls
>> dev_stop() which calls dev_clear_queues().
> Users do hotplug then must call stop() --> close() --> dev_uninit(), it
> works fine. But do you think it make sense to release memory when
> close() it?
>
>> The dev_clear_queues() function does not release the queue_memory or the
>> queue array memory. The queue memory is now released in the dev_uninit() and
>> the queue array memory is released in the rte_eth_dev_uninit() function.
> That's your implementation, make sure not all users will detach a
> device, but the
[dpdk-dev] [PATCH v2] librte_ether: release memory in uninit function.
Changes in v2:
do not free mac_addrs and hash_mac_addrs here.
Signed-off-by: Bernard Iremonger
---
lib/librte_ether/rte_ethdev.c |6 +-
1 files changed, 5 insertions(+), 1 deletions(-)
diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index e13fde5..7ae101a 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -369,8 +369,12 @@ rte_eth_dev_uninit(struct rte_pci_device *pci_dev)
/* free ether device */
rte_eth_dev_release_port(eth_dev);
- if (rte_eal_process_type() == RTE_PROC_PRIMARY)
+ if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
+ rte_free(eth_dev->data->rx_queues);
+ rte_free(eth_dev->data->tx_queues);
rte_free(eth_dev->data->dev_private);
+ memset(eth_dev->data, 0, sizeof(struct rte_eth_dev_data));
+ }
eth_dev->pci_dev = NULL;
eth_dev->driver = NULL;
--
1.7.4.1
[dpdk-dev] [PATCH v2] librte_ether: release memory in uninit function.
> -Original Message-
> From: Qiu, Michael
> Sent: Monday, June 29, 2015 4:22 PM
> To: Iremonger, Bernard; dev at dpdk.org
> Cc: Zhang, Helin; Ananyev, Konstantin; mukawa at igel.co.jp; Stephen
> Hemminger
> Subject: Re: [PATCH v2] librte_ether: release memory in uninit function.
>
> On 2015/6/29 18:20, Iremonger, Bernard wrote:
> >
> >> -Original Message-
> >> From: Qiu, Michael
> >> Sent: Monday, June 29, 2015 9:55 AM
> >> To: Iremonger, Bernard; dev at dpdk.org
> >> Cc: Zhang, Helin; Ananyev, Konstantin; mukawa at igel.co.jp; Stephen
> >> Hemminger
> >> Subject: Re: [PATCH v2] librte_ether: release memory in uninit function.
> >>
> >> On 6/26/2015 5:32 PM, Iremonger, Bernard wrote:
> >>> Changes in v2:
> >>> do not free mac_addrs and hash_mac_addrs here.
> >>>
> >>> Signed-off-by: Bernard Iremonger
> >>> ---
> >>> lib/librte_ether/rte_ethdev.c |6 +-
> >>> 1 files changed, 5 insertions(+), 1 deletions(-)
> >>>
> >>> diff --git a/lib/librte_ether/rte_ethdev.c
> >>> b/lib/librte_ether/rte_ethdev.c index e13fde5..7ae101a 100644
> >>> --- a/lib/librte_ether/rte_ethdev.c
> >>> +++ b/lib/librte_ether/rte_ethdev.c
> >>> @@ -369,8 +369,12 @@ rte_eth_dev_uninit(struct rte_pci_device
> >> *pci_dev)
> >>> /* free ether device */
> >>> rte_eth_dev_release_port(eth_dev);
> >>>
> >>> - if (rte_eal_process_type() == RTE_PROC_PRIMARY)
> >>> + if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
> >>> + rte_free(eth_dev->data->rx_queues);
> >>> + rte_free(eth_dev->data->tx_queues);
> >>> rte_free(eth_dev->data->dev_private);
> >>> + memset(eth_dev->data, 0, sizeof(struct
> >> rte_eth_dev_data));
> >>> + }
> >>>
> >>> eth_dev->pci_dev = NULL;
> >>> eth_dev->driver = NULL;
> >>
> >> Actually, This could be put in rte_eth_dev_close() becasue queues
> >> should be released when closed.
> >>
> >> Also before free dev->data->rx_queues you should make sure
> >> dev->data->rx_queues[i] has been freed in PMD close() function, So
> >> dev->data->this
> >> two should be better done at the same time, ether in
> >> rte_eth_dev_close() or in PMD close() function. For hotplug in fm10k,
> >> I put it in PMD close() function.
> >>
> >> Thanks,
> >> Michael
> > Hi Michael,
> >
> > The consensus is that the rx_queue and tx_queue memory should not be
> released in the PMD as it is not allocated by the PMD. The memory is
> allocated in rte_eth_dev_rx_queue_config() and
> rte_eth_dev_tx_queue_config(), which are both called from
> rte_eth_dev_configure() which is called by the application (for example
> test_pmd). So it seems to make sense to free this memory in
> rte_eth_dev_uninit().
>
> It really make sense to free memory in rte_ether level, but when close a port
> with out detached? just as stop --> close() --> quit(), the memory will not be
> released :)
>
In the above scenario lots of memory will not be released.
This is why the detach() and the underlying dev_uninit() functions were
introduced.
The dev_uninit() functions currently call dev_close() which in turn calls
dev_stop() which calls dev_clear_queues().
The dev_clear_queues() function does not release the queue_memory or the queue
array memory. The queue memory is now released in the dev_uninit() and the
queue array memory is released in the rte_eth_dev_uninit() function.
If the queue array memory is released in rte_eth_dev_close() then the release
of the queue_memory will have to be moved to the dev_close() functions from the
dev_uninit() functions. This will impact all the existing PMD hotplug patches.
It will also change the existing dev_close() functionality.
My preference is to leave the existing dev_close() functions unchanged as far
as possible and to do what needs to be done in the dev_uninit() functions.
We probably need the view of the maintainers as to whether this should be done
in the close() or uninit() functions.
Regards,
Bernard.
[dpdk-dev] [PATCH v2] librte_ether: release memory in uninit function.
On 2015/6/29 18:20, Iremonger, Bernard wrote:
>
>> -Original Message-
>> From: Qiu, Michael
>> Sent: Monday, June 29, 2015 9:55 AM
>> To: Iremonger, Bernard; dev at dpdk.org
>> Cc: Zhang, Helin; Ananyev, Konstantin; mukawa at igel.co.jp; Stephen
>> Hemminger
>> Subject: Re: [PATCH v2] librte_ether: release memory in uninit function.
>>
>> On 6/26/2015 5:32 PM, Iremonger, Bernard wrote:
>>> Changes in v2:
>>> do not free mac_addrs and hash_mac_addrs here.
>>>
>>> Signed-off-by: Bernard Iremonger
>>> ---
>>> lib/librte_ether/rte_ethdev.c |6 +-
>>> 1 files changed, 5 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/lib/librte_ether/rte_ethdev.c
>>> b/lib/librte_ether/rte_ethdev.c index e13fde5..7ae101a 100644
>>> --- a/lib/librte_ether/rte_ethdev.c
>>> +++ b/lib/librte_ether/rte_ethdev.c
>>> @@ -369,8 +369,12 @@ rte_eth_dev_uninit(struct rte_pci_device
>> *pci_dev)
>>> /* free ether device */
>>> rte_eth_dev_release_port(eth_dev);
>>>
>>> - if (rte_eal_process_type() == RTE_PROC_PRIMARY)
>>> + if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
>>> + rte_free(eth_dev->data->rx_queues);
>>> + rte_free(eth_dev->data->tx_queues);
>>> rte_free(eth_dev->data->dev_private);
>>> + memset(eth_dev->data, 0, sizeof(struct
>> rte_eth_dev_data));
>>> + }
>>>
>>> eth_dev->pci_dev = NULL;
>>> eth_dev->driver = NULL;
>>
>> Actually, This could be put in rte_eth_dev_close() becasue queues should be
>> released when closed.
>>
>> Also before free dev->data->rx_queues you should make sure
>> dev->data->rx_queues[i] has been freed in PMD close() function, So this
>> two should be better done at the same time, ether in
>> rte_eth_dev_close() or in PMD close() function. For hotplug in fm10k, I put
>> it
>> in PMD close() function.
>>
>> Thanks,
>> Michael
> Hi Michael,
>
> The consensus is that the rx_queue and tx_queue memory should not be released
> in the PMD as it is not allocated by the PMD. The memory is allocated in
> rte_eth_dev_rx_queue_config() and rte_eth_dev_tx_queue_config(), which are
> both called from rte_eth_dev_configure() which is called by the application
> (for example test_pmd). So it seems to make sense to free this memory in
> rte_eth_dev_uninit().
It really make sense to free memory in rte_ether level, but when close a
port with out detached? just as stop --> close() --> quit(), the memory
will not be released :)
Thanks,
Michael
>
> Regards,
>
> Bernard.
>
>
>
>
>
[dpdk-dev] [PATCH v2] librte_ether: release memory in uninit function.
> -Original Message-
> From: Qiu, Michael
> Sent: Monday, June 29, 2015 9:55 AM
> To: Iremonger, Bernard; dev at dpdk.org
> Cc: Zhang, Helin; Ananyev, Konstantin; mukawa at igel.co.jp; Stephen
> Hemminger
> Subject: Re: [PATCH v2] librte_ether: release memory in uninit function.
>
> On 6/26/2015 5:32 PM, Iremonger, Bernard wrote:
> > Changes in v2:
> > do not free mac_addrs and hash_mac_addrs here.
> >
> > Signed-off-by: Bernard Iremonger
> > ---
> > lib/librte_ether/rte_ethdev.c |6 +-
> > 1 files changed, 5 insertions(+), 1 deletions(-)
> >
> > diff --git a/lib/librte_ether/rte_ethdev.c
> > b/lib/librte_ether/rte_ethdev.c index e13fde5..7ae101a 100644
> > --- a/lib/librte_ether/rte_ethdev.c
> > +++ b/lib/librte_ether/rte_ethdev.c
> > @@ -369,8 +369,12 @@ rte_eth_dev_uninit(struct rte_pci_device
> *pci_dev)
> > /* free ether device */
> > rte_eth_dev_release_port(eth_dev);
> >
> > - if (rte_eal_process_type() == RTE_PROC_PRIMARY)
> > + if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
> > + rte_free(eth_dev->data->rx_queues);
> > + rte_free(eth_dev->data->tx_queues);
> > rte_free(eth_dev->data->dev_private);
> > + memset(eth_dev->data, 0, sizeof(struct
> rte_eth_dev_data));
> > + }
> >
> > eth_dev->pci_dev = NULL;
> > eth_dev->driver = NULL;
>
>
> Actually, This could be put in rte_eth_dev_close() becasue queues should be
> released when closed.
>
> Also before free dev->data->rx_queues you should make sure
> dev->data->rx_queues[i] has been freed in PMD close() function, So this
> two should be better done at the same time, ether in
> rte_eth_dev_close() or in PMD close() function. For hotplug in fm10k, I put it
> in PMD close() function.
>
> Thanks,
> Michael
Hi Michael,
The consensus is that the rx_queue and tx_queue memory should not be released
in the PMD as it is not allocated by the PMD. The memory is allocated in
rte_eth_dev_rx_queue_config() and rte_eth_dev_tx_queue_config(), which are both
called from rte_eth_dev_configure() which is called by the application (for
example test_pmd). So it seems to make sense to free this memory in
rte_eth_dev_uninit().
Regards,
Bernard.
[dpdk-dev] [PATCH v2] librte_ether: release memory in uninit function.
On 6/26/2015 5:32 PM, Iremonger, Bernard wrote:
> Changes in v2:
> do not free mac_addrs and hash_mac_addrs here.
>
> Signed-off-by: Bernard Iremonger
> ---
> lib/librte_ether/rte_ethdev.c |6 +-
> 1 files changed, 5 insertions(+), 1 deletions(-)
>
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index e13fde5..7ae101a 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -369,8 +369,12 @@ rte_eth_dev_uninit(struct rte_pci_device *pci_dev)
> /* free ether device */
> rte_eth_dev_release_port(eth_dev);
>
> - if (rte_eal_process_type() == RTE_PROC_PRIMARY)
> + if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
> + rte_free(eth_dev->data->rx_queues);
> + rte_free(eth_dev->data->tx_queues);
> rte_free(eth_dev->data->dev_private);
> + memset(eth_dev->data, 0, sizeof(struct rte_eth_dev_data));
> + }
>
> eth_dev->pci_dev = NULL;
> eth_dev->driver = NULL;
Actually, This could be put in rte_eth_dev_close() becasue queues should
be released when closed.
Also before free dev->data->rx_queues you should make sure
dev->data->rx_queues[i] has been freed in PMD close() function, So this
two should be better done at the same time, ether in
rte_eth_dev_close() or in PMD close() function. For hotplug in fm10k, I
put it in PMD close() function.
Thanks,
Michael
[dpdk-dev] [PATCH v2] librte_ether: release memory in uninit function.
Changes in v2:
do not free mac_addrs and hash_mac_addrs here.
Signed-off-by: Bernard Iremonger
---
lib/librte_ether/rte_ethdev.c |6 +-
1 files changed, 5 insertions(+), 1 deletions(-)
diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index e13fde5..7ae101a 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -369,8 +369,12 @@ rte_eth_dev_uninit(struct rte_pci_device *pci_dev)
/* free ether device */
rte_eth_dev_release_port(eth_dev);
- if (rte_eal_process_type() == RTE_PROC_PRIMARY)
+ if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
+ rte_free(eth_dev->data->rx_queues);
+ rte_free(eth_dev->data->tx_queues);
rte_free(eth_dev->data->dev_private);
+ memset(eth_dev->data, 0, sizeof(struct rte_eth_dev_data));
+ }
eth_dev->pci_dev = NULL;
eth_dev->driver = NULL;
--
1.7.4.1
[dpdk-dev] [PATCH v2] librte_ether: release memory in uninit function.
> -Original Message-
> From: Iremonger, Bernard
> Sent: Friday, June 26, 2015 10:33 AM
> To: dev at dpdk.org
> Cc: Zhang, Helin; Ananyev, Konstantin; Qiu, Michael; mukawa at igel.co.jp;
> Iremonger, Bernard
> Subject: [PATCH v2] librte_ether: release memory in uninit function.
>
> Changes in v2:
> do not free mac_addrs and hash_mac_addrs here.
>
> Signed-off-by: Bernard Iremonger
> ---
> lib/librte_ether/rte_ethdev.c |6 +-
> 1 files changed, 5 insertions(+), 1 deletions(-)
>
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index e13fde5..7ae101a 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -369,8 +369,12 @@ rte_eth_dev_uninit(struct rte_pci_device *pci_dev)
> /* free ether device */
> rte_eth_dev_release_port(eth_dev);
>
> - if (rte_eal_process_type() == RTE_PROC_PRIMARY)
> + if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
> + rte_free(eth_dev->data->rx_queues);
> + rte_free(eth_dev->data->tx_queues);
> rte_free(eth_dev->data->dev_private);
> + memset(eth_dev->data, 0, sizeof(struct rte_eth_dev_data));
> + }
>
> eth_dev->pci_dev = NULL;
> eth_dev->driver = NULL;
> --
Acked-by: Konstantin Ananyev
> 1.7.4.1
