[Qemu-devel] [PATCHv2 0/2] fix migration of zero pages

[Qemu-devel] [PATCHv2 0/2] fix migration of zero pages

[Peter Lieven]

There have been reports that migration is broken on pseries by Alexey Kardashevskiy.
It turned out that migration will fail in general as soon as a page is zero on the
source, but not on the destination. I thus reverted the skipping of zero pages
in bulk transfer phase and added a patch that does not (over)write zero pages
at the destination.

v2:
 - keep skipped pages fields in monitoring and qmp intact to avoid compatiblity
   issues.

Peter Lieven (2):
  Revert "migration: do not sent zero pages in bulk stage"
  migration: do not overwrite zero pages

 arch_init.c |   27 ++++++++++++---------------
 1 file changed, 12 insertions(+), 15 deletions(-)

-- 
1.7.9.5

[Qemu-devel] [PATCHv2 1/2] Revert "migration: do not sent zero pages in bulk stage"

[Peter Lieven]

Not sending zero pages breaks migration if a page is zero
at the source but not at the destination. This can e.g. happen
if different BIOS versions are used at source and destination.
It has also been reported that migration on pseries is completely
broken with this patch.

This effectively reverts commit f1c72795af573b24a7da5eb52375c9aba8a37972.

Conflicts:

	arch_init.c

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 arch_init.c |   13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/arch_init.c b/arch_init.c
index 5d32ecf..08fccf6 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -457,15 +457,10 @@ static int ram_save_block(QEMUFile *f, bool last_stage)
             bytes_sent = -1;
             if (is_zero_page(p)) {
                 acct_info.dup_pages++;
-                if (!ram_bulk_stage) {
-                    bytes_sent = save_block_hdr(f, block, offset, cont,
-                                                RAM_SAVE_FLAG_COMPRESS);
-                    qemu_put_byte(f, 0);
-                    bytes_sent++;
-                } else {
-                    acct_info.skipped_pages++;
-                    bytes_sent = 0;
-                }
+                bytes_sent = save_block_hdr(f, block, offset, cont,
+                                            RAM_SAVE_FLAG_COMPRESS);
+                qemu_put_byte(f, 0);
+                bytes_sent++;
             } else if (!ram_bulk_stage && migrate_use_xbzrle()) {
                 current_addr = block->offset + offset;
                 bytes_sent = save_xbzrle_page(f, p, current_addr, block,
-- 
1.7.9.5

[Qemu-devel] [PATCHv2 2/2] migration: do not overwrite zero pages

[Peter Lieven]

on incoming migration do not memset pages to zero if they already read as zero.
this will allocate a new zero page and consume memory unnecessarily. even
if we madvise a MADV_DONTNEED later this will only deallocate the memory
asynchronously.

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 arch_init.c |   14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/arch_init.c b/arch_init.c
index 08fccf6..cf4e1d5 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -832,14 +832,16 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
             }
 
             ch = qemu_get_byte(f);
-            memset(host, ch, TARGET_PAGE_SIZE);
+            if (ch != 0 || !is_zero_page(host)) {
+                memset(host, ch, TARGET_PAGE_SIZE);
 #ifndef _WIN32
-            if (ch == 0 &&
-                (!kvm_enabled() || kvm_has_sync_mmu()) &&
-                getpagesize() <= TARGET_PAGE_SIZE) {
-                qemu_madvise(host, TARGET_PAGE_SIZE, QEMU_MADV_DONTNEED);
-            }
+                if (ch == 0 &&
+                    (!kvm_enabled() || kvm_has_sync_mmu()) &&
+                    getpagesize() <= TARGET_PAGE_SIZE) {
+                    qemu_madvise(host, TARGET_PAGE_SIZE, QEMU_MADV_DONTNEED);
+                }
 #endif
+            }
         } else if (flags & RAM_SAVE_FLAG_PAGE) {
             void *host;
 
-- 
1.7.9.5

Re: [Qemu-devel] [PATCHv2 1/2] Revert "migration: do not sent zero pages in bulk stage"

[mdroth]

On Mon, Jun 10, 2013 at 12:14:19PM +0200, Peter Lieven wrote:
> Not sending zero pages breaks migration if a page is zero
> at the source but not at the destination. This can e.g. happen
> if different BIOS versions are used at source and destination.
> It has also been reported that migration on pseries is completely
> broken with this patch.
> 
> This effectively reverts commit f1c72795af573b24a7da5eb52375c9aba8a37972.
> 
> Conflicts:
> 
> 	arch_init.c
> 
> Signed-off-by: Peter Lieven <pl@kamp.de>

Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>

Also CC'ing qemu-stable

> ---
>  arch_init.c |   13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/arch_init.c b/arch_init.c
> index 5d32ecf..08fccf6 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -457,15 +457,10 @@ static int ram_save_block(QEMUFile *f, bool last_stage)
>              bytes_sent = -1;
>              if (is_zero_page(p)) {
>                  acct_info.dup_pages++;
> -                if (!ram_bulk_stage) {
> -                    bytes_sent = save_block_hdr(f, block, offset, cont,
> -                                                RAM_SAVE_FLAG_COMPRESS);
> -                    qemu_put_byte(f, 0);
> -                    bytes_sent++;
> -                } else {
> -                    acct_info.skipped_pages++;
> -                    bytes_sent = 0;
> -                }
> +                bytes_sent = save_block_hdr(f, block, offset, cont,
> +                                            RAM_SAVE_FLAG_COMPRESS);
> +                qemu_put_byte(f, 0);
> +                bytes_sent++;
>              } else if (!ram_bulk_stage && migrate_use_xbzrle()) {
>                  current_addr = block->offset + offset;
>                  bytes_sent = save_xbzrle_page(f, p, current_addr, block,
> -- 
> 1.7.9.5
> 
> 

Re: [Qemu-devel] [PATCHv2 2/2] migration: do not overwrite zero pages

[mdroth]

On Mon, Jun 10, 2013 at 12:14:20PM +0200, Peter Lieven wrote:
> on incoming migration do not memset pages to zero if they already read as zero.
> this will allocate a new zero page and consume memory unnecessarily. even
> if we madvise a MADV_DONTNEED later this will only deallocate the memory
> asynchronously.
> 
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  arch_init.c |   14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/arch_init.c b/arch_init.c
> index 08fccf6..cf4e1d5 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -832,14 +832,16 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
>              }
> 
>              ch = qemu_get_byte(f);
> -            memset(host, ch, TARGET_PAGE_SIZE);
> +            if (ch != 0 || !is_zero_page(host)) {
> +                memset(host, ch, TARGET_PAGE_SIZE);
>  #ifndef _WIN32
> -            if (ch == 0 &&
> -                (!kvm_enabled() || kvm_has_sync_mmu()) &&
> -                getpagesize() <= TARGET_PAGE_SIZE) {
> -                qemu_madvise(host, TARGET_PAGE_SIZE, QEMU_MADV_DONTNEED);
> -            }
> +                if (ch == 0 &&
> +                    (!kvm_enabled() || kvm_has_sync_mmu()) &&
> +                    getpagesize() <= TARGET_PAGE_SIZE) {
> +                    qemu_madvise(host, TARGET_PAGE_SIZE, QEMU_MADV_DONTNEED);
> +                }

Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>

Also CC'ing qemu-stable, but from what I gather this just mitigates the
performance impact of 1/2 and isn't strictly necessary to fix migration?
i.e. we can still do outgoing migration to 1.5.0 guests?

>  #endif
> +            }
>          } else if (flags & RAM_SAVE_FLAG_PAGE) {
>              void *host;
> 
> -- 
> 1.7.9.5
> 
> 

Re: [Qemu-devel] [PATCHv2 2/2] migration: do not overwrite zero pages

[mdroth]

On Mon, Jun 10, 2013 at 11:10:29AM -0500, mdroth wrote:
> On Mon, Jun 10, 2013 at 12:14:20PM +0200, Peter Lieven wrote:
> > on incoming migration do not memset pages to zero if they already read as zero.
> > this will allocate a new zero page and consume memory unnecessarily. even
> > if we madvise a MADV_DONTNEED later this will only deallocate the memory
> > asynchronously.
> > 
> > Signed-off-by: Peter Lieven <pl@kamp.de>
> > ---
> >  arch_init.c |   14 ++++++++------
> >  1 file changed, 8 insertions(+), 6 deletions(-)
> > 
> > diff --git a/arch_init.c b/arch_init.c
> > index 08fccf6..cf4e1d5 100644
> > --- a/arch_init.c
> > +++ b/arch_init.c
> > @@ -832,14 +832,16 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
> >              }
> > 
> >              ch = qemu_get_byte(f);
> > -            memset(host, ch, TARGET_PAGE_SIZE);
> > +            if (ch != 0 || !is_zero_page(host)) {
> > +                memset(host, ch, TARGET_PAGE_SIZE);
> >  #ifndef _WIN32
> > -            if (ch == 0 &&
> > -                (!kvm_enabled() || kvm_has_sync_mmu()) &&
> > -                getpagesize() <= TARGET_PAGE_SIZE) {
> > -                qemu_madvise(host, TARGET_PAGE_SIZE, QEMU_MADV_DONTNEED);
> > -            }
> > +                if (ch == 0 &&
> > +                    (!kvm_enabled() || kvm_has_sync_mmu()) &&
> > +                    getpagesize() <= TARGET_PAGE_SIZE) {
> > +                    qemu_madvise(host, TARGET_PAGE_SIZE, QEMU_MADV_DONTNEED);
> > +                }
> 
> Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> 
> Also CC'ing qemu-stable, but from what I gather this just mitigates the

*actually* CC'ing qemu-stable this time :)

Re: [Qemu-devel] [PATCHv2 1/2] Revert "migration: do not sent zero pages in bulk stage"

[Orit Wasserman]

On 06/10/2013 01:14 PM, Peter Lieven wrote:
> Not sending zero pages breaks migration if a page is zero
> at the source but not at the destination. This can e.g. happen
> if different BIOS versions are used at source and destination.
> It has also been reported that migration on pseries is completely
> broken with this patch.
> 
> This effectively reverts commit f1c72795af573b24a7da5eb52375c9aba8a37972.
> 
> Conflicts:
> 
> 	arch_init.c
> 
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  arch_init.c |   13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/arch_init.c b/arch_init.c
> index 5d32ecf..08fccf6 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -457,15 +457,10 @@ static int ram_save_block(QEMUFile *f, bool last_stage)
>              bytes_sent = -1;
>              if (is_zero_page(p)) {
>                  acct_info.dup_pages++;
> -                if (!ram_bulk_stage) {
> -                    bytes_sent = save_block_hdr(f, block, offset, cont,
> -                                                RAM_SAVE_FLAG_COMPRESS);
> -                    qemu_put_byte(f, 0);
> -                    bytes_sent++;
> -                } else {
> -                    acct_info.skipped_pages++;
> -                    bytes_sent = 0;
> -                }
> +                bytes_sent = save_block_hdr(f, block, offset, cont,
> +                                            RAM_SAVE_FLAG_COMPRESS);
> +                qemu_put_byte(f, 0);
> +                bytes_sent++;
>              } else if (!ram_bulk_stage && migrate_use_xbzrle()) {
>                  current_addr = block->offset + offset;
>                  bytes_sent = save_xbzrle_page(f, p, current_addr, block,
> 

Reviewed-by: Orit Wasserman <owasserm@redhat.com>

Re: [Qemu-devel] [PATCHv2 2/2] migration: do not overwrite zero pages

[Orit Wasserman]

On 06/10/2013 01:14 PM, Peter Lieven wrote:
> on incoming migration do not memset pages to zero if they already read as zero.
> this will allocate a new zero page and consume memory unnecessarily. even
> if we madvise a MADV_DONTNEED later this will only deallocate the memory
> asynchronously.
> 
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  arch_init.c |   14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/arch_init.c b/arch_init.c
> index 08fccf6..cf4e1d5 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -832,14 +832,16 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
>              }
>  
>              ch = qemu_get_byte(f);
> -            memset(host, ch, TARGET_PAGE_SIZE);
> +            if (ch != 0 || !is_zero_page(host)) {
> +                memset(host, ch, TARGET_PAGE_SIZE);
>  #ifndef _WIN32
> -            if (ch == 0 &&
> -                (!kvm_enabled() || kvm_has_sync_mmu()) &&
> -                getpagesize() <= TARGET_PAGE_SIZE) {
> -                qemu_madvise(host, TARGET_PAGE_SIZE, QEMU_MADV_DONTNEED);
> -            }
> +                if (ch == 0 &&
> +                    (!kvm_enabled() || kvm_has_sync_mmu()) &&
> +                    getpagesize() <= TARGET_PAGE_SIZE) {
> +                    qemu_madvise(host, TARGET_PAGE_SIZE, QEMU_MADV_DONTNEED);
> +                }
>  #endif
> +            }
>          } else if (flags & RAM_SAVE_FLAG_PAGE) {
>              void *host;
>  
> 
Reviewed-by: Orit Wasserman <owasserm@redhat.com?

Re: [Qemu-devel] [PATCHv2 2/2] migration: do not overwrite zero pages

[Peter Lieven]

Am 10.06.2013 um 18:10 schrieb mdroth <mdroth@linux.vnet.ibm.com>:

> On Mon, Jun 10, 2013 at 12:14:20PM +0200, Peter Lieven wrote:
>> on incoming migration do not memset pages to zero if they already read as zero.
>> this will allocate a new zero page and consume memory unnecessarily. even
>> if we madvise a MADV_DONTNEED later this will only deallocate the memory
>> asynchronously.
>> 
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> ---
>> arch_init.c |   14 ++++++++------
>> 1 file changed, 8 insertions(+), 6 deletions(-)
>> 
>> diff --git a/arch_init.c b/arch_init.c
>> index 08fccf6..cf4e1d5 100644
>> --- a/arch_init.c
>> +++ b/arch_init.c
>> @@ -832,14 +832,16 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
>>             }
>> 
>>             ch = qemu_get_byte(f);
>> -            memset(host, ch, TARGET_PAGE_SIZE);
>> +            if (ch != 0 || !is_zero_page(host)) {
>> +                memset(host, ch, TARGET_PAGE_SIZE);
>> #ifndef _WIN32
>> -            if (ch == 0 &&
>> -                (!kvm_enabled() || kvm_has_sync_mmu()) &&
>> -                getpagesize() <= TARGET_PAGE_SIZE) {
>> -                qemu_madvise(host, TARGET_PAGE_SIZE, QEMU_MADV_DONTNEED);
>> -            }
>> +                if (ch == 0 &&
>> +                    (!kvm_enabled() || kvm_has_sync_mmu()) &&
>> +                    getpagesize() <= TARGET_PAGE_SIZE) {
>> +                    qemu_madvise(host, TARGET_PAGE_SIZE, QEMU_MADV_DONTNEED);
>> +                }
> 
> Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> 
> Also CC'ing qemu-stable, but from what I gather this just mitigates the
> performance impact of 1/2 and isn't strictly necessary to fix migration?
> i.e. we can still do outgoing migration to 1.5.0 guests?

correct. the regression was introduced by the patch that was reverted in 1/2.
This patch is just a nice to have to avoid unnecessary allocation of zero pages.

Peter

Re: [Qemu-devel] [PATCHv2 1/2] Revert "migration: do not sent zero pages in bulk stage"

[Wenchao Xia]

clear patch, I don't see any issue.

Reviewed-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>

> Not sending zero pages breaks migration if a page is zero
> at the source but not at the destination. This can e.g. happen
> if different BIOS versions are used at source and destination.
> It has also been reported that migration on pseries is completely
> broken with this patch.
> 
> This effectively reverts commit f1c72795af573b24a7da5eb52375c9aba8a37972.
> 
> Conflicts:
> 
> 	arch_init.c
> 
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>   arch_init.c |   13 ++++---------
>   1 file changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/arch_init.c b/arch_init.c
> index 5d32ecf..08fccf6 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -457,15 +457,10 @@ static int ram_save_block(QEMUFile *f, bool last_stage)
>               bytes_sent = -1;
>               if (is_zero_page(p)) {
>                   acct_info.dup_pages++;
> -                if (!ram_bulk_stage) {
> -                    bytes_sent = save_block_hdr(f, block, offset, cont,
> -                                                RAM_SAVE_FLAG_COMPRESS);
> -                    qemu_put_byte(f, 0);
> -                    bytes_sent++;
> -                } else {
> -                    acct_info.skipped_pages++;
> -                    bytes_sent = 0;
> -                }
> +                bytes_sent = save_block_hdr(f, block, offset, cont,
> +                                            RAM_SAVE_FLAG_COMPRESS);
> +                qemu_put_byte(f, 0);
> +                bytes_sent++;
>               } else if (!ram_bulk_stage && migrate_use_xbzrle()) {
>                   current_addr = block->offset + offset;
>                   bytes_sent = save_xbzrle_page(f, p, current_addr, block,
> 


-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCHv2 2/2] migration: do not overwrite zero pages

[Wenchao Xia]

于 2013-6-10 18:14, Peter Lieven 写道:
> on incoming migration do not memset pages to zero if they already read as zero.
> this will allocate a new zero page and consume memory unnecessarily. even
> if we madvise a MADV_DONTNEED later this will only deallocate the memory
> asynchronously.
> 
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>   arch_init.c |   14 ++++++++------
>   1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/arch_init.c b/arch_init.c
> index 08fccf6..cf4e1d5 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -832,14 +832,16 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
>               }
> 
>               ch = qemu_get_byte(f);
> -            memset(host, ch, TARGET_PAGE_SIZE);
> +            if (ch != 0 || !is_zero_page(host)) {
  If incoming page is not zero, always memset. If incoming page is
zero, then if on destination it is not zero, memset. Logic is OK.
Only question is if the read operation in is_zero_page() consumes
memory, as there are doubt in the discuss before.
  Any way this patch will not break migration in my opinion.

Reviewed-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>


> +                memset(host, ch, TARGET_PAGE_SIZE);
>   #ifndef _WIN32
> -            if (ch == 0 &&
> -                (!kvm_enabled() || kvm_has_sync_mmu()) &&
> -                getpagesize() <= TARGET_PAGE_SIZE) {
> -                qemu_madvise(host, TARGET_PAGE_SIZE, QEMU_MADV_DONTNEED);
> -            }
> +                if (ch == 0 &&
> +                    (!kvm_enabled() || kvm_has_sync_mmu()) &&
> +                    getpagesize() <= TARGET_PAGE_SIZE) {
> +                    qemu_madvise(host, TARGET_PAGE_SIZE, QEMU_MADV_DONTNEED);
> +                }
>   #endif
> +            }
>           } else if (flags & RAM_SAVE_FLAG_PAGE) {
>               void *host;
> 


-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCHv2 2/2] migration: do not overwrite zero pages

[Peter Lieven]

On 13.06.2013 05:30, Wenchao Xia wrote:
> 于 2013-6-10 18:14, Peter Lieven 写道:
>> on incoming migration do not memset pages to zero if they already read as zero.
>> this will allocate a new zero page and consume memory unnecessarily. even
>> if we madvise a MADV_DONTNEED later this will only deallocate the memory
>> asynchronously.
>>
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> ---
>>   arch_init.c |   14 ++++++++------
>>   1 file changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch_init.c b/arch_init.c
>> index 08fccf6..cf4e1d5 100644
>> --- a/arch_init.c
>> +++ b/arch_init.c
>> @@ -832,14 +832,16 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
>>               }
>>
>>               ch = qemu_get_byte(f);
>> -            memset(host, ch, TARGET_PAGE_SIZE);
>> +            if (ch != 0 || !is_zero_page(host)) {
>   If incoming page is not zero, always memset. If incoming page is
> zero, then if on destination it is not zero, memset. Logic is OK.
> Only question is if the read operation in is_zero_page() consumes
> memory, as there are doubt in the discuss before.

It does not consume memory for normal pages and for thp since 3.8.

BR,
Peter