On 2015/6/29 19:08, Ananyev, Konstantin wrote: > Hi Michael, > >> -----Original Message----- >> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Qiu, Michael >> Sent: Monday, June 29, 2015 10:21 AM >> To: Iremonger, Bernard; dev at dpdk.org >> Cc: He, Shaopeng >> Subject: Re: [dpdk-dev] [PATCH 1/2 v4] fm10k: Free queues when close port >> >> On 6/29/2015 4:57 PM, Iremonger, Bernard wrote: >>>> -----Original Message----- >>>> From: Qiu, Michael >>>> Sent: Monday, June 29, 2015 9:17 AM >>>> To: Iremonger, Bernard; dev at dpdk.org >>>> Cc: Chen, Jing D; He, Shaopeng >>>> Subject: Re: [PATCH 1/2 v4] fm10k: Free queues when close port >>>> >>>> On 6/26/2015 7:02 PM, Iremonger, Bernard wrote: >>>>>> -----Original Message----- >>>>>> From: Qiu, Michael >>>>>> Sent: Friday, June 26, 2015 9:30 AM >>>>>> To: dev at dpdk.org >>>>>> Cc: Chen, Jing D; He, Shaopeng; Iremonger, Bernard; Qiu, Michael >>>>>> Subject: [PATCH 1/2 v4] fm10k: Free queues when close port >>>>>> >>>>>> When close a port, lots of memory should be released, such as >>>>>> software rings, queues, etc. >>>>>> >>>>>> Signed-off-by: Michael Qiu <michael.qiu at intel.com> >>>>>> --- >>>>> Hi Michael, >>>>> >>>>> There are 2 comments inline >>>>> >>>>>> drivers/net/fm10k/fm10k_ethdev.c | 37 >>>>>> +++++++++++++++++++++++++++++++++---- >>>>>> 1 file changed, 33 insertions(+), 4 deletions(-) >>>>>> >>>>>> diff --git a/drivers/net/fm10k/fm10k_ethdev.c >>>>>> b/drivers/net/fm10k/fm10k_ethdev.c >>>>>> index 406c350..eba7228 100644 >>>>>> --- a/drivers/net/fm10k/fm10k_ethdev.c >>>>>> +++ b/drivers/net/fm10k/fm10k_ethdev.c >>>>>> @@ -65,6 +65,8 @@ static void >>>>>> fm10k_MAC_filter_set(struct rte_eth_dev *dev, const u8 *mac, bool >>>>>> add); static void fm10k_MACVLAN_remove_all(struct rte_eth_dev >>>> *dev); >>>>>> +static void fm10k_tx_queue_release(void *queue); static void >>>>>> +fm10k_rx_queue_release(void *queue); >>>>>> >>>>>> static void >>>>>> fm10k_mbx_initlock(struct fm10k_hw *hw) @@ -809,11 +811,37 @@ >>>>>> fm10k_dev_stop(struct rte_eth_dev *dev) >>>>>> >>>>>> PMD_INIT_FUNC_TRACE(); >>>>>> >>>>>> - for (i = 0; i < dev->data->nb_tx_queues; i++) >>>>>> - fm10k_dev_tx_queue_stop(dev, i); >>>>>> + if (dev->data->tx_queues) >>>>>> + for (i = 0; i < dev->data->nb_tx_queues; i++) >>>>>> + fm10k_dev_tx_queue_stop(dev, i); >>>>>> >>>>>> - for (i = 0; i < dev->data->nb_rx_queues; i++) >>>>>> - fm10k_dev_rx_queue_stop(dev, i); >>>>>> + if (dev->data->rx_queues) >>>>>> + for (i = 0; i < dev->data->nb_rx_queues; i++) >>>>>> + fm10k_dev_rx_queue_stop(dev, i); >>>>>> +} >>>>>> + >>>>>> +static void >>>>>> +fm10k_dev_queue_release(struct rte_eth_dev *dev) { >>>>>> + int i; >>>>>> + >>>>>> + PMD_INIT_FUNC_TRACE(); >>>>>> + >>>>>> + if (dev->data->tx_queues) { >>>>>> + for (i = 0; i < dev->data->nb_tx_queues; i++) >>>>>> + fm10k_tx_queue_release(dev->data- >>>>>>> tx_queues[i]); >>>>>> + rte_free(dev->data->tx_queues); >>>>>> + dev->data->tx_queues = NULL; >>>>> The memory for dev->data->tx_queues is not allocated in the fm10k >>>>> PMD, so it should probably not be released here. >>>>> I have submitted a patch today for rte_eth_dev.c to do this. >>>>> /dev/patchwork/patch/5829/ >>>>> >>>>>> + dev->data->nb_tx_queues = 0; >>>>>> + } >>>>>> + >>>>>> + if (dev->data->rx_queues) { >>>>>> + for (i = 0; i < dev->data->nb_rx_queues; i++) >>>>>> + fm10k_rx_queue_release(dev->data- >>>>>>> rx_queues[i]); >>>>>> + rte_free(dev->data->rx_queues); >>>>>> + dev->data->rx_queues = NULL; >>>>> The memory for dev->data->rx_queues is not allocated in the fm10k >>>>> PMD, so it should probably not be released here. >>>>> I have submitted a patch today for rte_eth_dev.c to do this. >>>>> /dev/patchwork/patch/5829/ >>>> Is it a good idea? What about to close the port for twice at a time? >>>> I think it is better to do it in rte_eth_dev_close(), I will give the >>>> comments to >>>> you. >>>> >>>> Thanks, >>>> Michael >>> Hi Michael, >>> Could you take a look at the comments on >>> http://dpdk.org/dev/patchwork/patch/5829/ >> Hi, Bernard >> >> I have give comments on it. >> >>> The consensus is that memory should be freed in the component that >>> allocated it. >>> In my pmd hotplug patches I have used a flag to ensure that dev_close is >>> not called twice. >>> In the e1000 patch I have added a stopped flag to struct e1000_adapter. >>> http://dpdk.org/dev/patchwork/patch/5655/ >> >> >> I reviewed your patch about ixgbe and fvl before. But forget e1000. >> >> In my logic, when dev->data->rx_queues is NULL, that means this device >> has been closed before. What else, we even do not care about whether it >> has been closed or not, when close() function be called, all memory >> should be freed if exist am I right? >> >> So, check dev->data->rx_queues whether it is NULL will be recommend in >> close function, only this could avoid unsafe situations for pointer. > > It seems you are mixing 2 things there: > Contents of dev->data->rx_queues[] is mamanged by each PMD, and yes should > be alloced and freed by each PMD. > dev->data->rx_queues[] itself is allocated/reallocated and should be freed by > rte_ethdev layer. > PMD, in theory, simly doesn't know how it was allocated.
I really do not mix these, what I mean is when you try to release the dev->data->rx_queues you must confirm two things: 1. dev->data->rx_queues is not NULL 2. dev->data->rx_queues[i] is already freed Also I agree dev->data->rx_queues is better freed in rte_ethdev layer. And I will remove free dev->data->rx_queues memory in PMD, and based on bernard's new patch. Also in PMD, to check dev->data->rx_queues to see if could release dev->data->rx_queues[i] when close() Thanks, Michael > Konstantin > > >> Thanks, >> Michael >>> Regards, >>> >>> Bernard. >>> >>> <snip> >>> >>> >>> >