On 03/27/2018 11:24 PM, Xiao Guangrong wrote:
On 03/28/2018 11:01 AM, Wang, Wei W wrote: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, wepost 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.Thank you both for the nice help on the 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>Thanks.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.Yes, it is a good point.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 need1) 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.So there is another barrier introduced which hurts the parallel...Hmm, we need more deliberate consideration on this point, let me think it over after this work.
Sure. Just a reminder, this doesn't have to be a barrier to the compression, it is just used to serialize sending the pages.
Btw, this reminds me a possible bug in this patch (also in the current upstream code): there appears to be no guarantee that the first page will be sent before others. The migration thread and the compression thread use different buffers. The migration thread just puts the first page into its buffer first, the second page is put to the compression thread buffer later. There appears to be no guarantee that the migration thread will flush its buffer before the compression thread.
Best, Wei
