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

[David Hildenbrand <david@redhat.com>]

On 27.08.24 20:41, Peter Xu wrote:
> 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.

I'll move it to the higher level because I have more important stuff to 
work on and want to get this off my plate.

"num_blocks" does not quite make sense in RAMList (where we have a 
different "blocks" variable) so I'll call it "num_dirty_blocks" or sth 
like that.

-- 
Cheers,

David / dhildenb