Patchwork [3/4] exec, memory: Call to xen_modified_memory.

login
register
mail settings
Submitter Anthony PERARD
Date July 17, 2012, 1:30 p.m.
Message ID <1342531805-29894-4-git-send-email-anthony.perard@citrix.com>
Download mbox | patch
Permalink /patch/171427/
State New
Headers show

Comments

Anthony PERARD - July 17, 2012, 1:30 p.m.
This patch add some calls to xen_modified_memory to notify Xen about dirtybits
during migration.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 exec.c   |    4 ++++
 memory.c |    2 ++
 2 files changed, 6 insertions(+), 0 deletions(-)
Avi Kivity - July 17, 2012, 1:37 p.m.
On 07/17/2012 04:30 PM, Anthony PERARD wrote:
> This patch add some calls to xen_modified_memory to notify Xen about dirtybits
> during migration.
> 
> diff --git a/exec.c b/exec.c
> index c9fa17d..9f7a4f7 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -3438,6 +3438,7 @@ void cpu_physical_memory_rw(target_phys_addr_t addr, uint8_t *buf,
>                      cpu_physical_memory_set_dirty_flags(
>                          addr1, (0xff & ~CODE_DIRTY_FLAG));
>                  }
> +                xen_modified_memory(addr1, TARGET_PAGE_SIZE);
>                  qemu_put_ram_ptr(ptr);
>              }
>          } else {

This is pretty ugly.  An alternative is to set up a periodic bitmap scan
that looks at the qemu dirty bitmap and calls xen_modified_memory() for
dirty page ranges, and clears the bitmap for the next pass.  Is it workable?

(is xen_modified_memory a hypercall, or does it maintain an in-memory
structure?)
Anthony PERARD - July 17, 2012, 1:59 p.m.
On 17/07/12 14:37, Avi Kivity wrote:
> On 07/17/2012 04:30 PM, Anthony PERARD wrote:
>> This patch add some calls to xen_modified_memory to notify Xen about dirtybits
>> during migration.
>>
>> diff --git a/exec.c b/exec.c
>> index c9fa17d..9f7a4f7 100644
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -3438,6 +3438,7 @@ void cpu_physical_memory_rw(target_phys_addr_t addr, uint8_t *buf,
>>                       cpu_physical_memory_set_dirty_flags(
>>                           addr1, (0xff & ~CODE_DIRTY_FLAG));
>>                   }
>> +                xen_modified_memory(addr1, TARGET_PAGE_SIZE);
>>                   qemu_put_ram_ptr(ptr);
>>               }
>>           } else {
>
> This is pretty ugly.  An alternative is to set up a periodic bitmap scan
> that looks at the qemu dirty bitmap and calls xen_modified_memory() for
> dirty page ranges, and clears the bitmap for the next pass.  Is it workable?

I don't think a periodic scan can do anything useful, unfortunately.

> (is xen_modified_memory a hypercall, or does it maintain an in-memory
> structure?)

It's an hypercall. The function do something (call the hypercall) only 
during migration, otherwise it return immediately.
Avi Kivity - July 17, 2012, 2:44 p.m.
On 07/17/2012 04:59 PM, Anthony PERARD wrote:
>>
>> This is pretty ugly.  An alternative is to set up a periodic bitmap scan
>> that looks at the qemu dirty bitmap and calls xen_modified_memory() for
>> dirty page ranges, and clears the bitmap for the next pass.  Is it
>> workable?
> 
> I don't think a periodic scan can do anything useful, unfortunately.

Why not?

>> (is xen_modified_memory a hypercall, or does it maintain an in-memory
>> structure?)
> 
> It's an hypercall. The function do something (call the hypercall) only
> during migration, otherwise it return immediately.

I see.  I guess it isn't expensive for you because there isn't much dma
done by qemu usually with xen (unlike kvm where pv block devices are
implemented in qemu).

How about pushing the call into cpu_physical_memory_set_dirty_flags()?
Would that reduce the number of call sites?
Stefano Stabellini - July 17, 2012, 6:06 p.m.
On Tue, 17 Jul 2012, Anthony PERARD wrote:
> This patch add some calls to xen_modified_memory to notify Xen about dirtybits
> during migration.
> 
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
>  exec.c   |    4 ++++
>  memory.c |    2 ++
>  2 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index c9fa17d..9f7a4f7 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -3438,6 +3438,7 @@ void cpu_physical_memory_rw(target_phys_addr_t addr, uint8_t *buf,
>                      cpu_physical_memory_set_dirty_flags(
>                          addr1, (0xff & ~CODE_DIRTY_FLAG));
>                  }
> +                xen_modified_memory(addr1, TARGET_PAGE_SIZE);
>                  qemu_put_ram_ptr(ptr);
>              }
>          } else {
> @@ -3623,6 +3624,7 @@ void cpu_physical_memory_unmap(void *buffer, target_phys_addr_t len,
>      if (buffer != bounce.buffer) {
>          if (is_write) {
>              ram_addr_t addr1 = qemu_ram_addr_from_host_nofail(buffer);
> +            xen_modified_memory(addr1, access_len);
>              while (access_len) {
>                  unsigned l;
>                  l = TARGET_PAGE_SIZE;

You need to add xen_modified_memory in cpu_physical_memory_map, rather
than cpu_physical_memory_unmap.
Stefano Stabellini - July 17, 2012, 6:36 p.m.
On Tue, 17 Jul 2012, Avi Kivity wrote:
> On 07/17/2012 04:59 PM, Anthony PERARD wrote:
> >>
> >> This is pretty ugly.  An alternative is to set up a periodic bitmap scan
> >> that looks at the qemu dirty bitmap and calls xen_modified_memory() for
> >> dirty page ranges, and clears the bitmap for the next pass.  Is it
> >> workable?
> > 
> > I don't think a periodic scan can do anything useful, unfortunately.
> 
> Why not?

I vaguely remember that we used to have a bitmap years ago, but, aside from
making the code much more complicated, it caused blue screens on
intensive disk accesses.


> >> (is xen_modified_memory a hypercall, or does it maintain an in-memory
> >> structure?)
> > 
> > It's an hypercall. The function do something (call the hypercall) only
> > during migration, otherwise it return immediately.
> 
> I see.  I guess it isn't expensive for you because there isn't much dma
> done by qemu usually with xen (unlike kvm where pv block devices are
> implemented in qemu).
> 
> How about pushing the call into cpu_physical_memory_set_dirty_flags()?
> Would that reduce the number of call sites?

Pushing the calls to cpu_physical_memory_set_dirty_flags and
cpu_physical_memory_set_dirty_range would make the code much nicer.
However being these functions in exec-obsolete.h, are they at risk of
removal?
Avi Kivity - July 18, 2012, 8:32 a.m.
On 07/17/2012 09:36 PM, Stefano Stabellini wrote:
> On Tue, 17 Jul 2012, Avi Kivity wrote:
>> On 07/17/2012 04:59 PM, Anthony PERARD wrote:
>> >>
>> >> This is pretty ugly.  An alternative is to set up a periodic bitmap scan
>> >> that looks at the qemu dirty bitmap and calls xen_modified_memory() for
>> >> dirty page ranges, and clears the bitmap for the next pass.  Is it
>> >> workable?
>> > 
>> > I don't think a periodic scan can do anything useful, unfortunately.
>> 
>> Why not?
> 
> I vaguely remember that we used to have a bitmap years ago, but, aside from
> making the code much more complicated, it caused blue screens on
> intensive disk accesses.

Surely it was some bug, not the scan itself.

> 
> 
>> >> (is xen_modified_memory a hypercall, or does it maintain an in-memory
>> >> structure?)
>> > 
>> > It's an hypercall. The function do something (call the hypercall) only
>> > during migration, otherwise it return immediately.
>> 
>> I see.  I guess it isn't expensive for you because there isn't much dma
>> done by qemu usually with xen (unlike kvm where pv block devices are
>> implemented in qemu).
>> 
>> How about pushing the call into cpu_physical_memory_set_dirty_flags()?
>> Would that reduce the number of call sites?
> 
> Pushing the calls to cpu_physical_memory_set_dirty_flags and
> cpu_physical_memory_set_dirty_range would make the code much nicer.
> However being these functions in exec-obsolete.h, are they at risk of
> removal?

exec-obsolete.h just means don't add new call sites.  The functions
won't be removed, instead they'll be absorbed into the memory code with
different names and different implementations.
Anthony PERARD - July 19, 2012, 11:41 a.m.
On 17/07/12 19:36, Stefano Stabellini wrote:
> On Tue, 17 Jul 2012, Avi Kivity wrote:
>> How about pushing the call into cpu_physical_memory_set_dirty_flags()?
>> Would that reduce the number of call sites?
>
> Pushing the calls to cpu_physical_memory_set_dirty_flags and
> cpu_physical_memory_set_dirty_range would make the code much nicer.
> However being these functions in exec-obsolete.h, are they at risk of
> removal?

I thought about it, but when I saw that set_dirty were called only when 
it was not already set as dirty where the call seams to be necessary.

I just try to call xen_modified_mem only within 
cpu_phy_mem_set_dirty_flags but it does not work, even when I tried to 
clear the dirtybits. But I maybe don't do the right thing yet to clear 
the dirty bits
Avi Kivity - July 19, 2012, 11:50 a.m.
On 07/19/2012 02:41 PM, Anthony PERARD wrote:
> On 17/07/12 19:36, Stefano Stabellini wrote:
>> On Tue, 17 Jul 2012, Avi Kivity wrote:
>>> How about pushing the call into cpu_physical_memory_set_dirty_flags()?
>>> Would that reduce the number of call sites?
>>
>> Pushing the calls to cpu_physical_memory_set_dirty_flags and
>> cpu_physical_memory_set_dirty_range would make the code much nicer.
>> However being these functions in exec-obsolete.h, are they at risk of
>> removal?
> 
> I thought about it, but when I saw that set_dirty were called only when
> it was not already set as dirty where the call seams to be necessary.
> 
> I just try to call xen_modified_mem only within
> cpu_phy_mem_set_dirty_flags but it does not work, even when I tried to
> clear the dirtybits. But I maybe don't do the right thing yet to clear
> the dirty bits

You can wrap the if (not dirty) make_it_dirty() sequence in a helper,
and insert your hypercall in the helper, unconditionally.
Paolo Bonzini - July 19, 2012, 12:34 p.m.
Il 17/07/2012 20:06, Stefano Stabellini ha scritto:
> On Tue, 17 Jul 2012, Anthony PERARD wrote:
>> This patch add some calls to xen_modified_memory to notify Xen about dirtybits
>> during migration.
>>
>> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
>> ---
>>  exec.c   |    4 ++++
>>  memory.c |    2 ++
>>  2 files changed, 6 insertions(+), 0 deletions(-)
>>
>> diff --git a/exec.c b/exec.c
>> index c9fa17d..9f7a4f7 100644
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -3438,6 +3438,7 @@ void cpu_physical_memory_rw(target_phys_addr_t addr, uint8_t *buf,
>>                      cpu_physical_memory_set_dirty_flags(
>>                          addr1, (0xff & ~CODE_DIRTY_FLAG));
>>                  }
>> +                xen_modified_memory(addr1, TARGET_PAGE_SIZE);
>>                  qemu_put_ram_ptr(ptr);
>>              }
>>          } else {
>> @@ -3623,6 +3624,7 @@ void cpu_physical_memory_unmap(void *buffer, target_phys_addr_t len,
>>      if (buffer != bounce.buffer) {
>>          if (is_write) {
>>              ram_addr_t addr1 = qemu_ram_addr_from_host_nofail(buffer);
>> +            xen_modified_memory(addr1, access_len);
>>              while (access_len) {
>>                  unsigned l;
>>                  l = TARGET_PAGE_SIZE;
> 
> You need to add xen_modified_memory in cpu_physical_memory_map, rather
> than cpu_physical_memory_unmap.

No, adding it to map is wrong, because the RAM save routine can migrate
(and mark as non-dirty) the read buffers _before_ the device models have
written to them.

Paolo
Anthony PERARD - July 19, 2012, 2:27 p.m.
On 19/07/12 12:50, Avi Kivity wrote:
> On 07/19/2012 02:41 PM, Anthony PERARD wrote:
>> On 17/07/12 19:36, Stefano Stabellini wrote:
>>> On Tue, 17 Jul 2012, Avi Kivity wrote:
>>>> How about pushing the call into cpu_physical_memory_set_dirty_flags()?
>>>> Would that reduce the number of call sites?
>>>
>>> Pushing the calls to cpu_physical_memory_set_dirty_flags and
>>> cpu_physical_memory_set_dirty_range would make the code much nicer.
>>> However being these functions in exec-obsolete.h, are they at risk of
>>> removal?
>>
>> I thought about it, but when I saw that set_dirty were called only when
>> it was not already set as dirty where the call seams to be necessary.
>>
>> I just try to call xen_modified_mem only within
>> cpu_phy_mem_set_dirty_flags but it does not work, even when I tried to
>> clear the dirtybits. But I maybe don't do the right thing yet to clear
>> the dirty bits
>
> You can wrap the if (not dirty) make_it_dirty() sequence in a helper,
> and insert your hypercall in the helper, unconditionally.

Ok, I'll do that.

Thanks,
Stefano Stabellini - July 19, 2012, 3:37 p.m.
On Thu, 19 Jul 2012, Paolo Bonzini wrote:
> Il 17/07/2012 20:06, Stefano Stabellini ha scritto:
> > On Tue, 17 Jul 2012, Anthony PERARD wrote:
> >> This patch add some calls to xen_modified_memory to notify Xen about dirtybits
> >> during migration.
> >>
> >> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> >> ---
> >>  exec.c   |    4 ++++
> >>  memory.c |    2 ++
> >>  2 files changed, 6 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/exec.c b/exec.c
> >> index c9fa17d..9f7a4f7 100644
> >> --- a/exec.c
> >> +++ b/exec.c
> >> @@ -3438,6 +3438,7 @@ void cpu_physical_memory_rw(target_phys_addr_t addr, uint8_t *buf,
> >>                      cpu_physical_memory_set_dirty_flags(
> >>                          addr1, (0xff & ~CODE_DIRTY_FLAG));
> >>                  }
> >> +                xen_modified_memory(addr1, TARGET_PAGE_SIZE);
> >>                  qemu_put_ram_ptr(ptr);
> >>              }
> >>          } else {
> >> @@ -3623,6 +3624,7 @@ void cpu_physical_memory_unmap(void *buffer, target_phys_addr_t len,
> >>      if (buffer != bounce.buffer) {
> >>          if (is_write) {
> >>              ram_addr_t addr1 = qemu_ram_addr_from_host_nofail(buffer);
> >> +            xen_modified_memory(addr1, access_len);
> >>              while (access_len) {
> >>                  unsigned l;
> >>                  l = TARGET_PAGE_SIZE;
> > 
> > You need to add xen_modified_memory in cpu_physical_memory_map, rather
> > than cpu_physical_memory_unmap.
> 
> No, adding it to map is wrong, because the RAM save routine can migrate
> (and mark as non-dirty) the read buffers _before_ the device models have
> written to them.

You are correct, in fact this looks like a bug in the current qemu-xen
(non-upstream) codebase too!

What I think that we should do is only mark the memory as dirty
if(is_write) in cpu_physical_memory_unmap, like you are doing in this
patch.

Anthony, can you write a patch to change the behavior in
qemu-xen-traditional too?
Anthony PERARD - July 19, 2012, 3:41 p.m.
On Thu, Jul 19, 2012 at 4:37 PM, Stefano Stabellini
<stefano.stabellini@eu.citrix.com> wrote:
>
> Anthony, can you write a patch to change the behavior in
> qemu-xen-traditional too?

Yes.

Patch

diff --git a/exec.c b/exec.c
index c9fa17d..9f7a4f7 100644
--- a/exec.c
+++ b/exec.c
@@ -3438,6 +3438,7 @@  void cpu_physical_memory_rw(target_phys_addr_t addr, uint8_t *buf,
                     cpu_physical_memory_set_dirty_flags(
                         addr1, (0xff & ~CODE_DIRTY_FLAG));
                 }
+                xen_modified_memory(addr1, TARGET_PAGE_SIZE);
                 qemu_put_ram_ptr(ptr);
             }
         } else {
@@ -3623,6 +3624,7 @@  void cpu_physical_memory_unmap(void *buffer, target_phys_addr_t len,
     if (buffer != bounce.buffer) {
         if (is_write) {
             ram_addr_t addr1 = qemu_ram_addr_from_host_nofail(buffer);
+            xen_modified_memory(addr1, access_len);
             while (access_len) {
                 unsigned l;
                 l = TARGET_PAGE_SIZE;
@@ -3947,6 +3949,7 @@  static inline void stl_phys_internal(target_phys_addr_t addr, uint32_t val,
             cpu_physical_memory_set_dirty_flags(addr1,
                 (0xff & ~CODE_DIRTY_FLAG));
         }
+        xen_modified_memory(addr1, 4);
     }
 }
 
@@ -4020,6 +4023,7 @@  static inline void stw_phys_internal(target_phys_addr_t addr, uint32_t val,
             cpu_physical_memory_set_dirty_flags(addr1,
                 (0xff & ~CODE_DIRTY_FLAG));
         }
+        xen_modified_memory(addr1, 2);
     }
 }
 
diff --git a/memory.c b/memory.c
index aab4a31..4d004e2 100644
--- a/memory.c
+++ b/memory.c
@@ -19,6 +19,7 @@ 
 #include "bitops.h"
 #include "kvm.h"
 #include <assert.h>
+#include "hw/xen.h"
 
 #define WANT_EXEC_OBSOLETE
 #include "exec-obsolete.h"
@@ -1085,6 +1086,7 @@  void memory_region_set_dirty(MemoryRegion *mr, target_phys_addr_t addr,
                              target_phys_addr_t size)
 {
     assert(mr->terminates);
+    xen_modified_memory(mr->ram_addr + addr, size);
     return cpu_physical_memory_set_dirty_range(mr->ram_addr + addr, size, -1);
 }