[PATCH 0/5] block/nvme: Fix a memory leak in nvme_free_queue_pair()

[PATCH 0/5] block/nvme: Fix a memory leak in nvme_free_queue_pair()

[Philippe Mathieu-Daudé]

Hi,

While searching for a memory corruption in the NVMe block driver,
Valgrind revealed one leak. It is not what I'm looking for, but
worth fixing (at least one warning removed from the huge valgrind
report).

Philippe Mathieu-Daudé (5):
  block/nvme: Automatically free qemu_memalign() with QEMU_AUTO_VFREE
  block/nvme: Display CQ/SQ pointer in nvme_free_queue_pair()
  block/nvme: Extract nvme_free_queue() from nvme_free_queue_pair()
  block/nvme: Pass BDRVNVMeState* handle to nvme_free_queue_pair()
  block/nvme: Fix memory leak from nvme_init_queue()

 block/nvme.c       | 31 +++++++++++++++++--------------
 block/trace-events |  2 +-
 2 files changed, 18 insertions(+), 15 deletions(-)

-- 
2.31.1

[PATCH 1/5] block/nvme: Automatically free qemu_memalign() with QEMU_AUTO_VFREE

[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

[PATCH 2/5] block/nvme: Display CQ/SQ pointer in nvme_free_queue_pair()

[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

[PATCH 3/5] block/nvme: Extract nvme_free_queue() from nvme_free_queue_pair()

[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

[PATCH 4/5] block/nvme: Pass BDRVNVMeState* handle to nvme_free_queue_pair()

[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

[PATCH 5/5] block/nvme: Fix memory leak from nvme_init_queue()

[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

Re: [PATCH 5/5] block/nvme: Fix memory leak from nvme_init_queue()

[Philippe Mathieu-Daudé]

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);
>  }
>  
> 

Re: [PATCH 5/5] block/nvme: Fix memory leak from nvme_init_queue()

[Stefan Hajnoczi]

[-- 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 --]

Re: [PATCH 1/5] block/nvme: Automatically free qemu_memalign() with QEMU_AUTO_VFREE

[Stefan Hajnoczi]

[-- 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 --]

Re: [PATCH 2/5] block/nvme: Display CQ/SQ pointer in nvme_free_queue_pair()

[Stefan Hajnoczi]

[-- 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 --]

Re: [PATCH 3/5] block/nvme: Extract nvme_free_queue() from nvme_free_queue_pair()

[Stefan Hajnoczi]

[-- 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 --]

Re: [PATCH 4/5] block/nvme: Pass BDRVNVMeState* handle to nvme_free_queue_pair()

[Stefan Hajnoczi]

[-- 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 --]

Re: [PATCH 5/5] block/nvme: Fix memory leak from nvme_init_queue()

[Philippe Mathieu-Daudé]

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.

Re: [PATCH 5/5] block/nvme: Fix memory leak from nvme_init_queue()

[Kevin Wolf]

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

Re: [PATCH 5/5] block/nvme: Fix memory leak from nvme_init_queue()

[Philippe Mathieu-Daudé]

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.

Re: [PATCH 5/5] block/nvme: Fix memory leak from nvme_init_queue()

[Kevin Wolf]

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

Re: [PATCH 5/5] block/nvme: Fix memory leak from nvme_init_queue()

[Philippe Mathieu-Daudé]

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.