| Message ID | 20181211160224.22181-1-olaf@aepfle.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1] xen_disk: fix memory leak | expand |
Patchew URL: https://patchew.org/QEMU/20181211160224.22181-1-olaf@aepfle.de/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Message-id: 20181211160224.22181-1-olaf@aepfle.de
Subject: [Qemu-devel] [PATCH v1] xen_disk: fix memory leak
Type: series
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
aaf3499 xen_disk: fix memory leak
=== OUTPUT BEGIN ===
Checking PATCH 1/1: xen_disk: fix memory leak...
ERROR: braces {} are necessary for all arms of this statement
#26: FILE: hw/block/xen_disk.c:108:
+ if (ioreq->buf)
[...]
ERROR: braces {} are necessary for all arms of this statement
#32: FILE: hw/block/xen_disk.c:114:
+ if (ioreq->buf)
[...]
total: 2 errors, 0 warnings, 55 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
The full log is available at
http://patchew.org/logs/20181211160224.22181-1-olaf@aepfle.de/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
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 <olaf@aepfle.de> 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,
> -----Original Message----- > From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org] On Behalf > Of Anthony PERARD > Sent: 13 December 2018 11:34 > To: Olaf Hering <olaf@aepfle.de> > Cc: Kevin Wolf <kwolf@redhat.com>; Stefano Stabellini > <sstabellini@kernel.org>; open list:Block layer core <qemu- > block@nongnu.org>; qemu-devel@nongnu.org; Max Reitz <mreitz@redhat.com>; > open list:X86 <xen-devel@lists.xenproject.org> > 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 <olaf@aepfle.de> > > 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
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) +{ + if (ioreq->buf) + qemu_vfree(ioreq->buf); + 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; +} static void ioreq_reset(struct ioreq *ioreq) { + ioreq_buf_free(ioreq); memset(&ioreq->req, 0, sizeof(ioreq->req)); ioreq->status = 0; ioreq->start = 0; - ioreq->buf = NULL; ioreq->size = 0; ioreq->presync = 0; @@ -315,14 +327,14 @@ static void qemu_aio_complete(void *opaque, int ret) if (ret == 0) { ioreq_grant_copy(ioreq); } - qemu_vfree(ioreq->buf); + ioreq_buf_free(ioreq); break; case BLKIF_OP_WRITE: case BLKIF_OP_FLUSH_DISKCACHE: if (!ioreq->req.nr_segments) { break; } - qemu_vfree(ioreq->buf); + ioreq_buf_free(ioreq); break; default: break; @@ -390,12 +402,12 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq) { struct XenBlkDev *blkdev = ioreq->blkdev; - ioreq->buf = qemu_memalign(XC_PAGE_SIZE, ioreq->size); + ioreq_buf_alloc(ioreq, XC_PAGE_SIZE); if (ioreq->req.nr_segments && (ioreq->req.operation == BLKIF_OP_WRITE || ioreq->req.operation == BLKIF_OP_FLUSH_DISKCACHE) && ioreq_grant_copy(ioreq)) { - qemu_vfree(ioreq->buf); + ioreq_buf_free(ioreq); goto err; }
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 ioreq_reset, which might clobber ioreq->buf as well. Add wrappers to free ioreq->buf before reassignment. Signed-off-by: Olaf Hering <olaf@aepfle.de> --- hw/block/xen_disk.c | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-)