[Qemu-devel] [RESEND][PATCH] migration: drop MADVISE_DONT_NEED for incoming zero pages

[Qemu-devel] [RESEND][PATCH] migration: drop MADVISE_DONT_NEED for incoming zero pages

[Peter Lieven]

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

Re: [Qemu-devel] [RESEND][PATCH 1.7] migration: drop MADVISE_DONT_NEED for incoming zero pages

[Paolo Bonzini]

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

Re: [Qemu-devel] [RESEND][PATCH] migration: drop MADVISE_DONT_NEED for incoming zero pages

[Juan Quintela]

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
>      }
>  }

Re: [Qemu-devel] [RESEND][PATCH] migration: drop MADVISE_DONT_NEED for incoming zero pages

[Zhanghaoyu (A)]

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
> 

Re: [Qemu-devel] [RESEND][PATCH] migration: drop MADVISE_DONT_NEED for incoming zero pages

[Peter Lieven]

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

Re: [Qemu-devel] [RESEND][PATCH 1.7] migration: drop MADVISE_DONT_NEED for incoming zero pages

[Peter Lieven]

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

Re: [Qemu-devel] [RESEND][PATCH 1.7] migration: drop MADVISE_DONT_NEED for incoming zero pages

[Paolo Bonzini]

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