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

[Peter Xu]

On Mon, Jul 23, 2018 at 03:56:33PM +0800, Xiao Guangrong wrote:

[...]

> > > @@ -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;
>  }

Ah, then it seems fine.  Though I'd suggest you comment these into the
commit message in case people won't get it easily.

> 
> 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.

Do you mean ram_release_pages() is not thread-safe?  Why?  I didn't
notice it before but I feel like it is safe.

Regards,

-- 
Peter Xu