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
