Re: [PATCH v2 4/4] migration/ram: Merge save_zero_page functions

[Claudio Fontana]

Apologies, already reviewed, will ping for the merge of the series momentarily,

Claudio

On 8/22/23 14:18, Claudio Fontana wrote:
> Hello,
> 
> this patch would still need a review,
> 
> and is needed as a precondition for further work to improve dramatically the 
> performance of virsh save, virsh restore
> when migrating to disk, since Peter Xu already reviewed PATCH 1, 2, 3, maybe 
> it makes sense to look at 4 too?
> 
> Thanks,
> 
> Claudio
> 
> On 8/16/23 20:28, Fabiano Rosas wrote:
>> We don't need to do this in two pieces. One single function makes it
>> easier to grasp, specially since it removes the indirection on the
>> return value handling.
>>
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>  migration/ram.c | 46 +++++++++++++---------------------------------
>>  1 file changed, 13 insertions(+), 33 deletions(-)
>>
>> diff --git a/migration/ram.c b/migration/ram.c
>> index 82ff53beec..13935ead1c 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -1128,32 +1128,6 @@ void ram_release_page(const char *rbname, uint64_t 
>> offset)
>>      ram_discard_range(rbname, offset, TARGET_PAGE_SIZE);
>>  }
>>  
>> -/**
>> - * save_zero_page_to_file: send the zero page to the file
>> - *
>> - * Returns the size of data written to the file, 0 means the page is not
>> - * a zero page
>> - *
>> - * @pss: current PSS channel
>> - * @block: block that contains the page we want to send
>> - * @offset: offset inside the block for the page
>> - */
>> -static int save_zero_page_to_file(PageSearchStatus *pss, RAMBlock *block,
>> -                                  ram_addr_t offset)
>> -{
>> -    uint8_t *p = block->host + offset;
>> -    QEMUFile *file = pss->pss_channel;
>> -    int len = 0;
>> -
>> -    if (buffer_is_zero(p, TARGET_PAGE_SIZE)) {
>> -        len += save_page_header(pss, file, block, offset | 
>> RAM_SAVE_FLAG_ZERO);
>> -        qemu_put_byte(file, 0);
>> -        len += 1;
>> -        ram_release_page(block->idstr, offset);
>> -    }
>> -    return len;
>> -}
>> -
>>  /**
>>   * save_zero_page: send the zero page to the stream
>>   *
>> @@ -1167,12 +1141,19 @@ static int save_zero_page_to_file(PageSearchStatus 
>> *pss, RAMBlock *block,
>>  static int save_zero_page(RAMState *rs, PageSearchStatus *pss, RAMBlock 
>> *block,
>>                            ram_addr_t offset)
>>  {
>> -    int len = save_zero_page_to_file(pss, block, offset);
>> +    uint8_t *p = block->host + offset;
>> +    QEMUFile *file = pss->pss_channel;
>> +    int len = 0;
>>  
>> -    if (!len) {
>> -        return -1;
>> +    if (!buffer_is_zero(p, TARGET_PAGE_SIZE)) {
>> +        return 0;
>>      }
>>  
>> +    len += save_page_header(pss, file, block, offset | RAM_SAVE_FLAG_ZERO);
>> +    qemu_put_byte(file, 0);
>> +    len += 1;
>> +    ram_release_page(block->idstr, offset);
>> +
>>      stat64_add(&mig_stats.zero_pages, 1);
>>      ram_transferred_add(len);
>>  
>> @@ -1186,7 +1167,7 @@ static int save_zero_page(RAMState *rs, 
>> PageSearchStatus *pss, RAMBlock *block,
>>          XBZRLE_cache_unlock();
>>      }
>>  
>> -    return 1;
>> +    return len;
>>  }
>>  
>>  /*
>> @@ -2154,9 +2135,8 @@ static int ram_save_target_page_legacy(RAMState *rs, 
>> PageSearchStatus *pss)
>>          return 1;
>>      }
>>  
>> -    res = save_zero_page(rs, pss, block, offset);
>> -    if (res > 0) {
>> -        return res;
>> +    if (save_zero_page(rs, pss, block, offset)) {
>> +        return 1;
>>      }
>>  
>>      /*
>