DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] net/vhost: fix potential memory leak
@ 2020-03-03  0:03 Itsuro Oda
  2020-03-05  2:15 ` Ye Xiaolong
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Itsuro Oda @ 2020-03-03  0:03 UTC (permalink / raw)
  To: maxime.coquelin, tiwei.bie, zhihong.wang, anatoly.burakov, dev

If a vhost device is closed before eth_dev_configure is done
to the device, internal resources allocated to the device
does not freed. This patch fixes it.

Fixes: 3d01b759d267 ("net/vhost: delay driver setup")

Signed-off-by: Itsuro Oda <oda@valinux.co.jp>
---
 drivers/net/vhost/rte_eth_vhost.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
index 458ed58f5..1ed977e9b 100644
--- a/drivers/net/vhost/rte_eth_vhost.c
+++ b/drivers/net/vhost/rte_eth_vhost.c
@@ -1065,16 +1065,14 @@ eth_dev_close(struct rte_eth_dev *dev)
 
 	eth_dev_stop(dev);
 
-	rte_vhost_driver_unregister(internal->iface_name);
-
 	list = find_internal_resource(internal->iface_name);
-	if (!list)
-		return;
-
-	pthread_mutex_lock(&internal_list_lock);
-	TAILQ_REMOVE(&internal_list, list, next);
-	pthread_mutex_unlock(&internal_list_lock);
-	rte_free(list);
+	if (list) {
+		rte_vhost_driver_unregister(internal->iface_name);
+		pthread_mutex_lock(&internal_list_lock);
+		TAILQ_REMOVE(&internal_list, list, next);
+		pthread_mutex_unlock(&internal_list_lock);
+		rte_free(list);
+	}
 
 	if (dev->data->rx_queues)
 		for (i = 0; i < dev->data->nb_rx_queues; i++)
-- 
2.17.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH] net/vhost: fix potential memory leak
  2020-03-03  0:03 [dpdk-dev] [PATCH] net/vhost: fix potential memory leak Itsuro Oda
@ 2020-03-05  2:15 ` Ye Xiaolong
  2020-03-05  2:50 ` [dpdk-dev] [PATCH v2] " Itsuro Oda
  2020-03-05  2:54 ` [dpdk-dev] [PATCH v3] " Itsuro Oda
  2 siblings, 0 replies; 8+ messages in thread
From: Ye Xiaolong @ 2020-03-05  2:15 UTC (permalink / raw)
  To: Itsuro Oda; +Cc: maxime.coquelin, tiwei.bie, zhihong.wang, anatoly.burakov, dev

On 03/03, Itsuro Oda wrote:
>If a vhost device is closed before eth_dev_configure is done
>to the device, internal resources allocated to the device
>does not freed. This patch fixes it.

s/does not freed/would not be freed

>
>Fixes: 3d01b759d267 ("net/vhost: delay driver setup")

Cc: stable@dpdk.org

>
>Signed-off-by: Itsuro Oda <oda@valinux.co.jp>
>---
> drivers/net/vhost/rte_eth_vhost.c | 16 +++++++---------
> 1 file changed, 7 insertions(+), 9 deletions(-)
>
>diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
>index 458ed58f5..1ed977e9b 100644
>--- a/drivers/net/vhost/rte_eth_vhost.c
>+++ b/drivers/net/vhost/rte_eth_vhost.c
>@@ -1065,16 +1065,14 @@ eth_dev_close(struct rte_eth_dev *dev)
> 
> 	eth_dev_stop(dev);
> 
>-	rte_vhost_driver_unregister(internal->iface_name);
>-
> 	list = find_internal_resource(internal->iface_name);
>-	if (!list)
>-		return;
>-
>-	pthread_mutex_lock(&internal_list_lock);
>-	TAILQ_REMOVE(&internal_list, list, next);
>-	pthread_mutex_unlock(&internal_list_lock);
>-	rte_free(list);
>+	if (list) {
>+		rte_vhost_driver_unregister(internal->iface_name);
>+		pthread_mutex_lock(&internal_list_lock);
>+		TAILQ_REMOVE(&internal_list, list, next);
>+		pthread_mutex_unlock(&internal_list_lock);
>+		rte_free(list);
>+	}
> 
> 	if (dev->data->rx_queues)
> 		for (i = 0; i < dev->data->nb_rx_queues; i++)
>-- 
>2.17.0
>

For the rest,

Reviewed-by: Xiaolong Ye <xiaolong.ye@intel.com>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [dpdk-dev] [PATCH v2] net/vhost: fix potential memory leak
  2020-03-03  0:03 [dpdk-dev] [PATCH] net/vhost: fix potential memory leak Itsuro Oda
  2020-03-05  2:15 ` Ye Xiaolong
@ 2020-03-05  2:50 ` Itsuro Oda
  2020-03-05  2:54 ` [dpdk-dev] [PATCH v3] " Itsuro Oda
  2 siblings, 0 replies; 8+ messages in thread
From: Itsuro Oda @ 2020-03-05  2:50 UTC (permalink / raw)
  To: maxime.coquelin, tiwei.bie, zhihong.wang, anatoly.burakov, dev, stable

If a vhost device is closed before eth_dev_configure is done
to the device, internal resources allocated to the device
would not be freed. This patch fixes it.

Fixes: 3d01b759d267 ("net/vhost: delay driver setup")
Cc: stable@dpdk.org

Signed-off-by: Itsuro Oda <oda@valinux.co.jp>
Reviewd-by: Xiaolong Ye <xiaolong.ye@intel.com>
---
v2:
- fix commit message

 drivers/net/vhost/rte_eth_vhost.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
index 458ed58f5..1ed977e9b 100644
--- a/drivers/net/vhost/rte_eth_vhost.c
+++ b/drivers/net/vhost/rte_eth_vhost.c
@@ -1065,16 +1065,14 @@ eth_dev_close(struct rte_eth_dev *dev)
 
 	eth_dev_stop(dev);
 
-	rte_vhost_driver_unregister(internal->iface_name);
-
 	list = find_internal_resource(internal->iface_name);
-	if (!list)
-		return;
-
-	pthread_mutex_lock(&internal_list_lock);
-	TAILQ_REMOVE(&internal_list, list, next);
-	pthread_mutex_unlock(&internal_list_lock);
-	rte_free(list);
+	if (list) {
+		rte_vhost_driver_unregister(internal->iface_name);
+		pthread_mutex_lock(&internal_list_lock);
+		TAILQ_REMOVE(&internal_list, list, next);
+		pthread_mutex_unlock(&internal_list_lock);
+		rte_free(list);
+	}
 
 	if (dev->data->rx_queues)
 		for (i = 0; i < dev->data->nb_rx_queues; i++)
-- 
2.17.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [dpdk-dev] [PATCH v3] net/vhost: fix potential memory leak
  2020-03-03  0:03 [dpdk-dev] [PATCH] net/vhost: fix potential memory leak Itsuro Oda
  2020-03-05  2:15 ` Ye Xiaolong
  2020-03-05  2:50 ` [dpdk-dev] [PATCH v2] " Itsuro Oda
@ 2020-03-05  2:54 ` Itsuro Oda
  2020-03-19 17:15   ` Kevin Traynor
  2020-04-10 14:44   ` Maxime Coquelin
  2 siblings, 2 replies; 8+ messages in thread
From: Itsuro Oda @ 2020-03-05  2:54 UTC (permalink / raw)
  To: maxime.coquelin, tiwei.bie, zhihong.wang, anatoly.burakov, dev, stable

If a vhost device is closed before eth_dev_configure is done
to the device, internal resources allocated to the device
would not be freed. This patch fixes it.

Fixes: 3d01b759d267 ("net/vhost: delay driver setup")
Cc: stable@dpdk.org

Signed-off-by: Itsuro Oda <oda@valinux.co.jp>
Reviewed-by: Xiaolong Ye <xiaolong.ye@intel.com>
---
v2:
- fix commit message

v3:
- fix spell error of Reviewed-by

 drivers/net/vhost/rte_eth_vhost.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
index 458ed58f5..1ed977e9b 100644
--- a/drivers/net/vhost/rte_eth_vhost.c
+++ b/drivers/net/vhost/rte_eth_vhost.c
@@ -1065,16 +1065,14 @@ eth_dev_close(struct rte_eth_dev *dev)
 
 	eth_dev_stop(dev);
 
-	rte_vhost_driver_unregister(internal->iface_name);
-
 	list = find_internal_resource(internal->iface_name);
-	if (!list)
-		return;
-
-	pthread_mutex_lock(&internal_list_lock);
-	TAILQ_REMOVE(&internal_list, list, next);
-	pthread_mutex_unlock(&internal_list_lock);
-	rte_free(list);
+	if (list) {
+		rte_vhost_driver_unregister(internal->iface_name);
+		pthread_mutex_lock(&internal_list_lock);
+		TAILQ_REMOVE(&internal_list, list, next);
+		pthread_mutex_unlock(&internal_list_lock);
+		rte_free(list);
+	}
 
 	if (dev->data->rx_queues)
 		for (i = 0; i < dev->data->nb_rx_queues; i++)
-- 
2.17.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH v3] net/vhost: fix potential memory leak
  2020-03-05  2:54 ` [dpdk-dev] [PATCH v3] " Itsuro Oda
@ 2020-03-19 17:15   ` Kevin Traynor
  2020-03-20 16:23     ` Maxime Coquelin
  2020-04-10 14:44   ` Maxime Coquelin
  1 sibling, 1 reply; 8+ messages in thread
From: Kevin Traynor @ 2020-03-19 17:15 UTC (permalink / raw)
  To: Itsuro Oda, maxime.coquelin, tiwei.bie, zhihong.wang,
	anatoly.burakov, dev, stable
  Cc: Xiaolong Ye

Hi Itsuro,

On 05/03/2020 02:54, Itsuro Oda wrote:
> If a vhost device is closed before eth_dev_configure is done
> to the device, internal resources allocated to the device
> would not be freed. This patch fixes it.
> 
> Fixes: 3d01b759d267 ("net/vhost: delay driver setup")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Itsuro Oda <oda@valinux.co.jp>
> Reviewed-by: Xiaolong Ye <xiaolong.ye@intel.com>

This fixes an issue with the patch you backported for 18.11. Is the
issue also present in the backported version?

If so, this patch is not in upstream dpdk or gone through validation. So
the choices are,

1. revert your patches from 18.11
2. go ahead on stable without this patch
3. delay until this patch is in master (but not until validated) and
then backport to stable

Itsuro/Maxime, what do you think?

thanks,
Kevin.

> ---
> v2:
> - fix commit message
> 
> v3:
> - fix spell error of Reviewed-by
> 
>  drivers/net/vhost/rte_eth_vhost.c | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
> index 458ed58f5..1ed977e9b 100644
> --- a/drivers/net/vhost/rte_eth_vhost.c
> +++ b/drivers/net/vhost/rte_eth_vhost.c
> @@ -1065,16 +1065,14 @@ eth_dev_close(struct rte_eth_dev *dev)
>  
>  	eth_dev_stop(dev);
>  
> -	rte_vhost_driver_unregister(internal->iface_name);
> -
>  	list = find_internal_resource(internal->iface_name);
> -	if (!list)
> -		return;
> -
> -	pthread_mutex_lock(&internal_list_lock);
> -	TAILQ_REMOVE(&internal_list, list, next);
> -	pthread_mutex_unlock(&internal_list_lock);
> -	rte_free(list);
> +	if (list) {
> +		rte_vhost_driver_unregister(internal->iface_name);
> +		pthread_mutex_lock(&internal_list_lock);
> +		TAILQ_REMOVE(&internal_list, list, next);
> +		pthread_mutex_unlock(&internal_list_lock);
> +		rte_free(list);
> +	}
>  
>  	if (dev->data->rx_queues)
>  		for (i = 0; i < dev->data->nb_rx_queues; i++)
> 


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH v3] net/vhost: fix potential memory leak
  2020-03-19 17:15   ` Kevin Traynor
@ 2020-03-20 16:23     ` Maxime Coquelin
  2020-03-20 19:25       ` Kevin Traynor
  0 siblings, 1 reply; 8+ messages in thread
From: Maxime Coquelin @ 2020-03-20 16:23 UTC (permalink / raw)
  To: Kevin Traynor, Itsuro Oda, tiwei.bie, zhihong.wang,
	anatoly.burakov, dev, stable
  Cc: Xiaolong Ye



On 3/19/20 6:15 PM, Kevin Traynor wrote:
> Hi Itsuro,
> 
> On 05/03/2020 02:54, Itsuro Oda wrote:
>> If a vhost device is closed before eth_dev_configure is done
>> to the device, internal resources allocated to the device
>> would not be freed. This patch fixes it.
>>
>> Fixes: 3d01b759d267 ("net/vhost: delay driver setup")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Itsuro Oda <oda@valinux.co.jp>
>> Reviewed-by: Xiaolong Ye <xiaolong.ye@intel.com>
> 
> This fixes an issue with the patch you backported for 18.11. Is the
> issue also present in the backported version?
> 
> If so, this patch is not in upstream dpdk or gone through validation. So
> the choices are,
> 
> 1. revert your patches from 18.11
> 2. go ahead on stable without this patch
> 3. delay until this patch is in master (but not until validated) and
> then backport to stable
> 
> Itsuro/Maxime, what do you think?

I think you should drop Itsuro patches for now, as long as this patch is
not in master. Secondary process was broken for several revisions in
Vhost, so it is better to keep it broken for now than risking
regressions on primary process support.

If this patch is in master before the next 18.11 is done, then we can
pick all the patches.

Thanks,
Maxime
> thanks,
> Kevin.
> 
>> ---
>> v2:
>> - fix commit message
>>
>> v3:
>> - fix spell error of Reviewed-by
>>
>>  drivers/net/vhost/rte_eth_vhost.c | 16 +++++++---------
>>  1 file changed, 7 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
>> index 458ed58f5..1ed977e9b 100644
>> --- a/drivers/net/vhost/rte_eth_vhost.c
>> +++ b/drivers/net/vhost/rte_eth_vhost.c
>> @@ -1065,16 +1065,14 @@ eth_dev_close(struct rte_eth_dev *dev)
>>  
>>  	eth_dev_stop(dev);
>>  
>> -	rte_vhost_driver_unregister(internal->iface_name);
>> -
>>  	list = find_internal_resource(internal->iface_name);
>> -	if (!list)
>> -		return;
>> -
>> -	pthread_mutex_lock(&internal_list_lock);
>> -	TAILQ_REMOVE(&internal_list, list, next);
>> -	pthread_mutex_unlock(&internal_list_lock);
>> -	rte_free(list);
>> +	if (list) {
>> +		rte_vhost_driver_unregister(internal->iface_name);
>> +		pthread_mutex_lock(&internal_list_lock);
>> +		TAILQ_REMOVE(&internal_list, list, next);
>> +		pthread_mutex_unlock(&internal_list_lock);
>> +		rte_free(list);
>> +	}
>>  
>>  	if (dev->data->rx_queues)
>>  		for (i = 0; i < dev->data->nb_rx_queues; i++)
>>
> 


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH v3] net/vhost: fix potential memory leak
  2020-03-20 16:23     ` Maxime Coquelin
@ 2020-03-20 19:25       ` Kevin Traynor
  0 siblings, 0 replies; 8+ messages in thread
From: Kevin Traynor @ 2020-03-20 19:25 UTC (permalink / raw)
  To: Maxime Coquelin, Itsuro Oda, tiwei.bie, zhihong.wang,
	anatoly.burakov, dev, stable
  Cc: Xiaolong Ye

On 20/03/2020 16:23, Maxime Coquelin wrote:
> 
> 
> On 3/19/20 6:15 PM, Kevin Traynor wrote:
>> Hi Itsuro,
>>
>> On 05/03/2020 02:54, Itsuro Oda wrote:
>>> If a vhost device is closed before eth_dev_configure is done
>>> to the device, internal resources allocated to the device
>>> would not be freed. This patch fixes it.
>>>
>>> Fixes: 3d01b759d267 ("net/vhost: delay driver setup")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Itsuro Oda <oda@valinux.co.jp>
>>> Reviewed-by: Xiaolong Ye <xiaolong.ye@intel.com>
>>
>> This fixes an issue with the patch you backported for 18.11. Is the
>> issue also present in the backported version?
>>
>> If so, this patch is not in upstream dpdk or gone through validation. So
>> the choices are,
>>
>> 1. revert your patches from 18.11
>> 2. go ahead on stable without this patch
>> 3. delay until this patch is in master (but not until validated) and
>> then backport to stable
>>
>> Itsuro/Maxime, what do you think?
> 
> I think you should drop Itsuro patches for now, as long as this patch is
> not in master. Secondary process was broken for several revisions in
> Vhost, so it is better to keep it broken for now than risking
> regressions on primary process support.
> 

I agree, vhost primary is too well used to take risks on regression
while there are still some outstanding issues being resolved with these
patches for secondary process.

Patches reverted.

> If this patch is in master before the next 18.11 is done, then we can
> pick all the patches.
> 

Yes, we can do that.

thanks,
Kevin.

> Thanks,
> Maxime
>> thanks,
>> Kevin.
>>
>>> ---
>>> v2:
>>> - fix commit message
>>>
>>> v3:
>>> - fix spell error of Reviewed-by
>>>
>>>  drivers/net/vhost/rte_eth_vhost.c | 16 +++++++---------
>>>  1 file changed, 7 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
>>> index 458ed58f5..1ed977e9b 100644
>>> --- a/drivers/net/vhost/rte_eth_vhost.c
>>> +++ b/drivers/net/vhost/rte_eth_vhost.c
>>> @@ -1065,16 +1065,14 @@ eth_dev_close(struct rte_eth_dev *dev)
>>>  
>>>  	eth_dev_stop(dev);
>>>  
>>> -	rte_vhost_driver_unregister(internal->iface_name);
>>> -
>>>  	list = find_internal_resource(internal->iface_name);
>>> -	if (!list)
>>> -		return;
>>> -
>>> -	pthread_mutex_lock(&internal_list_lock);
>>> -	TAILQ_REMOVE(&internal_list, list, next);
>>> -	pthread_mutex_unlock(&internal_list_lock);
>>> -	rte_free(list);
>>> +	if (list) {
>>> +		rte_vhost_driver_unregister(internal->iface_name);
>>> +		pthread_mutex_lock(&internal_list_lock);
>>> +		TAILQ_REMOVE(&internal_list, list, next);
>>> +		pthread_mutex_unlock(&internal_list_lock);
>>> +		rte_free(list);
>>> +	}
>>>  
>>>  	if (dev->data->rx_queues)
>>>  		for (i = 0; i < dev->data->nb_rx_queues; i++)
>>>
>>


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH v3] net/vhost: fix potential memory leak
  2020-03-05  2:54 ` [dpdk-dev] [PATCH v3] " Itsuro Oda
  2020-03-19 17:15   ` Kevin Traynor
@ 2020-04-10 14:44   ` Maxime Coquelin
  1 sibling, 0 replies; 8+ messages in thread
From: Maxime Coquelin @ 2020-04-10 14:44 UTC (permalink / raw)
  To: Itsuro Oda, tiwei.bie, zhihong.wang, anatoly.burakov, dev, stable



On 3/5/20 3:54 AM, Itsuro Oda wrote:
> If a vhost device is closed before eth_dev_configure is done
> to the device, internal resources allocated to the device
> would not be freed. This patch fixes it.
> 
> Fixes: 3d01b759d267 ("net/vhost: delay driver setup")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Itsuro Oda <oda@valinux.co.jp>
> Reviewed-by: Xiaolong Ye <xiaolong.ye@intel.com>
> ---
> v2:
> - fix commit message
> 
> v3:
> - fix spell error of Reviewed-by
> 
>  drivers/net/vhost/rte_eth_vhost.c | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)

Applied to dpdk-next-virtio/master.

Thanks,
Maxime


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2020-04-10 14:44 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-03  0:03 [dpdk-dev] [PATCH] net/vhost: fix potential memory leak Itsuro Oda
2020-03-05  2:15 ` Ye Xiaolong
2020-03-05  2:50 ` [dpdk-dev] [PATCH v2] " Itsuro Oda
2020-03-05  2:54 ` [dpdk-dev] [PATCH v3] " Itsuro Oda
2020-03-19 17:15   ` Kevin Traynor
2020-03-20 16:23     ` Maxime Coquelin
2020-03-20 19:25       ` Kevin Traynor
2020-04-10 14:44   ` Maxime Coquelin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).