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

[Peter Xu]

On Tue, Aug 27, 2024 at 08:00:07PM +0200, David Hildenbrand wrote:
> On 27.08.24 19:57, Peter Xu wrote:
> > On Tue, Aug 27, 2024 at 10:37:15AM +0200, David Hildenbrand wrote:
> > >   /* Called with ram_list.mutex held */
> > > -static void dirty_memory_extend(ram_addr_t old_ram_size,
> > > -                                ram_addr_t new_ram_size)
> > > +static void dirty_memory_extend(ram_addr_t new_ram_size)
> > >   {
> > > -    ram_addr_t old_num_blocks = DIV_ROUND_UP(old_ram_size,
> > > -                                             DIRTY_MEMORY_BLOCK_SIZE);
> > >       ram_addr_t new_num_blocks = DIV_ROUND_UP(new_ram_size,
> > >                                                DIRTY_MEMORY_BLOCK_SIZE);
> > >       int i;
> > > -    /* Only need to extend if block count increased */
> > > -    if (new_num_blocks <= old_num_blocks) {
> > > -        return;
> > > -    }
> > 
> > One nitpick here: IMHO we could move the n_blocks cache in ram_list
> > instead, then we keep the check here and avoid caching it three times with
> > the same value.
> 
> yes, as written in the patch description: "We'll store the number of blocks
> along with the actual pointer to keep it simple."
> 
> It's cleaner to me to store it along the RCU-freed data structure that has
> this size.

Yep, I can get that.

I think one reason I had my current preference is to avoid things like:

  for (...) {
    if (...)
       return;
  }

I'd at least want to sanity check before "return" to make sure all three
bitmap chunks are having the same size.  It gave me the feeling that we
could process "blocks[]" differently but we actually couldn't - In our case
it has the ram_list mutex when update, so it must be guaranteed.  However
due to the same reason, I see it cleaner to just keep the counter there
too.

No strong feelings, though.

-- 
Peter Xu