Re: [Xen-devel] [PATCH v1] xen_disk: fix memory leak

[Paul Durrant]

> -Original Message-
> From: Xen-devel [mailto:xen-devel-boun...@lists.xenproject.org] On Behalf
> Of Anthony PERARD
> Sent: 13 December 2018 11:34
> To: Olaf Hering 
> Cc: Kevin Wolf ; Stefano Stabellini
> ; open list:Block layer core  bl...@nongnu.org>; qemu-de...@nongnu.org; Max Reitz ;
> open list:X86 
> Subject: Re: [Xen-devel] [PATCH v1] xen_disk: fix memory leak
> 
> On Tue, Dec 11, 2018 at 05:02:24PM +0100, Olaf Hering wrote:
> > There are some code paths that clobber ioreq->buf, which leads to a huge
> > memory leak after a few hours of runtime. One code path is
> > qemu_aio_complete, which might be called recursive. Another one is
> 
> I think it's s/recursive/recursively/.
> 
> > ioreq_reset, which might clobber ioreq->buf as well.
> >
> > Add wrappers to free ioreq->buf before reassignment.
> >
> > Signed-off-by: Olaf Hering 
> 
> That patch seems fine, with a few coding style issues, and is going to
> be needed to be forward ported to Paul's reimplementation (not yet
> merged).

I already posted a patch from Tim Smith (re-based to the new xen-block 
datapath) that should fix this issue.

  Paul

> 
> > ---
> >  hw/block/xen_disk.c | 22 +-
> >  1 file changed, 17 insertions(+), 5 deletions(-)
> >
> > diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
> > index 36eff94f84..e15eefe625 100644
> > --- a/hw/block/xen_disk.c
> > +++ b/hw/block/xen_disk.c
> > @@ -103,12 +103,24 @@ struct XenBlkDev {
> >
> >  /* - */
> >
> > +static void ioreq_buf_alloc(struct ioreq *ioreq, size_t alignment)
> 
> You have the parameter `alignment` but don't actually use it, I don't
> think it's needed.
> 
> > +{
> > +if (ioreq->buf)
> > +qemu_vfree(ioreq->buf);
> 
> You could call ioreq_buf_free here instead of duplicating the code.
> 
> > +ioreq->buf = qemu_memalign(XC_PAGE_SIZE, ioreq->size);
> > +}
> > +static void ioreq_buf_free(struct ioreq *ioreq)
> > +{
> > +if (ioreq->buf)
> > +qemu_vfree(ioreq->buf);
> > +ioreq->buf = NULL;
> > +}
> 
> Thanks,
> 
> --
> Anthony PERARD
> 
> ___
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v1] xen_disk: fix memory leak

[Anthony PERARD]

On Tue, Dec 11, 2018 at 05:02:24PM +0100, Olaf Hering wrote:
> There are some code paths that clobber ioreq->buf, which leads to a huge
> memory leak after a few hours of runtime. One code path is
> qemu_aio_complete, which might be called recursive. Another one is

I think it's s/recursive/recursively/.

> ioreq_reset, which might clobber ioreq->buf as well.
> 
> Add wrappers to free ioreq->buf before reassignment.
> 
> Signed-off-by: Olaf Hering 

That patch seems fine, with a few coding style issues, and is going to
be needed to be forward ported to Paul's reimplementation (not yet
merged).

> ---
>  hw/block/xen_disk.c | 22 +-
>  1 file changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
> index 36eff94f84..e15eefe625 100644
> --- a/hw/block/xen_disk.c
> +++ b/hw/block/xen_disk.c
> @@ -103,12 +103,24 @@ struct XenBlkDev {
>  
>  /* - */
>  
> +static void ioreq_buf_alloc(struct ioreq *ioreq, size_t alignment)

You have the parameter `alignment` but don't actually use it, I don't
think it's needed.

> +{
> +if (ioreq->buf)
> +qemu_vfree(ioreq->buf);

You could call ioreq_buf_free here instead of duplicating the code.

> +ioreq->buf = qemu_memalign(XC_PAGE_SIZE, ioreq->size);
> +}
> +static void ioreq_buf_free(struct ioreq *ioreq)
> +{
> +if (ioreq->buf)
> +qemu_vfree(ioreq->buf);
> +ioreq->buf = NULL;
> +}

Thanks,

-- 
Anthony PERARD

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel