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

2013-06-13 Thread 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 p...@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

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

2013-06-12 Thread 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 p...@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 xiaw...@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

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

2013-06-10 Thread 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 p...@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 2/2] migration: do not overwrite zero pages

2013-06-10 Thread 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 p...@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 mdr...@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

2013-06-10 Thread 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 p...@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 mdr...@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 2/2] migration: do not overwrite zero pages

2013-06-10 Thread 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 p...@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 owass...@redhat.com?

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

2013-06-10 Thread Peter Lieven 

Am 10.06.2013 um 18:10 schrieb mdroth mdr...@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 p...@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 mdr...@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