qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Philippe Mathieu-Daudé
Subject: Re: [PATCH 5/5] block/nvme: Fix memory leak from nvme_init_queue()
Date: Tue, 2 Nov 2021 13:36:38 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.2.0

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.




reply via email to

[Prev in Thread] Current Thread [Next in Thread]