| Message ID | 1370859260-8183-3-git-send-email-pl@kamp.de |
|---|---|
| State | New |
| Headers | show |
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 > >
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 :)
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?
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
于 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; >
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
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;
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(-)