[PATCH 2/2] virtio-iommu: delete vqs in unrealize to fix memleaks

Pan Nengyuan posted 2 patches 5 years, 10 months ago
Maintainers: Eric Auger <eric.auger@redhat.com>, Max Reitz <mreitz@redhat.com>, Kevin Wolf <kwolf@redhat.com>, "Michael S. Tsirkin" <mst@redhat.com>, Stefan Hajnoczi <stefanha@redhat.com>
There is a newer version of this series
[PATCH 2/2] virtio-iommu: delete vqs in unrealize to fix memleaks
Posted by Pan Nengyuan 5 years, 10 months ago
req_vq/event_vq forgot to free in unrealize(). Fix that.

Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
---
Cc: Eric Auger <eric.auger@redhat.com>
---
 hw/virtio/virtio-iommu.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
index 4cee8083bc..9d2ff0693c 100644
--- a/hw/virtio/virtio-iommu.c
+++ b/hw/virtio/virtio-iommu.c
@@ -696,6 +696,8 @@ static void virtio_iommu_device_unrealize(DeviceState *dev, Error **errp)
     g_tree_destroy(s->domains);
     g_tree_destroy(s->endpoints);
 
+    virtio_delete_queue(s->req_vq);
+    virtio_delete_queue(s->event_vq);
     virtio_cleanup(vdev);
 }
 
-- 
2.18.2


Re: [PATCH 2/2] virtio-iommu: delete vqs in unrealize to fix memleaks
Posted by Auger Eric 5 years, 10 months ago
Hi Pan,

On 3/27/20 4:56 AM, Pan Nengyuan wrote:
> req_vq/event_vq forgot to free in unrealize(). Fix that.
> 
> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
> ---
> Cc: Eric Auger <eric.auger@redhat.com>
> ---
>  hw/virtio/virtio-iommu.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
> index 4cee8083bc..9d2ff0693c 100644
> --- a/hw/virtio/virtio-iommu.c
> +++ b/hw/virtio/virtio-iommu.c
> @@ -696,6 +696,8 @@ static void virtio_iommu_device_unrealize(DeviceState *dev, Error **errp)
>      g_tree_destroy(s->domains);
>      g_tree_destroy(s->endpoints);
>  
> +    virtio_delete_queue(s->req_vq);
> +    virtio_delete_queue(s->event_vq);
>      virtio_cleanup(vdev);
>  }
>  
> 

thanks for fixing this.

Acked-by: Eric Auger <eric.auger@redhat.com>

Eric


Re: [PATCH 2/2] virtio-iommu: delete vqs in unrealize to fix memleaks
Posted by Stefano Garzarella 5 years, 10 months ago
On Fri, Mar 27, 2020 at 11:56:50AM +0800, Pan Nengyuan wrote:
> req_vq/event_vq forgot to free in unrealize(). Fix that.
> 
> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
> ---
> Cc: Eric Auger <eric.auger@redhat.com>
> ---
>  hw/virtio/virtio-iommu.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
> index 4cee8083bc..9d2ff0693c 100644
> --- a/hw/virtio/virtio-iommu.c
> +++ b/hw/virtio/virtio-iommu.c
> @@ -696,6 +696,8 @@ static void virtio_iommu_device_unrealize(DeviceState *dev, Error **errp)
>      g_tree_destroy(s->domains);
>      g_tree_destroy(s->endpoints);
>  
> +    virtio_delete_queue(s->req_vq);
> +    virtio_delete_queue(s->event_vq);
>      virtio_cleanup(vdev);
>  }

Hi Pan,
thanks for this fix.

Since we are here, should we also clean 's->as_by_busptr' hash table?

Maybe adding this in the unrealize:

    g_hash_table_destroy(s->as_by_busptr);

Thanks,
Stefano


Re: [PATCH 2/2] virtio-iommu: delete vqs in unrealize to fix memleaks
Posted by Pan Nengyuan 5 years, 10 months ago

On 3/27/2020 4:53 PM, Stefano Garzarella wrote:
> On Fri, Mar 27, 2020 at 11:56:50AM +0800, Pan Nengyuan wrote:
>> req_vq/event_vq forgot to free in unrealize(). Fix that.
>>
>> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
>> ---
>> Cc: Eric Auger <eric.auger@redhat.com>
>> ---
>>  hw/virtio/virtio-iommu.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
>> index 4cee8083bc..9d2ff0693c 100644
>> --- a/hw/virtio/virtio-iommu.c
>> +++ b/hw/virtio/virtio-iommu.c
>> @@ -696,6 +696,8 @@ static void virtio_iommu_device_unrealize(DeviceState *dev, Error **errp)
>>      g_tree_destroy(s->domains);
>>      g_tree_destroy(s->endpoints);
>>  
>> +    virtio_delete_queue(s->req_vq);
>> +    virtio_delete_queue(s->event_vq);
>>      virtio_cleanup(vdev);
>>  }
> 
> Hi Pan,
> thanks for this fix.
> 
> Since we are here, should we also clean 's->as_by_busptr' hash table?
> 
> Maybe adding this in the unrealize:
> 
>     g_hash_table_destroy(s->as_by_busptr);

Yes, you are right. I will add it.

Thanks.

> 
> Thanks,
> Stefano
>