| Message ID | 1382599289-9503-1-git-send-email-pl@kamp.de |
|---|---|
| State | New |
| Headers | show |
Il 24/10/2013 08:21, Peter Lieven ha scritto: > Additionally we memmap target memory so it is essentially > zero initialized (except for e.g. option roms and bios which are loaded > into target memory although they shouldn't). > > It was reported recently that this madvise causes a performance degradation > in some situations. As the madvise should only be called rarely and if it's called > it is likely on a busy page (it was non-zero and changed to zero during migration) > drop it completely. Tagging this patch for 1.7. Paolo
Peter Lieven <pl@kamp.de> wrote: > The madvise for zeroed out pages was introduced when every transferred > zero page was memset to zero and thus allocated. Since commit > 211ea740 we check for zeroness of a target page before we memset > it to zero. Additionally we memmap target memory so it is essentially > zero initialized (except for e.g. option roms and bios which are loaded > into target memory although they shouldn't). > > It was reported recently that this madvise causes a performance degradation > in some situations. As the madvise should only be called rarely and if it's called > it is likely on a busy page (it was non-zero and changed to zero during migration) > drop it completely. Reviewed-by: Juan Quintela <quintela@redhat.com> I take it. I am on KVM Forum/LinuxCon this week. Will send when back at home. Thanks. > > Reported-By: Zhang Haoyu <haoyu.zhang@huawei.com> > Acked-by: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Peter Lieven <pl@kamp.de> > --- > arch_init.c | 8 -------- > 1 file changed, 8 deletions(-) > > diff --git a/arch_init.c b/arch_init.c > index 7545d96..e0acbc5 100644 > --- a/arch_init.c > +++ b/arch_init.c > @@ -850,14 +850,6 @@ void ram_handle_compressed(void *host, uint8_t ch, uint64_t size) > { > if (ch != 0 || !is_zero_range(host, size)) { > memset(host, ch, size); > -#ifndef _WIN32 > - if (ch == 0 && (!kvm_enabled() || kvm_has_sync_mmu())) { > - size = size & ~(getpagesize() - 1); > - if (size > 0) { > - qemu_madvise(host, size, QEMU_MADV_DONTNEED); > - } > - } > -#endif > } > }
The comments of ram_handle_compressed needs to be changed accordingly, "Do not memset pages to zero if they already read as zero to avoid allocating zero pages and consuming memory unnecessarily." Thanks, Zhang Haoyu > The madvise for zeroed out pages was introduced when every transferred > zero page was memset to zero and thus allocated. Since commit > 211ea740 we check for zeroness of a target page before we memset > it to zero. Additionally we memmap target memory so it is essentially > zero initialized (except for e.g. option roms and bios which are loaded > into target memory although they shouldn't). > > It was reported recently that this madvise causes a performance > degradation > in some situations. As the madvise should only be called rarely and if > it's called > it is likely on a busy page (it was non-zero and changed to zero during > migration) > drop it completely. > > Reported-By: Zhang Haoyu <haoyu.zhang@huawei.com> > Acked-by: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Peter Lieven <pl@kamp.de> > --- > arch_init.c | 8 -------- > 1 file changed, 8 deletions(-) > > diff --git a/arch_init.c b/arch_init.c > index 7545d96..e0acbc5 100644 > --- a/arch_init.c > +++ b/arch_init.c > @@ -850,14 +850,6 @@ void ram_handle_compressed(void *host, uint8_t ch, > uint64_t size) > { > if (ch != 0 || !is_zero_range(host, size)) { > memset(host, ch, size); > -#ifndef _WIN32 > - if (ch == 0 && (!kvm_enabled() || kvm_has_sync_mmu())) { > - size = size & ~(getpagesize() - 1); > - if (size > 0) { > - qemu_madvise(host, size, QEMU_MADV_DONTNEED); > - } > - } > -#endif > } > } > > -- > 1.7.9.5 >
On 24.10.2013 12:07, Juan Quintela wrote: > Peter Lieven <pl@kamp.de> wrote: >> The madvise for zeroed out pages was introduced when every transferred >> zero page was memset to zero and thus allocated. Since commit >> 211ea740 we check for zeroness of a target page before we memset >> it to zero. Additionally we memmap target memory so it is essentially >> zero initialized (except for e.g. option roms and bios which are loaded >> into target memory although they shouldn't). >> >> It was reported recently that this madvise causes a performance degradation >> in some situations. As the madvise should only be called rarely and if it's called >> it is likely on a busy page (it was non-zero and changed to zero during migration) >> drop it completely. > Reviewed-by: Juan Quintela <quintela@redhat.com> > > I take it. I am on KVM Forum/LinuxCon this week. Will send when back > at home. Ping Peter
On 24.10.2013 11:14, Paolo Bonzini wrote: > Il 24/10/2013 08:21, Peter Lieven ha scritto: >> Additionally we memmap target memory so it is essentially >> zero initialized (except for e.g. option roms and bios which are loaded >> into target memory although they shouldn't). >> >> It was reported recently that this madvise causes a performance degradation >> in some situations. As the madvise should only be called rarely and if it's called >> it is likely on a busy page (it was non-zero and changed to zero during migration) >> drop it completely. > Tagging this patch for 1.7. has this been merged? Peter
Il 18/11/2013 13:48, Peter Lieven ha scritto: > On 24.10.2013 11:14, Paolo Bonzini wrote: >> Il 24/10/2013 08:21, Peter Lieven ha scritto: >>> Additionally we memmap target memory so it is essentially >>> zero initialized (except for e.g. option roms and bios which are loaded >>> into target memory although they shouldn't). >>> >>> It was reported recently that this madvise causes a performance >>> degradation >>> in some situations. As the madvise should only be called rarely and >>> if it's called >>> it is likely on a busy page (it was non-zero and changed to zero >>> during migration) >>> drop it completely. >> Tagging this patch for 1.7. > has this been merged? No. Juan, can you prepare a quick pull request? Paolo
diff --git a/arch_init.c b/arch_init.c index 7545d96..e0acbc5 100644 --- a/arch_init.c +++ b/arch_init.c @@ -850,14 +850,6 @@ void ram_handle_compressed(void *host, uint8_t ch, uint64_t size) { if (ch != 0 || !is_zero_range(host, size)) { memset(host, ch, size); -#ifndef _WIN32 - if (ch == 0 && (!kvm_enabled() || kvm_has_sync_mmu())) { - size = size & ~(getpagesize() - 1); - if (size > 0) { - qemu_madvise(host, size, QEMU_MADV_DONTNEED); - } - } -#endif } }