* [PATCH 1/5] block/nvme: Automatically free qemu_memalign() with QEMU_AUTO_VFREE
2021-10-06 16:49 [PATCH 0/5] block/nvme: Fix a memory leak in nvme_free_queue_pair() Philippe Mathieu-Daudé
@ 2021-10-06 16:49 ` Philippe Mathieu-Daudé
2021-10-07 13:29 ` Stefan Hajnoczi
2021-10-06 16:49 ` [PATCH 2/5] block/nvme: Display CQ/SQ pointer in nvme_free_queue_pair() Philippe Mathieu-Daudé
` (3 subsequent siblings)
4 siblings, 1 reply; 17+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-10-06 16:49 UTC (permalink / raw)
To: qemu-devel
Cc: Fam Zheng, Kevin Wolf, qemu-block, Max Reitz, Hanna Reitz,
Stefan Hajnoczi, Philippe Mathieu-Daudé
Since commit 4d324c0bf65 ("introduce QEMU_AUTO_VFREE") buffers
allocated by qemu_memalign() can automatically freed when using
the QEMU_AUTO_VFREE macro. Use it to simplify a bit.
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
block/nvme.c | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/block/nvme.c b/block/nvme.c
index 1cc7b62bb4b..fefcc04abe6 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -514,10 +514,10 @@ static bool nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
{
BDRVNVMeState *s = bs->opaque;
bool ret = false;
- union {
+ QEMU_AUTO_VFREE union {
NvmeIdCtrl ctrl;
NvmeIdNs ns;
- } *id;
+ } *id = NULL;
NvmeLBAF *lbaf;
uint16_t oncs;
int r;
@@ -595,7 +595,6 @@ static bool nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
s->blkshift = lbaf->ds;
out:
qemu_vfio_dma_unmap(s->vfio, id);
- qemu_vfree(id);
return ret;
}
@@ -1219,7 +1218,7 @@ static int nvme_co_prw(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
{
BDRVNVMeState *s = bs->opaque;
int r;
- uint8_t *buf = NULL;
+ QEMU_AUTO_VFREE uint8_t *buf = NULL;
QEMUIOVector local_qiov;
size_t len = QEMU_ALIGN_UP(bytes, qemu_real_host_page_size);
assert(QEMU_IS_ALIGNED(offset, s->page_size));
@@ -1246,7 +1245,6 @@ static int nvme_co_prw(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
if (!r && !is_write) {
qemu_iovec_from_buf(qiov, 0, buf, bytes);
}
- qemu_vfree(buf);
return r;
}
@@ -1365,7 +1363,7 @@ static int coroutine_fn nvme_co_pdiscard(BlockDriverState *bs,
BDRVNVMeState *s = bs->opaque;
NVMeQueuePair *ioq = s->queues[INDEX_IO(0)];
NVMeRequest *req;
- NvmeDsmRange *buf;
+ QEMU_AUTO_VFREE NvmeDsmRange *buf = NULL;
QEMUIOVector local_qiov;
int ret;
@@ -1440,7 +1438,6 @@ static int coroutine_fn nvme_co_pdiscard(BlockDriverState *bs,
trace_nvme_dsm_done(s, offset, bytes, ret);
out:
qemu_iovec_destroy(&local_qiov);
- qemu_vfree(buf);
return ret;
}
--
2.31.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 1/5] block/nvme: Automatically free qemu_memalign() with QEMU_AUTO_VFREE
2021-10-06 16:49 ` [PATCH 1/5] block/nvme: Automatically free qemu_memalign() with QEMU_AUTO_VFREE Philippe Mathieu-Daudé
@ 2021-10-07 13:29 ` Stefan Hajnoczi
0 siblings, 0 replies; 17+ messages in thread
From: Stefan Hajnoczi @ 2021-10-07 13:29 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Fam Zheng, Kevin Wolf, qemu-block, qemu-devel, Max Reitz, Hanna Reitz
[-- Attachment #1: Type: text/plain, Size: 474 bytes --]
On Wed, Oct 06, 2021 at 06:49:27PM +0200, Philippe Mathieu-Daudé wrote:
> Since commit 4d324c0bf65 ("introduce QEMU_AUTO_VFREE") buffers
> allocated by qemu_memalign() can automatically freed when using
> the QEMU_AUTO_VFREE macro. Use it to simplify a bit.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> block/nvme.c | 11 ++++-------
> 1 file changed, 4 insertions(+), 7 deletions(-)
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 2/5] block/nvme: Display CQ/SQ pointer in nvme_free_queue_pair()
2021-10-06 16:49 [PATCH 0/5] block/nvme: Fix a memory leak in nvme_free_queue_pair() Philippe Mathieu-Daudé
2021-10-06 16:49 ` [PATCH 1/5] block/nvme: Automatically free qemu_memalign() with QEMU_AUTO_VFREE Philippe Mathieu-Daudé
@ 2021-10-06 16:49 ` Philippe Mathieu-Daudé
2021-10-07 13:29 ` Stefan Hajnoczi
2021-10-06 16:49 ` [PATCH 3/5] block/nvme: Extract nvme_free_queue() from nvme_free_queue_pair() Philippe Mathieu-Daudé
` (2 subsequent siblings)
4 siblings, 1 reply; 17+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-10-06 16:49 UTC (permalink / raw)
To: qemu-devel
Cc: Fam Zheng, Kevin Wolf, qemu-block, Max Reitz, Hanna Reitz,
Stefan Hajnoczi, Philippe Mathieu-Daudé
For debugging purpose it is helpful to know the CQ/SQ pointers.
We already have a trace event in nvme_free_queue_pair(), extend
it to report these pointer addresses.
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
block/nvme.c | 2 +-
block/trace-events | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/block/nvme.c b/block/nvme.c
index fefcc04abe6..0c94799a541 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -185,7 +185,7 @@ static bool nvme_init_queue(BDRVNVMeState *s, NVMeQueue *q,
static void nvme_free_queue_pair(NVMeQueuePair *q)
{
- trace_nvme_free_queue_pair(q->index, q);
+ trace_nvme_free_queue_pair(q->index, q, &q->cq, &q->sq);
if (q->completion_bh) {
qemu_bh_delete(q->completion_bh);
}
diff --git a/block/trace-events b/block/trace-events
index f2d0a9b62a7..491e765c611 100644
--- a/block/trace-events
+++ b/block/trace-events
@@ -157,7 +157,7 @@ nvme_dsm_done(void *s, int64_t offset, int64_t bytes, int ret) "s %p offset 0x%"
nvme_dma_map_flush(void *s) "s %p"
nvme_free_req_queue_wait(void *s, unsigned q_index) "s %p q #%u"
nvme_create_queue_pair(unsigned q_index, void *q, size_t size, void *aio_context, int fd) "index %u q %p size %zu aioctx %p fd %d"
-nvme_free_queue_pair(unsigned q_index, void *q) "index %u q %p"
+nvme_free_queue_pair(unsigned q_index, void *q, void *cq, void *sq) "index %u q %p cq %p sq %p"
nvme_cmd_map_qiov(void *s, void *cmd, void *req, void *qiov, int entries) "s %p cmd %p req %p qiov %p entries %d"
nvme_cmd_map_qiov_pages(void *s, int i, uint64_t page) "s %p page[%d] 0x%"PRIx64
nvme_cmd_map_qiov_iov(void *s, int i, void *page, int pages) "s %p iov[%d] %p pages %d"
--
2.31.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 2/5] block/nvme: Display CQ/SQ pointer in nvme_free_queue_pair()
2021-10-06 16:49 ` [PATCH 2/5] block/nvme: Display CQ/SQ pointer in nvme_free_queue_pair() Philippe Mathieu-Daudé
@ 2021-10-07 13:29 ` Stefan Hajnoczi
0 siblings, 0 replies; 17+ messages in thread
From: Stefan Hajnoczi @ 2021-10-07 13:29 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Fam Zheng, Kevin Wolf, qemu-block, qemu-devel, Max Reitz, Hanna Reitz
[-- Attachment #1: Type: text/plain, Size: 487 bytes --]
On Wed, Oct 06, 2021 at 06:49:28PM +0200, Philippe Mathieu-Daudé wrote:
> For debugging purpose it is helpful to know the CQ/SQ pointers.
> We already have a trace event in nvme_free_queue_pair(), extend
> it to report these pointer addresses.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> block/nvme.c | 2 +-
> block/trace-events | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 3/5] block/nvme: Extract nvme_free_queue() from nvme_free_queue_pair()
2021-10-06 16:49 [PATCH 0/5] block/nvme: Fix a memory leak in nvme_free_queue_pair() Philippe Mathieu-Daudé
2021-10-06 16:49 ` [PATCH 1/5] block/nvme: Automatically free qemu_memalign() with QEMU_AUTO_VFREE Philippe Mathieu-Daudé
2021-10-06 16:49 ` [PATCH 2/5] block/nvme: Display CQ/SQ pointer in nvme_free_queue_pair() Philippe Mathieu-Daudé
@ 2021-10-06 16:49 ` Philippe Mathieu-Daudé
2021-10-07 13:29 ` Stefan Hajnoczi
2021-10-06 16:49 ` [PATCH 4/5] block/nvme: Pass BDRVNVMeState* handle to nvme_free_queue_pair() Philippe Mathieu-Daudé
2021-10-06 16:49 ` [PATCH 5/5] block/nvme: Fix memory leak from nvme_init_queue() Philippe Mathieu-Daudé
4 siblings, 1 reply; 17+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-10-06 16:49 UTC (permalink / raw)
To: qemu-devel
Cc: Fam Zheng, Kevin Wolf, qemu-block, Max Reitz, Hanna Reitz,
Stefan Hajnoczi, Philippe Mathieu-Daudé
Instead of duplicating code, extract the common helper to free
a single queue.
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
block/nvme.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/block/nvme.c b/block/nvme.c
index 0c94799a541..e4f336d79c2 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -183,15 +183,20 @@ static bool nvme_init_queue(BDRVNVMeState *s, NVMeQueue *q,
return r == 0;
}
+static void nvme_free_queue(NVMeQueue *q)
+{
+ qemu_vfree(q->queue);
+}
+
static void nvme_free_queue_pair(NVMeQueuePair *q)
{
trace_nvme_free_queue_pair(q->index, q, &q->cq, &q->sq);
if (q->completion_bh) {
qemu_bh_delete(q->completion_bh);
}
+ nvme_free_queue(&q->sq);
+ nvme_free_queue(&q->cq);
qemu_vfree(q->prp_list_pages);
- qemu_vfree(q->sq.queue);
- qemu_vfree(q->cq.queue);
qemu_mutex_destroy(&q->lock);
g_free(q);
}
--
2.31.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 3/5] block/nvme: Extract nvme_free_queue() from nvme_free_queue_pair()
2021-10-06 16:49 ` [PATCH 3/5] block/nvme: Extract nvme_free_queue() from nvme_free_queue_pair() Philippe Mathieu-Daudé
@ 2021-10-07 13:29 ` Stefan Hajnoczi
0 siblings, 0 replies; 17+ messages in thread
From: Stefan Hajnoczi @ 2021-10-07 13:29 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Fam Zheng, Kevin Wolf, qemu-block, qemu-devel, Max Reitz, Hanna Reitz
[-- Attachment #1: Type: text/plain, Size: 367 bytes --]
On Wed, Oct 06, 2021 at 06:49:29PM +0200, Philippe Mathieu-Daudé wrote:
> Instead of duplicating code, extract the common helper to free
> a single queue.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> block/nvme.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 4/5] block/nvme: Pass BDRVNVMeState* handle to nvme_free_queue_pair()
2021-10-06 16:49 [PATCH 0/5] block/nvme: Fix a memory leak in nvme_free_queue_pair() Philippe Mathieu-Daudé
` (2 preceding siblings ...)
2021-10-06 16:49 ` [PATCH 3/5] block/nvme: Extract nvme_free_queue() from nvme_free_queue_pair() Philippe Mathieu-Daudé
@ 2021-10-06 16:49 ` Philippe Mathieu-Daudé
2021-10-07 13:30 ` Stefan Hajnoczi
2021-10-06 16:49 ` [PATCH 5/5] block/nvme: Fix memory leak from nvme_init_queue() Philippe Mathieu-Daudé
4 siblings, 1 reply; 17+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-10-06 16:49 UTC (permalink / raw)
To: qemu-devel
Cc: Fam Zheng, Kevin Wolf, qemu-block, Max Reitz, Hanna Reitz,
Stefan Hajnoczi, Philippe Mathieu-Daudé
In the next commit we want to access BDRVNVMeState from
nvme_free_queue_pair() callee. Pass it along first.
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
block/nvme.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/block/nvme.c b/block/nvme.c
index e4f336d79c2..6e476f54b9f 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -183,19 +183,19 @@ static bool nvme_init_queue(BDRVNVMeState *s, NVMeQueue *q,
return r == 0;
}
-static void nvme_free_queue(NVMeQueue *q)
+static void nvme_free_queue(BDRVNVMeState *s, NVMeQueue *q)
{
qemu_vfree(q->queue);
}
-static void nvme_free_queue_pair(NVMeQueuePair *q)
+static void nvme_free_queue_pair(BDRVNVMeState *s, NVMeQueuePair *q)
{
trace_nvme_free_queue_pair(q->index, q, &q->cq, &q->sq);
if (q->completion_bh) {
qemu_bh_delete(q->completion_bh);
}
- nvme_free_queue(&q->sq);
- nvme_free_queue(&q->cq);
+ nvme_free_queue(s, &q->sq);
+ nvme_free_queue(s, &q->cq);
qemu_vfree(q->prp_list_pages);
qemu_mutex_destroy(&q->lock);
g_free(q);
@@ -270,7 +270,7 @@ static NVMeQueuePair *nvme_create_queue_pair(BDRVNVMeState *s,
return q;
fail:
- nvme_free_queue_pair(q);
+ nvme_free_queue_pair(s, q);
return NULL;
}
@@ -693,7 +693,7 @@ static bool nvme_add_io_queue(BlockDriverState *bs, Error **errp)
s->queue_count++;
return true;
out_error:
- nvme_free_queue_pair(q);
+ nvme_free_queue_pair(s, q);
return false;
}
@@ -918,7 +918,7 @@ static void nvme_close(BlockDriverState *bs)
BDRVNVMeState *s = bs->opaque;
for (unsigned i = 0; i < s->queue_count; ++i) {
- nvme_free_queue_pair(s->queues[i]);
+ nvme_free_queue_pair(s, s->queues[i]);
}
g_free(s->queues);
aio_set_event_notifier(bdrv_get_aio_context(bs),
--
2.31.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 4/5] block/nvme: Pass BDRVNVMeState* handle to nvme_free_queue_pair()
2021-10-06 16:49 ` [PATCH 4/5] block/nvme: Pass BDRVNVMeState* handle to nvme_free_queue_pair() Philippe Mathieu-Daudé
@ 2021-10-07 13:30 ` Stefan Hajnoczi
0 siblings, 0 replies; 17+ messages in thread
From: Stefan Hajnoczi @ 2021-10-07 13:30 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Fam Zheng, Kevin Wolf, qemu-block, qemu-devel, Max Reitz, Hanna Reitz
[-- Attachment #1: Type: text/plain, Size: 402 bytes --]
On Wed, Oct 06, 2021 at 06:49:30PM +0200, Philippe Mathieu-Daudé wrote:
> In the next commit we want to access BDRVNVMeState from
> nvme_free_queue_pair() callee. Pass it along first.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> block/nvme.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 5/5] block/nvme: Fix memory leak from nvme_init_queue()
2021-10-06 16:49 [PATCH 0/5] block/nvme: Fix a memory leak in nvme_free_queue_pair() Philippe Mathieu-Daudé
` (3 preceding siblings ...)
2021-10-06 16:49 ` [PATCH 4/5] block/nvme: Pass BDRVNVMeState* handle to nvme_free_queue_pair() Philippe Mathieu-Daudé
@ 2021-10-06 16:49 ` Philippe Mathieu-Daudé
2021-10-06 16:58 ` Philippe Mathieu-Daudé
2021-10-07 13:29 ` Stefan Hajnoczi
4 siblings, 2 replies; 17+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-10-06 16:49 UTC (permalink / raw)
To: qemu-devel
Cc: Fam Zheng, Kevin Wolf, qemu-block, Max Reitz, Hanna Reitz,
Stefan Hajnoczi, Philippe Mathieu-Daudé
nvme_create_queue_pair() allocates resources with qemu_vfio_dma_map(),
but we never release them. Do it in nvme_free_queue() which is called
from nvme_free_queue_pair().
Reported by valgrind:
==252858== 520,192 bytes in 1 blocks are still reachable in loss record 8,293 of 8,302
==252858== at 0x4846803: memalign (vg_replace_malloc.c:1265)
==252858== by 0x484691F: posix_memalign (vg_replace_malloc.c:1429)
==252858== by 0xB8AFE4: qemu_try_memalign (oslib-posix.c:210)
==252858== by 0xA9E315: nvme_create_queue_pair (nvme.c:229)
==252858== by 0xAA0125: nvme_init (nvme.c:799)
==252858== by 0xAA081C: nvme_file_open (nvme.c:953)
==252858== by 0xA23DDD: bdrv_open_driver (block.c:1550)
==252858== by 0xA24806: bdrv_open_common (block.c:1827)
==252858== by 0xA2889B: bdrv_open_inherit (block.c:3747)
==252858== by 0xA28DE4: bdrv_open (block.c:3840)
==252858== by 0x9E0F8E: bds_tree_init (blockdev.c:675)
==252858== by 0x9E7C74: qmp_blockdev_add (blockdev.c:3551)
Fixes: bdd6a90a9e5 ("block: Add VFIO based NVMe driver")
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
block/nvme.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/block/nvme.c b/block/nvme.c
index 6e476f54b9f..903c8ffa060 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -185,6 +185,7 @@ static bool nvme_init_queue(BDRVNVMeState *s, NVMeQueue *q,
static void nvme_free_queue(BDRVNVMeState *s, NVMeQueue *q)
{
+ qemu_vfio_dma_unmap(s->vfio, q->queue);
qemu_vfree(q->queue);
}
--
2.31.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 5/5] block/nvme: Fix memory leak from nvme_init_queue()
2021-10-06 16:49 ` [PATCH 5/5] block/nvme: Fix memory leak from nvme_init_queue() Philippe Mathieu-Daudé
@ 2021-10-06 16:58 ` Philippe Mathieu-Daudé
2021-10-07 13:29 ` Stefan Hajnoczi
1 sibling, 0 replies; 17+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-10-06 16:58 UTC (permalink / raw)
To: qemu-devel
Cc: Fam Zheng, Kevin Wolf, qemu-block, Max Reitz, Hanna Reitz,
Stefan Hajnoczi
On 10/6/21 18:49, Philippe Mathieu-Daudé wrote:
> nvme_create_queue_pair() allocates resources with qemu_vfio_dma_map(),
> but we never release them. Do it in nvme_free_queue() which is called
> from nvme_free_queue_pair().
>
> Reported by valgrind:
>
> ==252858== 520,192 bytes in 1 blocks are still reachable in loss record 8,293 of 8,302
BTW the "8302" number is kinda depressing...
Good news, with this patch I'm now at 8301.
> ==252858== at 0x4846803: memalign (vg_replace_malloc.c:1265)
> ==252858== by 0x484691F: posix_memalign (vg_replace_malloc.c:1429)
> ==252858== by 0xB8AFE4: qemu_try_memalign (oslib-posix.c:210)
> ==252858== by 0xA9E315: nvme_create_queue_pair (nvme.c:229)
> ==252858== by 0xAA0125: nvme_init (nvme.c:799)
> ==252858== by 0xAA081C: nvme_file_open (nvme.c:953)
> ==252858== by 0xA23DDD: bdrv_open_driver (block.c:1550)
> ==252858== by 0xA24806: bdrv_open_common (block.c:1827)
> ==252858== by 0xA2889B: bdrv_open_inherit (block.c:3747)
> ==252858== by 0xA28DE4: bdrv_open (block.c:3840)
> ==252858== by 0x9E0F8E: bds_tree_init (blockdev.c:675)
> ==252858== by 0x9E7C74: qmp_blockdev_add (blockdev.c:3551)
>
> Fixes: bdd6a90a9e5 ("block: Add VFIO based NVMe driver")
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> block/nvme.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/block/nvme.c b/block/nvme.c
> index 6e476f54b9f..903c8ffa060 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -185,6 +185,7 @@ static bool nvme_init_queue(BDRVNVMeState *s, NVMeQueue *q,
>
> static void nvme_free_queue(BDRVNVMeState *s, NVMeQueue *q)
> {
> + qemu_vfio_dma_unmap(s->vfio, q->queue);
> qemu_vfree(q->queue);
> }
>
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 5/5] block/nvme: Fix memory leak from nvme_init_queue()
2021-10-06 16:49 ` [PATCH 5/5] block/nvme: Fix memory leak from nvme_init_queue() Philippe Mathieu-Daudé
2021-10-06 16:58 ` Philippe Mathieu-Daudé
@ 2021-10-07 13:29 ` Stefan Hajnoczi
2021-10-07 13:34 ` Philippe Mathieu-Daudé
1 sibling, 1 reply; 17+ messages in thread
From: Stefan Hajnoczi @ 2021-10-07 13:29 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Fam Zheng, Kevin Wolf, qemu-block, qemu-devel, Max Reitz, Hanna Reitz
[-- Attachment #1: Type: text/plain, Size: 1918 bytes --]
On Wed, Oct 06, 2021 at 06:49:31PM +0200, Philippe Mathieu-Daudé wrote:
> nvme_create_queue_pair() allocates resources with qemu_vfio_dma_map(),
> but we never release them. Do it in nvme_free_queue() which is called
> from nvme_free_queue_pair().
>
> Reported by valgrind:
>
> ==252858== 520,192 bytes in 1 blocks are still reachable in loss record 8,293 of 8,302
> ==252858== at 0x4846803: memalign (vg_replace_malloc.c:1265)
> ==252858== by 0x484691F: posix_memalign (vg_replace_malloc.c:1429)
> ==252858== by 0xB8AFE4: qemu_try_memalign (oslib-posix.c:210)
> ==252858== by 0xA9E315: nvme_create_queue_pair (nvme.c:229)
> ==252858== by 0xAA0125: nvme_init (nvme.c:799)
> ==252858== by 0xAA081C: nvme_file_open (nvme.c:953)
> ==252858== by 0xA23DDD: bdrv_open_driver (block.c:1550)
> ==252858== by 0xA24806: bdrv_open_common (block.c:1827)
> ==252858== by 0xA2889B: bdrv_open_inherit (block.c:3747)
> ==252858== by 0xA28DE4: bdrv_open (block.c:3840)
> ==252858== by 0x9E0F8E: bds_tree_init (blockdev.c:675)
> ==252858== by 0x9E7C74: qmp_blockdev_add (blockdev.c:3551)
>
> Fixes: bdd6a90a9e5 ("block: Add VFIO based NVMe driver")
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> block/nvme.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/block/nvme.c b/block/nvme.c
> index 6e476f54b9f..903c8ffa060 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -185,6 +185,7 @@ static bool nvme_init_queue(BDRVNVMeState *s, NVMeQueue *q,
>
> static void nvme_free_queue(BDRVNVMeState *s, NVMeQueue *q)
> {
> + qemu_vfio_dma_unmap(s->vfio, q->queue);
> qemu_vfree(q->queue);
> }
I can't figure out the issue. qemu_vfree(q->queue) was already called
before this patch. How does adding qemu_vfio_dma_unmap() help with the
valgrind report in the commit description?
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 5/5] block/nvme: Fix memory leak from nvme_init_queue()
2021-10-07 13:29 ` Stefan Hajnoczi
@ 2021-10-07 13:34 ` Philippe Mathieu-Daudé
2021-11-02 12:33 ` Kevin Wolf
0 siblings, 1 reply; 17+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-10-07 13:34 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Fam Zheng, Kevin Wolf, qemu-block, qemu-devel, Max Reitz, Hanna Reitz
On 10/7/21 15:29, Stefan Hajnoczi wrote:
> On Wed, Oct 06, 2021 at 06:49:31PM +0200, Philippe Mathieu-Daudé wrote:
>> nvme_create_queue_pair() allocates resources with qemu_vfio_dma_map(),
>> but we never release them. Do it in nvme_free_queue() which is called
>> from nvme_free_queue_pair().
>>
>> Reported by valgrind:
>>
>> ==252858== 520,192 bytes in 1 blocks are still reachable in loss record 8,293 of 8,302
>> ==252858== at 0x4846803: memalign (vg_replace_malloc.c:1265)
>> ==252858== by 0x484691F: posix_memalign (vg_replace_malloc.c:1429)
>> ==252858== by 0xB8AFE4: qemu_try_memalign (oslib-posix.c:210)
>> ==252858== by 0xA9E315: nvme_create_queue_pair (nvme.c:229)
>> ==252858== by 0xAA0125: nvme_init (nvme.c:799)
>> ==252858== by 0xAA081C: nvme_file_open (nvme.c:953)
>> ==252858== by 0xA23DDD: bdrv_open_driver (block.c:1550)
>> ==252858== by 0xA24806: bdrv_open_common (block.c:1827)
>> ==252858== by 0xA2889B: bdrv_open_inherit (block.c:3747)
>> ==252858== by 0xA28DE4: bdrv_open (block.c:3840)
>> ==252858== by 0x9E0F8E: bds_tree_init (blockdev.c:675)
>> ==252858== by 0x9E7C74: qmp_blockdev_add (blockdev.c:3551)
>>
>> Fixes: bdd6a90a9e5 ("block: Add VFIO based NVMe driver")
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>> block/nvme.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/block/nvme.c b/block/nvme.c
>> index 6e476f54b9f..903c8ffa060 100644
>> --- a/block/nvme.c
>> +++ b/block/nvme.c
>> @@ -185,6 +185,7 @@ static bool nvme_init_queue(BDRVNVMeState *s, NVMeQueue *q,
>>
>> static void nvme_free_queue(BDRVNVMeState *s, NVMeQueue *q)
>> {
>> + qemu_vfio_dma_unmap(s->vfio, q->queue);
>> qemu_vfree(q->queue);
>> }
>
> I can't figure out the issue. qemu_vfree(q->queue) was already called
> before this patch. How does adding qemu_vfio_dma_unmap() help with the
> valgrind report in the commit description?
You are right, I think I didn't select the correct record
between the 8302 reported by valgrind. I will revisit, thanks.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 5/5] block/nvme: Fix memory leak from nvme_init_queue()
2021-10-07 13:34 ` Philippe Mathieu-Daudé
@ 2021-11-02 12:33 ` Kevin Wolf
2021-11-02 12:36 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 17+ messages in thread
From: Kevin Wolf @ 2021-11-02 12:33 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Fam Zheng, qemu-block, qemu-devel, Max Reitz, Hanna Reitz,
Stefan Hajnoczi
Am 07.10.2021 um 15:34 hat Philippe Mathieu-Daudé geschrieben:
> On 10/7/21 15:29, Stefan Hajnoczi wrote:
> > On Wed, Oct 06, 2021 at 06:49:31PM +0200, Philippe Mathieu-Daudé wrote:
> >> nvme_create_queue_pair() allocates resources with qemu_vfio_dma_map(),
> >> but we never release them. Do it in nvme_free_queue() which is called
> >> from nvme_free_queue_pair().
> >>
> >> Reported by valgrind:
> >>
> >> ==252858== 520,192 bytes in 1 blocks are still reachable in loss record 8,293 of 8,302
> >> ==252858== at 0x4846803: memalign (vg_replace_malloc.c:1265)
> >> ==252858== by 0x484691F: posix_memalign (vg_replace_malloc.c:1429)
> >> ==252858== by 0xB8AFE4: qemu_try_memalign (oslib-posix.c:210)
> >> ==252858== by 0xA9E315: nvme_create_queue_pair (nvme.c:229)
> >> ==252858== by 0xAA0125: nvme_init (nvme.c:799)
> >> ==252858== by 0xAA081C: nvme_file_open (nvme.c:953)
> >> ==252858== by 0xA23DDD: bdrv_open_driver (block.c:1550)
> >> ==252858== by 0xA24806: bdrv_open_common (block.c:1827)
> >> ==252858== by 0xA2889B: bdrv_open_inherit (block.c:3747)
> >> ==252858== by 0xA28DE4: bdrv_open (block.c:3840)
> >> ==252858== by 0x9E0F8E: bds_tree_init (blockdev.c:675)
> >> ==252858== by 0x9E7C74: qmp_blockdev_add (blockdev.c:3551)
> >>
> >> Fixes: bdd6a90a9e5 ("block: Add VFIO based NVMe driver")
> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> >> ---
> >> block/nvme.c | 1 +
> >> 1 file changed, 1 insertion(+)
> >>
> >> diff --git a/block/nvme.c b/block/nvme.c
> >> index 6e476f54b9f..903c8ffa060 100644
> >> --- a/block/nvme.c
> >> +++ b/block/nvme.c
> >> @@ -185,6 +185,7 @@ static bool nvme_init_queue(BDRVNVMeState *s, NVMeQueue *q,
> >>
> >> static void nvme_free_queue(BDRVNVMeState *s, NVMeQueue *q)
> >> {
> >> + qemu_vfio_dma_unmap(s->vfio, q->queue);
> >> qemu_vfree(q->queue);
> >> }
> >
> > I can't figure out the issue. qemu_vfree(q->queue) was already called
> > before this patch. How does adding qemu_vfio_dma_unmap() help with the
> > valgrind report in the commit description?
>
> You are right, I think I didn't select the correct record
> between the 8302 reported by valgrind. I will revisit, thanks.
Should we still merge (parts of) this series for 6.2? Or does this mean
that we don't want it at all?
Kevin
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 5/5] block/nvme: Fix memory leak from nvme_init_queue()
2021-11-02 12:33 ` Kevin Wolf
@ 2021-11-02 12:36 ` Philippe Mathieu-Daudé
2021-11-02 14:50 ` Kevin Wolf
0 siblings, 1 reply; 17+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-11-02 12:36 UTC (permalink / raw)
To: Kevin Wolf
Cc: Fam Zheng, qemu-block, qemu-devel, Max Reitz, Hanna Reitz,
Stefan Hajnoczi
On 11/2/21 13:33, Kevin Wolf wrote:
> Am 07.10.2021 um 15:34 hat Philippe Mathieu-Daudé geschrieben:
>> On 10/7/21 15:29, Stefan Hajnoczi wrote:
>>> On Wed, Oct 06, 2021 at 06:49:31PM +0200, Philippe Mathieu-Daudé wrote:
>>>> nvme_create_queue_pair() allocates resources with qemu_vfio_dma_map(),
>>>> but we never release them. Do it in nvme_free_queue() which is called
>>>> from nvme_free_queue_pair().
>>>>
>>>> Reported by valgrind:
>>>>
>>>> ==252858== 520,192 bytes in 1 blocks are still reachable in loss record 8,293 of 8,302
>>>> ==252858== at 0x4846803: memalign (vg_replace_malloc.c:1265)
>>>> ==252858== by 0x484691F: posix_memalign (vg_replace_malloc.c:1429)
>>>> ==252858== by 0xB8AFE4: qemu_try_memalign (oslib-posix.c:210)
>>>> ==252858== by 0xA9E315: nvme_create_queue_pair (nvme.c:229)
>>>> ==252858== by 0xAA0125: nvme_init (nvme.c:799)
>>>> ==252858== by 0xAA081C: nvme_file_open (nvme.c:953)
>>>> ==252858== by 0xA23DDD: bdrv_open_driver (block.c:1550)
>>>> ==252858== by 0xA24806: bdrv_open_common (block.c:1827)
>>>> ==252858== by 0xA2889B: bdrv_open_inherit (block.c:3747)
>>>> ==252858== by 0xA28DE4: bdrv_open (block.c:3840)
>>>> ==252858== by 0x9E0F8E: bds_tree_init (blockdev.c:675)
>>>> ==252858== by 0x9E7C74: qmp_blockdev_add (blockdev.c:3551)
>>>>
>>>> Fixes: bdd6a90a9e5 ("block: Add VFIO based NVMe driver")
>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>>> ---
>>>> block/nvme.c | 1 +
>>>> 1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/block/nvme.c b/block/nvme.c
>>>> index 6e476f54b9f..903c8ffa060 100644
>>>> --- a/block/nvme.c
>>>> +++ b/block/nvme.c
>>>> @@ -185,6 +185,7 @@ static bool nvme_init_queue(BDRVNVMeState *s, NVMeQueue *q,
>>>>
>>>> static void nvme_free_queue(BDRVNVMeState *s, NVMeQueue *q)
>>>> {
>>>> + qemu_vfio_dma_unmap(s->vfio, q->queue);
>>>> qemu_vfree(q->queue);
>>>> }
>>>
>>> I can't figure out the issue. qemu_vfree(q->queue) was already called
>>> before this patch. How does adding qemu_vfio_dma_unmap() help with the
>>> valgrind report in the commit description?
>>
>> You are right, I think I didn't select the correct record
>> between the 8302 reported by valgrind. I will revisit, thanks.
>
> Should we still merge (parts of) this series for 6.2? Or does this mean
> that we don't want it at all?
Patches #1-4 are cleanups welcome for 6.2 :) However we do not want #5.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 5/5] block/nvme: Fix memory leak from nvme_init_queue()
2021-11-02 12:36 ` Philippe Mathieu-Daudé
@ 2021-11-02 14:50 ` Kevin Wolf
2021-11-02 15:17 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 17+ messages in thread
From: Kevin Wolf @ 2021-11-02 14:50 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Fam Zheng, qemu-block, qemu-devel, Max Reitz, Hanna Reitz,
Stefan Hajnoczi
Am 02.11.2021 um 13:36 hat Philippe Mathieu-Daudé geschrieben:
> On 11/2/21 13:33, Kevin Wolf wrote:
> > Am 07.10.2021 um 15:34 hat Philippe Mathieu-Daudé geschrieben:
> >> On 10/7/21 15:29, Stefan Hajnoczi wrote:
> >>> On Wed, Oct 06, 2021 at 06:49:31PM +0200, Philippe Mathieu-Daudé wrote:
> >>>> nvme_create_queue_pair() allocates resources with qemu_vfio_dma_map(),
> >>>> but we never release them. Do it in nvme_free_queue() which is called
> >>>> from nvme_free_queue_pair().
> >>>>
> >>>> Reported by valgrind:
> >>>>
> >>>> ==252858== 520,192 bytes in 1 blocks are still reachable in loss record 8,293 of 8,302
> >>>> ==252858== at 0x4846803: memalign (vg_replace_malloc.c:1265)
> >>>> ==252858== by 0x484691F: posix_memalign (vg_replace_malloc.c:1429)
> >>>> ==252858== by 0xB8AFE4: qemu_try_memalign (oslib-posix.c:210)
> >>>> ==252858== by 0xA9E315: nvme_create_queue_pair (nvme.c:229)
> >>>> ==252858== by 0xAA0125: nvme_init (nvme.c:799)
> >>>> ==252858== by 0xAA081C: nvme_file_open (nvme.c:953)
> >>>> ==252858== by 0xA23DDD: bdrv_open_driver (block.c:1550)
> >>>> ==252858== by 0xA24806: bdrv_open_common (block.c:1827)
> >>>> ==252858== by 0xA2889B: bdrv_open_inherit (block.c:3747)
> >>>> ==252858== by 0xA28DE4: bdrv_open (block.c:3840)
> >>>> ==252858== by 0x9E0F8E: bds_tree_init (blockdev.c:675)
> >>>> ==252858== by 0x9E7C74: qmp_blockdev_add (blockdev.c:3551)
> >>>>
> >>>> Fixes: bdd6a90a9e5 ("block: Add VFIO based NVMe driver")
> >>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> >>>> ---
> >>>> block/nvme.c | 1 +
> >>>> 1 file changed, 1 insertion(+)
> >>>>
> >>>> diff --git a/block/nvme.c b/block/nvme.c
> >>>> index 6e476f54b9f..903c8ffa060 100644
> >>>> --- a/block/nvme.c
> >>>> +++ b/block/nvme.c
> >>>> @@ -185,6 +185,7 @@ static bool nvme_init_queue(BDRVNVMeState *s, NVMeQueue *q,
> >>>>
> >>>> static void nvme_free_queue(BDRVNVMeState *s, NVMeQueue *q)
> >>>> {
> >>>> + qemu_vfio_dma_unmap(s->vfio, q->queue);
> >>>> qemu_vfree(q->queue);
> >>>> }
> >>>
> >>> I can't figure out the issue. qemu_vfree(q->queue) was already called
> >>> before this patch. How does adding qemu_vfio_dma_unmap() help with the
> >>> valgrind report in the commit description?
> >>
> >> You are right, I think I didn't select the correct record
> >> between the 8302 reported by valgrind. I will revisit, thanks.
> >
> > Should we still merge (parts of) this series for 6.2? Or does this mean
> > that we don't want it at all?
>
> Patches #1-4 are cleanups welcome for 6.2 :) However we do not want #5.
Thanks. Patch 4 doesn't seem to make sense without 5 (and definitely not
without rewriting the commit message), but I'm applying patches 1-3.
Kevin
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 5/5] block/nvme: Fix memory leak from nvme_init_queue()
2021-11-02 14:50 ` Kevin Wolf
@ 2021-11-02 15:17 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 17+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-11-02 15:17 UTC (permalink / raw)
To: Kevin Wolf
Cc: Fam Zheng, qemu-block, qemu-devel, Max Reitz, Hanna Reitz,
Stefan Hajnoczi
On 11/2/21 15:50, Kevin Wolf wrote:
> Am 02.11.2021 um 13:36 hat Philippe Mathieu-Daudé geschrieben:
>> On 11/2/21 13:33, Kevin Wolf wrote:
>>> Should we still merge (parts of) this series for 6.2? Or does this mean
>>> that we don't want it at all?
>>
>> Patches #1-4 are cleanups welcome for 6.2 :) However we do not want #5.
>
> Thanks. Patch 4 doesn't seem to make sense without 5 (and definitely not
> without rewriting the commit message), but I'm applying patches 1-3.
Oops indeed. Thank you, that helps.
^ permalink raw reply [flat|nested] 17+ messages in thread