Patchwork [PATCHv2,7/9] migration: do not sent zero pages in bulk stage

login
register
mail settings
Submitter Peter Lieven
Date March 15, 2013, 3:50 p.m.
Message ID <1363362619-3190-8-git-send-email-pl@kamp.de>
Download mbox | patch
Permalink /patch/228152/
State New
Headers show

Comments

Peter Lieven - March 15, 2013, 3:50 p.m.
during bulk stage of ram migration if a page is a
zero page do not send it at all.
the memory at the destination reads as zero anyway.

even if there is an madvise with QEMU_MADV_DONTNEED
at the target upon receival of a zero page I have observed
that the target starts swapping if the memory is overcommitted.
it seems that the pages are dropped asynchronously.

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 arch_init.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)
Eric Blake - March 19, 2013, 5:36 p.m.
On 03/15/2013 09:50 AM, Peter Lieven wrote:
> during bulk stage of ram migration if a page is a
> zero page do not send it at all.
> the memory at the destination reads as zero anyway.
> 
> even if there is an madvise with QEMU_MADV_DONTNEED
> at the target upon receival of a zero page I have observed

s/receival/receipt/

> that the target starts swapping if the memory is overcommitted.
> it seems that the pages are dropped asynchronously.
> 
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  arch_init.c |    8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)

> +++ b/arch_init.c
> @@ -432,9 +432,11 @@ static int ram_save_block(QEMUFile *f, bool last_stage)
>              bytes_sent = -1;
>              if (buffer_is_zero(p, TARGET_PAGE_SIZE)) {
>                  acct_info.dup_pages++;
> -                bytes_sent = save_block_hdr(f, block, offset, cont,
> -                                            RAM_SAVE_FLAG_COMPRESS);
> -                qemu_put_byte(f, *p);
> +                if (!ram_bulk_stage) {
> +                    bytes_sent = save_block_hdr(f, block, offset, cont,
> +                                                RAM_SAVE_FLAG_COMPRESS);
> +                    qemu_put_byte(f, *p);

Hmm, in patch 5/9, why not 'qemu_put_byte(f, 0)' instead of spending
time dereferencing *p, since you already know the byte is 0?

> +                }
>                  bytes_sent += 1;

Your accounting is now off.  This increments bytes_sent even when
nothing was sent in the bulk phase.  When touching this line, consider
using ++ instead of += 1.

>              } else if (migrate_use_xbzrle()) {
>                  current_addr = block->offset + offset;
>
Peter Lieven - March 19, 2013, 7:35 p.m.
Am 19.03.2013 um 18:36 schrieb Eric Blake <eblake@redhat.com>:

> On 03/15/2013 09:50 AM, Peter Lieven wrote:
>> during bulk stage of ram migration if a page is a
>> zero page do not send it at all.
>> the memory at the destination reads as zero anyway.
>> 
>> even if there is an madvise with QEMU_MADV_DONTNEED
>> at the target upon receival of a zero page I have observed
> 
> s/receival/receipt/
> 
>> that the target starts swapping if the memory is overcommitted.
>> it seems that the pages are dropped asynchronously.
>> 
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> ---
>> arch_init.c |    8 +++++---
>> 1 file changed, 5 insertions(+), 3 deletions(-)
> 
>> +++ b/arch_init.c
>> @@ -432,9 +432,11 @@ static int ram_save_block(QEMUFile *f, bool last_stage)
>>             bytes_sent = -1;
>>             if (buffer_is_zero(p, TARGET_PAGE_SIZE)) {
>>                 acct_info.dup_pages++;
>> -                bytes_sent = save_block_hdr(f, block, offset, cont,
>> -                                            RAM_SAVE_FLAG_COMPRESS);
>> -                qemu_put_byte(f, *p);
>> +                if (!ram_bulk_stage) {
>> +                    bytes_sent = save_block_hdr(f, block, offset, cont,
>> +                                                RAM_SAVE_FLAG_COMPRESS);
>> +                    qemu_put_byte(f, *p);
> 
> Hmm, in patch 5/9, why not 'qemu_put_byte(f, 0)' instead of spending
> time dereferencing *p, since you already know the byte is 0?

Good idea. Actually to avoid unnecessary checks in buffer_is_zero I think
I would like to change "if (buffer_is_zero(p, TARGET_PAGE_SIZE)) {..} "  to

bool inline is_zero_page(uint8_t *p) {return buffer_find_nonzero_offset(p, TARGET_PAGE_SIZE) == TARGET_PAGE_SIZE);}
...
if (is_zero_page(p)) {
…
}

also for better readability. For pages we know that they satisfy
can_use_buffer_find_nonzero_offset().

Peter


> 
>> +                }
>>                 bytes_sent += 1;
> 
> Your accounting is now off.  This increments bytes_sent even when
> nothing was sent in the bulk phase.  When touching this line, consider
> using ++ instead of += 1.
> 
>>             } else if (migrate_use_xbzrle()) {
>>                 current_addr = block->offset + offset;
>> 
> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>

Patch

diff --git a/arch_init.c b/arch_init.c
index e5531e8..a3dc20d 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -432,9 +432,11 @@  static int ram_save_block(QEMUFile *f, bool last_stage)
             bytes_sent = -1;
             if (buffer_is_zero(p, TARGET_PAGE_SIZE)) {
                 acct_info.dup_pages++;
-                bytes_sent = save_block_hdr(f, block, offset, cont,
-                                            RAM_SAVE_FLAG_COMPRESS);
-                qemu_put_byte(f, *p);
+                if (!ram_bulk_stage) {
+                    bytes_sent = save_block_hdr(f, block, offset, cont,
+                                                RAM_SAVE_FLAG_COMPRESS);
+                    qemu_put_byte(f, *p);
+                }
                 bytes_sent += 1;
             } else if (migrate_use_xbzrle()) {
                 current_addr = block->offset + offset;