Re: [Qemu-devel] [PATCH 4/6] save_xbzrle_page: change calling convention

[Juan Quintela <quintela@redhat.com>]

Amit Shah <amit.shah@redhat.com> wrote:
> On (Thu) 12 Feb 2015 [23:03:09], Juan Quintela wrote:
>> Add a parameter to pass the number of bytes written, and make it return
>> the number of pages written instead.
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  arch_init.c | 44 +++++++++++++++++++++++++-------------------
>>  1 file changed, 25 insertions(+), 19 deletions(-)
>> 
>> diff --git a/arch_init.c b/arch_init.c
>> index f243c6e..834f40c 100644
>> --- a/arch_init.c
>> +++ b/arch_init.c
>> @@ -352,11 +352,27 @@ static void xbzrle_cache_zero_page(ram_addr_t current_addr)
>> 
>>  #define ENCODING_FLAG_XBZRLE 0x1
>> 
>> +/**
>> + * save_xbzrle_page: compress and send current page
>> + *
>> + * Returns: 1 means that we wrote the page
>> + *          0 means that page is identical to the one already sent
>> + *          -1 means that xbzrle would be longer than normal
>> + *
>> + * @f: QEMUFile where to send the data
>> + * @current_data:
>> + * @current_addr:
>> + * @block: block that contains the page we want to send
>> + * @offset: offset inside the block for the page
>> + * @last_stage: if we are at the completion stage
>> + * @bytes_transferred: increase it with the number of transferred bytes
>> + */
>>  static int save_xbzrle_page(QEMUFile *f, uint8_t **current_data,
>>                              ram_addr_t current_addr, RAMBlock *block,
>> -                            ram_addr_t offset, int cont, bool last_stage)
>> +                            ram_addr_t offset, int cont, bool last_stage,
>> +                            uint64_t *bytes_transferred)
>>  {
>> -    int encoded_len = 0, bytes_sent = -1;
>> +    int encoded_len = 0, bytes_xbzrle;
>>      uint8_t *prev_cached_page;
>> 
>>      if (!cache_is_cached(XBZRLE.cache, current_addr, bitmap_sync_count)) {
>
> This patch exposes a bug in here:
>
>     if (!cache_is_cached(XBZRLE.cache, current_addr, bitmap_sync_count)) {
>         acct_info.xbzrle_cache_miss++;
>         if (!last_stage) {
>              if (cache_insert(XBZRLE.cache, current_addr, *current_data,
>                               bitmap_sync_count) == -1) {
>         	                     return -1;
>              } else {
>                  /* update *current_data when the page has been
>                     inserted into cache */
>                  *current_data = get_cached_data(XBZRLE.cache, current_addr);
>              }
>         }
>         return -1;
>     }
>
> The return -1 should not be done for a successful insert of page in
> cache.

Why?
Return -1 means:
>> + *          -1 means that xbzrle would be longer than normal

in the case that page was not on the cache, we just sent the page as
normal page, that is what we want.



>
> Also, return -1 for both cache overflow and cache miss are perhaps not
> relevant for the caller but the doctext needs to be updated.

-1 means: send the full page, please.
I am not sure that caller want to know if it is because the cache was
full or because the page was not in cache, or because the page was not
"comprhensible".

Later, Juan.