On Tuesday, March 13, 2018 3:58 PM, Xiao Guangrong wrote:
>
> As compression is a heavy work, do not do it in migration thread, instead, we
> post it out as a normal page
>
> Signed-off-by: Xiao Guangrong <xiaoguangrong@xxxxxxxxxxx>
Hi Guangrong,
Dave asked me to help review your patch, so I will just drop my 2 cents wherever possible, and hope that could be inspiring for your work.
> ---
> migration/ram.c | 32 ++++++++++++++++----------------
> 1 file changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/migration/ram.c b/migration/ram.c index
> 7266351fd0..615693f180 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -1132,7 +1132,7 @@ static int ram_save_compressed_page(RAMState
> *rs, PageSearchStatus *pss,
> int pages = -1;
> uint64_t bytes_xmit = 0;
> uint8_t *p;
> - int ret, blen;
> + int ret;
> RAMBlock *block = pss->block;
> ram_addr_t offset = pss->page << TARGET_PAGE_BITS;
>
> @@ -1162,23 +1162,23 @@ static int
> ram_save_compressed_page(RAMState *rs, PageSearchStatus *pss,
> if (block != rs->last_sent_block) {
> flush_compressed_data(rs);
> pages = save_zero_page(rs, block, offset);
> - if (pages == -1) {
> - /* Make sure the first page is sent out before other pages */
> - bytes_xmit = save_page_header(rs, rs->f, block, offset |
> - RAM_SAVE_FLAG_COMPRESS_PAGE);
> - blen = qemu_put_compression_data(rs->f, p, TARGET_PAGE_SIZE,
> - migrate_compress_level());
> - if (blen > 0) {
> - ram_counters.transferred += bytes_xmit + blen;
> - ram_counters.normal++;
> - pages = 1;
> - } else {
> - qemu_file_set_error(rs->f, blen);
> - error_report("compressed data failed!");
> - }
> - }
> if (pages > 0) {
> ram_release_pages(block->idstr, offset, pages);
> + } else {
> + /*
> + * Make sure the first page is sent out before other pages.
> + *
> + * we post it as normal page as compression will take much
> + * CPU resource.
> + */
> + ram_counters.transferred += save_page_header(rs, rs->f, block,
> + offset | RAM_SAVE_FLAG_PAGE);
> + qemu_put_buffer_async(rs->f, p, TARGET_PAGE_SIZE,
> + migrate_release_ram() &
> + migration_in_postcopy());
> + ram_counters.transferred += TARGET_PAGE_SIZE;
> + ram_counters.normal++;
> + pages = 1;
> }
> } else {
> pages = save_zero_page(rs, block, offset);
> --
I agree that this patch is an improvement for the current implementation. So just pile up mine here:
Reviewed-by: Wei Wang <wei.w.wang@xxxxxxxxx>
If you are interested in something more aggressive, I can share an alternative approach, which I think would be better. Please see below.
Actually, we can use the multi-threaded compression for the first page as well, which will not block the migration thread progress. The advantage is that we can enjoy the compression benefit for the first page and meanwhile not blocking the migration thread - the page is given to a compression thread and compressed asynchronously to the migration thread execution.
The main barrier to achieving the above that is that we need to make sure the first page of each block is sent first in the multi-threaded environment. We can twist the current implementation to achieve that, which is not hard:
For example, we can add a new flag to RAMBlock - bool first_page_added. In each thread of compression, they need
1) check if this is the first page of the block.
2) If it is the first page, set block->first_page_added after sending the page;
3) If it is not the first the page, wait to send the page only when block->first_page_added is set.
Best,
Wei
