Patchwork [PATCHv2,2/2] migration: do not overwrite zero pages

login
register
mail settings
Submitter Peter Lieven
Date June 10, 2013, 10:14 a.m.
Message ID <1370859260-8183-3-git-send-email-pl@kamp.de>
Download mbox | patch
Permalink /patch/250207/
State New
Headers show

Comments

Peter Lieven - June 10, 2013, 10:14 a.m.
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(-)
Michael Roth - June 10, 2013, 4:10 p.m.
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
> 
>
Michael Roth - June 10, 2013, 4:17 p.m.
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 :)
Orit Wasserman - June 10, 2013, 5:13 p.m.
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?
Peter Lieven - June 10, 2013, 6:50 p.m.
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
Wayne Xia - June 13, 2013, 3:30 a.m.
于 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;
>
Peter Lieven - June 13, 2013, 6:21 a.m.
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

Patch

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;