Re: [PATCH v1] softmmu/physmem: fix memory leak in dirty_memory_extend()

[David Hildenbrand <david@redhat.com>]

On 27.08.24 19:50, Peter Xu wrote:
> On Tue, Aug 27, 2024 at 01:28:02PM -0400, Stefan Hajnoczi wrote:
>> On Tue, 27 Aug 2024 at 13:24, David Hildenbrand <david@redhat.com> wrote:
>>>
>>> On 27.08.24 18:52, Stefan Hajnoczi wrote:
>>>> On Tue, 27 Aug 2024 at 04:38, David Hildenbrand <david@redhat.com> wrote:
>>>>>
>>>>> As reported by Peter, we might be leaking memory when removing the
>>>>> highest RAMBlock (in the weird ram_addr_t space), and adding a new one.
>>>>>
>>>>> We will fail to realize that we already allocated bitmaps for more
>>>>> dirty memory blocks, and effectively discard the pointers to them.
>>>>>
>>>>> Fix it by getting rid of last_ram_page() and simply storing the number
>>>>> of dirty memory blocks that have been allocated. We'll store the number
>>>>> of blocks along with the actual pointer to keep it simple.
>>>>>
>>>>> Looks like this leak was introduced as we switched from using a single
>>>>> bitmap_zero_extend() to allocating multiple bitmaps:
>>>>> bitmap_zero_extend() relies on g_renew() which should have taken care of
>>>>> this.
>>>>>
>>>>> Resolves: https://lkml.kernel.org/r/CAFEAcA-k7a+VObGAfCFNygQNfCKL=AfX6A4kScq=VSSK0peqPg@mail.gmail.com
>>>>> Reported-by: Peter Maydell <peter.maydell@linaro.org>
>>>>> Fixes: 5b82b703b69a ("memory: RCU ram_list.dirty_memory[] for safe RAM hotplug")
>>>>> Cc: qemu-stable@nongnu.org
>>>>> Cc: Stefan Hajnoczi <stefanha@redhat.com>
>>>>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>>>>> Cc: Peter Xu <peterx@redhat.com>
>>>>> Cc: "Philippe Mathieu-Daudé" <philmd@linaro.org>
>>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>>> ---
>>>>>    include/exec/ramlist.h |  1 +
>>>>>    system/physmem.c       | 44 ++++++++++++++----------------------------
>>>>>    2 files changed, 16 insertions(+), 29 deletions(-)
>>>>>
>>>>> diff --git a/include/exec/ramlist.h b/include/exec/ramlist.h
>>>>> index 2ad2a81acc..f2a965f293 100644
>>>>> --- a/include/exec/ramlist.h
>>>>> +++ b/include/exec/ramlist.h
>>>>> @@ -41,6 +41,7 @@ typedef struct RAMBlockNotifier RAMBlockNotifier;
>>>>>    #define DIRTY_MEMORY_BLOCK_SIZE ((ram_addr_t)256 * 1024 * 8)
>>>>>    typedef struct {
>>>>>        struct rcu_head rcu;
>>>>> +    unsigned int num_blocks;
>>>>
>>>> The maximum amount of memory supported by unsigned int is:
>>>> (2 ^ 32 - 1) * 4KB * DIRTY_MEMORY_BLOCK_SIZE
>>>> = ~32 exabytes
>>>>
>>>
>>> True, should we simply use ram_addr_t ?
>>
>> Sounds good to me. In practice scalability bottlenecks are likely with
>> those memory sizes and it will be necessary to change how guest memory
>> is organized anyway. But it doesn't hurt to make this counter
>> future-proof.
> 
> IMHO it'll be nice to only use ram_addr_t when a variable is describing the
> ramblock address space (with an offset, or a length there).  In this case
> it is a pure counter for how many bitmap chunks we allocated, so maybe
> "unsigned long" or "uint64_t" would suite more?
> 
> Though I'd think "unsigned int" is good enough per the calculation Stefan
> provided.

Likely best, "ram_addr_t" requires including "exec/cpu-common.h".

So let's stick to "unsigned int" for now. Likely best to also include for consistency:

diff --git a/system/physmem.c b/system/physmem.c
index fa48ff8333..e1391492fd 100644
--- a/system/physmem.c
+++ b/system/physmem.c
@@ -1789,14 +1789,14 @@ void qemu_ram_msync(RAMBlock *block, ram_addr_t start, ram_addr_t length)
  /* Called with ram_list.mutex held */
  static void dirty_memory_extend(ram_addr_t new_ram_size)
  {
-    ram_addr_t new_num_blocks = DIV_ROUND_UP(new_ram_size,
-                                             DIRTY_MEMORY_BLOCK_SIZE);
+    unsigned int new_num_blocks = DIV_ROUND_UP(new_ram_size,
+                                               DIRTY_MEMORY_BLOCK_SIZE);
      int i;
  
      for (i = 0; i < DIRTY_MEMORY_NUM; i++) {
          DirtyMemoryBlocks *old_blocks;
          DirtyMemoryBlocks *new_blocks;
-        ram_addr_t old_num_blocks = 0;
+        unsigned int old_num_blocks = 0;
          int j;
  
          old_blocks = qatomic_rcu_read(&ram_list.dirty_memory[i]);


> 
> Reviewed-by: Peter Xu <peterx@redhat.com>

Thanks!

-- 
Cheers,

David / dhildenb