Re: [PATCH v2 6/8] migration: move handle of zero page to the thread

[Xiao Guangrong <guangrong.xiao@gmail.com>]

On 07/23/2018 01:03 PM, Peter Xu wrote:
> On Thu, Jul 19, 2018 at 08:15:18PM +0800, guangrong.xiao@gmail.com wrote:
> 
> [...]
> 
>> @@ -1950,12 +1971,16 @@ retry:
>>               set_compress_params(&comp_param[idx], block, offset);
>>               qemu_cond_signal(&comp_param[idx].cond);
>>               qemu_mutex_unlock(&comp_param[idx].mutex);
>> -            pages = 1;
>> -            /* 8 means a header with RAM_SAVE_FLAG_CONTINUE. */
>> -            compression_counters.reduced_size += TARGET_PAGE_SIZE -
>> -                                                 bytes_xmit + 8;
>> -            compression_counters.pages++;
>>               ram_counters.transferred += bytes_xmit;
>> +            pages = 1;
> 
> (moving of this line seems irrelevant; meanwhile more duplicated codes
> so even better to have a helper now)
> 

Good to me. :)

>> +            if (comp_param[idx].zero_page) {
>> +                ram_counters.duplicate++;
>> +            } else {
>> +                /* 8 means a header with RAM_SAVE_FLAG_CONTINUE. */
>> +                compression_counters.reduced_size += TARGET_PAGE_SIZE -
>> +                                                     bytes_xmit + 8;
>> +                compression_counters.pages++;
>> +            }
>>               break;
>>           }
>>       }
> 
> [...]
> 
>> @@ -2249,15 +2308,8 @@ static int ram_save_target_page(RAMState *rs, PageSearchStatus *pss,
>>           return res;
>>       }
>>   
>> -    /*
>> -     * When starting the process of a new block, the first page of
>> -     * the block should be sent out before other pages in the same
>> -     * block, and all the pages in last block should have been sent
>> -     * out, keeping this order is important, because the 'cont' flag
>> -     * is used to avoid resending the block name.
>> -     */
>> -    if (block != rs->last_sent_block && save_page_use_compression(rs)) {
>> -            flush_compressed_data(rs);
>> +    if (save_compress_page(rs, block, offset)) {
>> +        return 1;
> 
> It's a bit tricky (though it seems to be a good idea too) to move the
> zero detect into the compression thread, though I noticed that we also
> do something else for zero pages:
> 
>      res = save_zero_page(rs, block, offset);
>      if (res > 0) {
>          /* Must let xbzrle know, otherwise a previous (now 0'd) cached
>           * page would be stale
>           */
>          if (!save_page_use_compression(rs)) {
>              XBZRLE_cache_lock();
>              xbzrle_cache_zero_page(rs, block->offset + offset);
>              XBZRLE_cache_unlock();
>          }
>          ram_release_pages(block->idstr, offset, res);
>          return res;
>      }
> 
> I'd guess that the xbzrle update of the zero page is not needed for
> compression since after all xbzrle is not enabled when compression is

Yup. if they are both enabled, compression works only for the first
iteration (i.e, ram_bulk_stage), at that point, nothing is cached
in xbzrle's cahe, in other words, xbzrle has posted nothing to the
destination.

> enabled, however do we need to do ram_release_pages() somehow?
> 

We have done it in the thread:

+static bool do_compress_ram_page(QEMUFile *f, z_stream *stream, RAMBlock *block,
                                   ram_addr_t offset, uint8_t *source_buf)
  {


+    if (save_zero_page_to_file(rs, f, block, offset)) {
+        zero_page = true;
+        goto exit;
+    }
......

+exit:
      ram_release_pages(block->idstr, offset & TARGET_PAGE_MASK, 1);
+    return zero_page;
  }

However, it is not safe to do ram_release_pages in the thread as it's
not protected it multithreads. Fortunately, compression will be disabled
if it switches to post-copy, so i preferred to keep current behavior and
deferred to fix it after this patchset has been merged.