Re: [PATCH 2/2] softmmu/physmem: fix dirty memory bitmap memleak

[Peter Xu]

On Thu, Mar 31, 2022 at 07:14:12PM +0300, Andrey Ryabinin wrote:
> 
> 
> On 3/30/22 22:25, Peter Xu wrote:
> > On Fri, Mar 25, 2022 at 06:40:13PM +0300, Andrey Ryabinin wrote:
> >> The sequence of ram_block_add()/qemu_ram_free()/ram_block_add()
> >> function calls leads to leaking some memory.
> >>
> >> ram_block_add() calls dirty_memory_extend() to allocate bitmap blocks
> >> for new memory. These blocks only grow but never shrink. So the
> >> qemu_ram_free() restores RAM size back to it's original stat but
> >> doesn't touch dirty memory bitmaps.
> >>
> >> After qemu_ram_free() there is no way of knowing that we have
> >> allocated dirty memory bitmaps beyond current RAM size.
> >> So the next ram_block_add() will call dirty_memory_extend() again to
> >> to allocate new bitmaps and rewrite pointers to bitmaps left after
> >> the first ram_block_add()/dirty_memory_extend() calls.
> >>
> >> Rework dirty_memory_extend() to be able to shrink dirty maps,
> >> also rename it to dirty_memory_resize(). And fix the leak by
> >> shrinking dirty memory maps on qemu_ram_free() if needed.
> >>
> >> Fixes: 5b82b703b69a ("memory: RCU ram_list.dirty_memory[] for safe RAM 
> >> hotplug")
> >> Cc: qemu-stable@nongnu.org
> >> Signed-off-by: Andrey Ryabinin <arbn@yandex-team.com>
> >> ---
> >>  include/exec/ramlist.h |  2 ++
> >>  softmmu/physmem.c      | 38 ++++++++++++++++++++++++++++++++------
> >>  2 files changed, 34 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/include/exec/ramlist.h b/include/exec/ramlist.h
> >> index 2ad2a81acc..019e238e7c 100644
> >> --- a/include/exec/ramlist.h
> >> +++ b/include/exec/ramlist.h
> >> @@ -41,6 +41,8 @@ typedef struct RAMBlockNotifier RAMBlockNotifier;
> >>  #define DIRTY_MEMORY_BLOCK_SIZE ((ram_addr_t)256 * 1024 * 8)
> >>  typedef struct {
> >>      struct rcu_head rcu;
> >> +    unsigned int nr_blocks;
> >> +    unsigned int nr_blocks_inuse;
> >>      unsigned long *blocks[];
> >>  } DirtyMemoryBlocks;
> > 
> > The problem looks valid, but can we avoid introducing these variables at
> > all?
> > 
> > IIUC what we miss here is the proper releasing of dirty blocks when ram is
> > released.  IMHO as long as we properly release the extra dirty memory
> > blocks in qemu_ram_free(), then last_ram_page() will reflect the existing
> > dirty memory block size.  That looks simpler if I'm correct..
> > 
> 
> ram_list.dirty_memory is RCU protected which means we can't free it 
> immediately.
> Freeing must be delayed until the end of RCU grace period.
> One way to do it is using rcu callback, like in this patch. That why we need 
> these
> variables - to pass the information into rcu callback.
> 
> Another way is using synchronize_rcu() before freeing the memory. In that case
> the variables won't be needed. But it's expensive.
> Also I'm not sure if using synchronize_rcu() under a mutex lock is a good 
> idea.
> Perhaps it will needed somehow to rework dirty_memory_resize() to move 
> synchronize_rcu() and freeing
> outside of mutex_lock()/unlock() section.

I suggested that just because from the 1st glance I failed to read clearly
on the current patch due to some trivial details (I commented inline),
which made me thought that merging extend/shrink is not necessary.  But I
agree it looks mostly correct, and call_rcu() should indeed be preferred.

Thanks,

-- 
Peter Xu