| Message ID | 1342531805-29894-4-git-send-email-anthony.perard@citrix.com |
|---|---|
| State | New |
| Headers | show |
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?)
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.
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?
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.
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?
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.
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
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.
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
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,
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?
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.
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); }
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(-)