Re: [PATCH 6/7] vhost-user-gpu: fix memory leak in 'virgl_resource_attach_backing'

["Marc-André Lureau" <marcandre.lureau@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2457 bytes --]

Hi

On Wed, May 5, 2021 at 1:24 PM Li Qiang <liq3ea@gmail.com> wrote:

> Marc-André Lureau <marcandre.lureau@gmail.com> 于2021年5月5日周三 下午5:08写道：
> >
> > Hi
> >
> > On Wed, May 5, 2021 at 12:03 PM P J P <ppandit@redhat.com> wrote:
> >>
> >> +-- On Tue, 4 May 2021, Li Qiang wrote --+
> >> | diff --git a/contrib/vhost-user-gpu/virgl.c
> b/contrib/vhost-user-gpu/virgl.c
> >> | index c669d73a1d..a16a311d80 100644
> >> | --- a/contrib/vhost-user-gpu/virgl.c
> >> | +++ b/contrib/vhost-user-gpu/virgl.c
> >> | @@ -287,8 +287,11 @@ virgl_resource_attach_backing(VuGpu *g,
> >> |          return;
> >> |      }
> >> |
> >> | -    virgl_renderer_resource_attach_iov(att_rb.resource_id,
> >> | +    ret = virgl_renderer_resource_attach_iov(att_rb.resource_id,
> >> |                                         res_iovs, att_rb.nr_entries);
> >> | +    if (ret != 0) {
> >> | +        g_free(res_iovs);
> >> | +    }
> >> |  }
> >>
> >> * Similar to earlier,
> >>   hw/display/virtio-gpu-3d.c:virgl_resource_attach_backing() calls
> >>   'virtio_gpu_cleanup_mapping_iov'
> >>
> >> * should it be same for vhost-user-gpu?
> >>
> >>
> >
> > Good question, that's also what you did when you fixed it for virtio-gpu
> in:
> >
> > commit 33243031dad02d161225ba99d782616da133f689
> > Author: Li Qiang <liq3ea@gmail.com>
> > Date:   Thu Dec 29 03:11:26 2016 -0500
> >
> >     virtio-gpu-3d: fix memory leak in resource attach backing
> >
>
> Do you mean this;
> -->https://lists.gnu.org/archive/html/qemu-devel/2021-05/msg01156.html
>
> I think there is no need for this.
>
> The virtio_gpu_cleanup_mapping_iov is needed in virtio-gpu is because
> it need map guest memory.
> But in vhost-user-gpu case, the 'vg_create_mapping_iov' calls
> 'vu_gpa_to_va' and this function don't need
> do map memory.
>
> But for the beauty of symmetry we can abstract a function called
> 'vg_destroy_mapping_iov' and it just calls g_free(res->iov).
> Like the pair 'virtio_gpu_create_mapping_iov' and
> 'virtio_gpu_cleanup_mapping'.
>
>
Right. I think I like the suggestion to add a 'virtio_gpu_cleanup_mapping'
(with a comment to explain it) to avoid this kind of question again. Feel
free to add a patch on top if you want.

thanks


>
> >
> > Btw, for each patch, it would be nice to have a reference to the
> original fix in virtio-gpu.
> >
> > Thanks!
> >
>


-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 3752 bytes --]