| Message ID | 20171213180807.7066-2-dgilbert@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Rework vhost memory region updates | expand |
On Wed, 13 Dec 2017 18:08:02 +0000 "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote: > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > Move the log_dirty check into vhost_section. > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > --- > hw/virtio/trace-events | 3 +++ > hw/virtio/vhost.c | 20 +++++++++++++------- > 2 files changed, 16 insertions(+), 7 deletions(-) > > diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events > index 775461ae98..4a493bcd46 100644 > --- a/hw/virtio/trace-events > +++ b/hw/virtio/trace-events > @@ -1,5 +1,8 @@ > # See docs/devel/tracing.txt for syntax documentation. > > +# hw/virtio/vhost.c > +vhost_section(const char *name, int r) "%s:%d" > + > # hw/virtio/virtio.c > virtqueue_alloc_element(void *elem, size_t sz, unsigned in_num, unsigned out_num) "elem %p size %zd in_num %u out_num %u" > virtqueue_fill(void *vq, const void *elem, unsigned int len, unsigned int idx) "vq %p elem %p len %u idx %u" > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > index e4290ce93d..e923219e63 100644 > --- a/hw/virtio/vhost.c > +++ b/hw/virtio/vhost.c > @@ -27,6 +27,7 @@ > #include "hw/virtio/virtio-access.h" > #include "migration/blocker.h" > #include "sysemu/dma.h" > +#include "trace.h" > > /* enabled until disconnected backend stabilizes */ > #define _VHOST_DEBUG 1 > @@ -567,18 +568,12 @@ static void vhost_set_memory(MemoryListener *listener, > memory_listener); > hwaddr start_addr = section->offset_within_address_space; > ram_addr_t size = int128_get64(section->size); > - bool log_dirty = > - memory_region_get_dirty_log_mask(section->mr) & ~(1 << DIRTY_MEMORY_MIGRATION); > int s = offsetof(struct vhost_memory, regions) + > (dev->mem->nregions + 1) * sizeof dev->mem->regions[0]; > void *ram; > > dev->mem = g_realloc(dev->mem, s); > > - if (log_dirty) { > - add = false; > - } > - > assert(size); > > /* Optimize no-change case. At least cirrus_vga does this a lot at this time. */ > @@ -611,8 +606,19 @@ static void vhost_set_memory(MemoryListener *listener, > > static bool vhost_section(MemoryRegionSection *section) > { > - return memory_region_is_ram(section->mr) && > + bool result; > + bool log_dirty = memory_region_get_dirty_log_mask(section->mr) & > + ~(1 << DIRTY_MEMORY_MIGRATION); > + result = memory_region_is_ram(section->mr) && > !memory_region_is_rom(section->mr); > + > + /* Vhost doesn't handle any block which is doing dirty-tracking other > + * than migration; this typically fires on VGA areas. > + */ > + result &= !log_dirty; before patch even if log_dirty, vhost_set_memory will still proceed and may remove dirty section from memmap and set memory_changed = true but with this patch it will just ignore such section, I'm not sure it's right (it might be right with new approach add/nop but then this patch should go after new code in place and old one if gone). > + > + trace_vhost_section(section->mr->name, result); > + return result; > } > > static void vhost_begin(MemoryListener *listener)
* Igor Mammedov (imammedo@redhat.com) wrote: > On Wed, 13 Dec 2017 18:08:02 +0000 > "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote: > > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > > > Move the log_dirty check into vhost_section. > > > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > --- > > hw/virtio/trace-events | 3 +++ > > hw/virtio/vhost.c | 20 +++++++++++++------- > > 2 files changed, 16 insertions(+), 7 deletions(-) > > > > diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events > > index 775461ae98..4a493bcd46 100644 > > --- a/hw/virtio/trace-events > > +++ b/hw/virtio/trace-events > > @@ -1,5 +1,8 @@ > > # See docs/devel/tracing.txt for syntax documentation. > > > > +# hw/virtio/vhost.c > > +vhost_section(const char *name, int r) "%s:%d" > > + > > # hw/virtio/virtio.c > > virtqueue_alloc_element(void *elem, size_t sz, unsigned in_num, unsigned out_num) "elem %p size %zd in_num %u out_num %u" > > virtqueue_fill(void *vq, const void *elem, unsigned int len, unsigned int idx) "vq %p elem %p len %u idx %u" > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > > index e4290ce93d..e923219e63 100644 > > --- a/hw/virtio/vhost.c > > +++ b/hw/virtio/vhost.c > > @@ -27,6 +27,7 @@ > > #include "hw/virtio/virtio-access.h" > > #include "migration/blocker.h" > > #include "sysemu/dma.h" > > +#include "trace.h" > > > > /* enabled until disconnected backend stabilizes */ > > #define _VHOST_DEBUG 1 > > @@ -567,18 +568,12 @@ static void vhost_set_memory(MemoryListener *listener, > > memory_listener); > > hwaddr start_addr = section->offset_within_address_space; > > ram_addr_t size = int128_get64(section->size); > > - bool log_dirty = > > - memory_region_get_dirty_log_mask(section->mr) & ~(1 << DIRTY_MEMORY_MIGRATION); > > int s = offsetof(struct vhost_memory, regions) + > > (dev->mem->nregions + 1) * sizeof dev->mem->regions[0]; > > void *ram; > > > > dev->mem = g_realloc(dev->mem, s); > > > > - if (log_dirty) { > > - add = false; > > - } > > - > > assert(size); > > > > /* Optimize no-change case. At least cirrus_vga does this a lot at this time. */ > > @@ -611,8 +606,19 @@ static void vhost_set_memory(MemoryListener *listener, > > > > static bool vhost_section(MemoryRegionSection *section) > > { > > - return memory_region_is_ram(section->mr) && > > + bool result; > > + bool log_dirty = memory_region_get_dirty_log_mask(section->mr) & > > + ~(1 << DIRTY_MEMORY_MIGRATION); > > + result = memory_region_is_ram(section->mr) && > > !memory_region_is_rom(section->mr); > > + > > + /* Vhost doesn't handle any block which is doing dirty-tracking other > > + * than migration; this typically fires on VGA areas. > > + */ > > + result &= !log_dirty; > before patch even if log_dirty, vhost_set_memory will still proceed > and may remove dirty section from memmap and set memory_changed = true > > but with this patch it will just ignore such section, > I'm not sure it's right (it might be right with new approach add/nop > but then this patch should go after new code in place and old one > if gone). I thought about that, but then I came to the conclusion that the whole idea is that we're supposed to be ignoring these regions - so why should they cause any change to the behaviour of vhost at all? Thus this way seems safer. Dave > > + > > + trace_vhost_section(section->mr->name, result); > > + return result; > > } > > > > static void vhost_begin(MemoryListener *listener) > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Thu, 14 Dec 2017 15:20:10 +0000 "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: > * Igor Mammedov (imammedo@redhat.com) wrote: > > On Wed, 13 Dec 2017 18:08:02 +0000 > > "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote: > > > > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > > > > > Move the log_dirty check into vhost_section. > > > > > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > > --- > > > hw/virtio/trace-events | 3 +++ > > > hw/virtio/vhost.c | 20 +++++++++++++------- > > > 2 files changed, 16 insertions(+), 7 deletions(-) > > > > > > diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events > > > index 775461ae98..4a493bcd46 100644 > > > --- a/hw/virtio/trace-events > > > +++ b/hw/virtio/trace-events > > > @@ -1,5 +1,8 @@ > > > # See docs/devel/tracing.txt for syntax documentation. > > > > > > +# hw/virtio/vhost.c > > > +vhost_section(const char *name, int r) "%s:%d" > > > + > > > # hw/virtio/virtio.c > > > virtqueue_alloc_element(void *elem, size_t sz, unsigned in_num, unsigned out_num) "elem %p size %zd in_num %u out_num %u" > > > virtqueue_fill(void *vq, const void *elem, unsigned int len, unsigned int idx) "vq %p elem %p len %u idx %u" > > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > > > index e4290ce93d..e923219e63 100644 > > > --- a/hw/virtio/vhost.c > > > +++ b/hw/virtio/vhost.c > > > @@ -27,6 +27,7 @@ > > > #include "hw/virtio/virtio-access.h" > > > #include "migration/blocker.h" > > > #include "sysemu/dma.h" > > > +#include "trace.h" > > > > > > /* enabled until disconnected backend stabilizes */ > > > #define _VHOST_DEBUG 1 > > > @@ -567,18 +568,12 @@ static void vhost_set_memory(MemoryListener *listener, > > > memory_listener); > > > hwaddr start_addr = section->offset_within_address_space; > > > ram_addr_t size = int128_get64(section->size); > > > - bool log_dirty = > > > - memory_region_get_dirty_log_mask(section->mr) & ~(1 << DIRTY_MEMORY_MIGRATION); > > > int s = offsetof(struct vhost_memory, regions) + > > > (dev->mem->nregions + 1) * sizeof dev->mem->regions[0]; > > > void *ram; > > > > > > dev->mem = g_realloc(dev->mem, s); > > > > > > - if (log_dirty) { > > > - add = false; > > > - } > > > - > > > assert(size); > > > > > > /* Optimize no-change case. At least cirrus_vga does this a lot at this time. */ > > > @@ -611,8 +606,19 @@ static void vhost_set_memory(MemoryListener *listener, > > > > > > static bool vhost_section(MemoryRegionSection *section) > > > { > > > - return memory_region_is_ram(section->mr) && > > > + bool result; > > > + bool log_dirty = memory_region_get_dirty_log_mask(section->mr) & > > > + ~(1 << DIRTY_MEMORY_MIGRATION); > > > + result = memory_region_is_ram(section->mr) && > > > !memory_region_is_rom(section->mr); > > > + > > > + /* Vhost doesn't handle any block which is doing dirty-tracking other > > > + * than migration; this typically fires on VGA areas. > > > + */ > > > + result &= !log_dirty; > > before patch even if log_dirty, vhost_set_memory will still proceed > > and may remove dirty section from memmap and set memory_changed = true > > > > but with this patch it will just ignore such section, > > I'm not sure it's right (it might be right with new approach add/nop > > but then this patch should go after new code in place and old one > > if gone). > > I thought about that, but then I came to the conclusion that the whole > idea is that we're supposed to be ignoring these regions - so why > should they cause any change to the behaviour of vhost at all? > Thus this way seems safer. Could it happen that, while being deleted section could become dirty at the same time? if that could happen then region_del won't process section and won't call unref on it either. one more reason to operate on tmp_mem_sections and then unconditionally unref old mem_sections at commit time at the right time (after set mem table). > > Dave > > > > + > > > + trace_vhost_section(section->mr->name, result); > > > + return result; > > > } > > > > > > static void vhost_begin(MemoryListener *listener) > > > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Thu, 14 Dec 2017 15:20:10 +0000 "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: > * Igor Mammedov (imammedo@redhat.com) wrote: > > On Wed, 13 Dec 2017 18:08:02 +0000 > > "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote: > > > > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > > > > > Move the log_dirty check into vhost_section. > > > > > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > > --- > > > hw/virtio/trace-events | 3 +++ > > > hw/virtio/vhost.c | 20 +++++++++++++------- > > > 2 files changed, 16 insertions(+), 7 deletions(-) > > > > > > diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events > > > index 775461ae98..4a493bcd46 100644 > > > --- a/hw/virtio/trace-events > > > +++ b/hw/virtio/trace-events > > > @@ -1,5 +1,8 @@ > > > # See docs/devel/tracing.txt for syntax documentation. > > > > > > +# hw/virtio/vhost.c > > > +vhost_section(const char *name, int r) "%s:%d" > > > + > > > # hw/virtio/virtio.c > > > virtqueue_alloc_element(void *elem, size_t sz, unsigned in_num, unsigned out_num) "elem %p size %zd in_num %u out_num %u" > > > virtqueue_fill(void *vq, const void *elem, unsigned int len, unsigned int idx) "vq %p elem %p len %u idx %u" > > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > > > index e4290ce93d..e923219e63 100644 > > > --- a/hw/virtio/vhost.c > > > +++ b/hw/virtio/vhost.c > > > @@ -27,6 +27,7 @@ > > > #include "hw/virtio/virtio-access.h" > > > #include "migration/blocker.h" > > > #include "sysemu/dma.h" > > > +#include "trace.h" > > > > > > /* enabled until disconnected backend stabilizes */ > > > #define _VHOST_DEBUG 1 > > > @@ -567,18 +568,12 @@ static void vhost_set_memory(MemoryListener *listener, > > > memory_listener); > > > hwaddr start_addr = section->offset_within_address_space; > > > ram_addr_t size = int128_get64(section->size); > > > - bool log_dirty = > > > - memory_region_get_dirty_log_mask(section->mr) & ~(1 << DIRTY_MEMORY_MIGRATION); > > > int s = offsetof(struct vhost_memory, regions) + > > > (dev->mem->nregions + 1) * sizeof dev->mem->regions[0]; > > > void *ram; > > > > > > dev->mem = g_realloc(dev->mem, s); > > > > > > - if (log_dirty) { > > > - add = false; > > > - } > > > - > > > assert(size); > > > > > > /* Optimize no-change case. At least cirrus_vga does this a lot at this time. */ > > > @@ -611,8 +606,19 @@ static void vhost_set_memory(MemoryListener *listener, > > > > > > static bool vhost_section(MemoryRegionSection *section) > > > { > > > - return memory_region_is_ram(section->mr) && > > > + bool result; > > > + bool log_dirty = memory_region_get_dirty_log_mask(section->mr) & > > > + ~(1 << DIRTY_MEMORY_MIGRATION); > > > + result = memory_region_is_ram(section->mr) && > > > !memory_region_is_rom(section->mr); > > > + > > > + /* Vhost doesn't handle any block which is doing dirty-tracking other > > > + * than migration; this typically fires on VGA areas. > > > + */ > > > + result &= !log_dirty; > > before patch even if log_dirty, vhost_set_memory will still proceed > > and may remove dirty section from memmap and set memory_changed = true > > > > but with this patch it will just ignore such section, > > I'm not sure it's right (it might be right with new approach add/nop > > but then this patch should go after new code in place and old one > > if gone). > > I thought about that, but then I came to the conclusion that the whole > idea is that we're supposed to be ignoring these regions - so why > should they cause any change to the behaviour of vhost at all? > Thus this way seems safer. the thing is that with originally with old code backend will use memap with not yet dirty region, then after region becomes dirty current code will REMOVE dirty region from map and NOTIFY backend. While this patch is fine for new approach as memmap is build from scratch, it doesn't look correct for current code since region that just became dirty will not be removed from memap and might be used backend. Whether it would really break something or not I'm not sure, maybe Michael may Ack this patch ii using dirty region is fine. > > Dave > > > > + > > > + trace_vhost_section(section->mr->name, result); > > > + return result; > > > } > > > > > > static void vhost_begin(MemoryListener *listener) > > > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
* Igor Mammedov (imammedo@redhat.com) wrote: > On Thu, 14 Dec 2017 15:20:10 +0000 > "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: > > > * Igor Mammedov (imammedo@redhat.com) wrote: > > > On Wed, 13 Dec 2017 18:08:02 +0000 > > > "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote: > > > > > > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > > > > > > > Move the log_dirty check into vhost_section. > > > > > > > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > > > --- > > > > hw/virtio/trace-events | 3 +++ > > > > hw/virtio/vhost.c | 20 +++++++++++++------- > > > > 2 files changed, 16 insertions(+), 7 deletions(-) > > > > > > > > diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events > > > > index 775461ae98..4a493bcd46 100644 > > > > --- a/hw/virtio/trace-events > > > > +++ b/hw/virtio/trace-events > > > > @@ -1,5 +1,8 @@ > > > > # See docs/devel/tracing.txt for syntax documentation. > > > > > > > > +# hw/virtio/vhost.c > > > > +vhost_section(const char *name, int r) "%s:%d" > > > > + > > > > # hw/virtio/virtio.c > > > > virtqueue_alloc_element(void *elem, size_t sz, unsigned in_num, unsigned out_num) "elem %p size %zd in_num %u out_num %u" > > > > virtqueue_fill(void *vq, const void *elem, unsigned int len, unsigned int idx) "vq %p elem %p len %u idx %u" > > > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > > > > index e4290ce93d..e923219e63 100644 > > > > --- a/hw/virtio/vhost.c > > > > +++ b/hw/virtio/vhost.c > > > > @@ -27,6 +27,7 @@ > > > > #include "hw/virtio/virtio-access.h" > > > > #include "migration/blocker.h" > > > > #include "sysemu/dma.h" > > > > +#include "trace.h" > > > > > > > > /* enabled until disconnected backend stabilizes */ > > > > #define _VHOST_DEBUG 1 > > > > @@ -567,18 +568,12 @@ static void vhost_set_memory(MemoryListener *listener, > > > > memory_listener); > > > > hwaddr start_addr = section->offset_within_address_space; > > > > ram_addr_t size = int128_get64(section->size); > > > > - bool log_dirty = > > > > - memory_region_get_dirty_log_mask(section->mr) & ~(1 << DIRTY_MEMORY_MIGRATION); > > > > int s = offsetof(struct vhost_memory, regions) + > > > > (dev->mem->nregions + 1) * sizeof dev->mem->regions[0]; > > > > void *ram; > > > > > > > > dev->mem = g_realloc(dev->mem, s); > > > > > > > > - if (log_dirty) { > > > > - add = false; > > > > - } > > > > - > > > > assert(size); > > > > > > > > /* Optimize no-change case. At least cirrus_vga does this a lot at this time. */ > > > > @@ -611,8 +606,19 @@ static void vhost_set_memory(MemoryListener *listener, > > > > > > > > static bool vhost_section(MemoryRegionSection *section) > > > > { > > > > - return memory_region_is_ram(section->mr) && > > > > + bool result; > > > > + bool log_dirty = memory_region_get_dirty_log_mask(section->mr) & > > > > + ~(1 << DIRTY_MEMORY_MIGRATION); > > > > + result = memory_region_is_ram(section->mr) && > > > > !memory_region_is_rom(section->mr); > > > > + > > > > + /* Vhost doesn't handle any block which is doing dirty-tracking other > > > > + * than migration; this typically fires on VGA areas. > > > > + */ > > > > + result &= !log_dirty; > > > before patch even if log_dirty, vhost_set_memory will still proceed > > > and may remove dirty section from memmap and set memory_changed = true > > > > > > but with this patch it will just ignore such section, > > > I'm not sure it's right (it might be right with new approach add/nop > > > but then this patch should go after new code in place and old one > > > if gone). > > > > I thought about that, but then I came to the conclusion that the whole > > idea is that we're supposed to be ignoring these regions - so why > > should they cause any change to the behaviour of vhost at all? > > Thus this way seems safer. > the thing is that with originally with old code backend will use memap > with not yet dirty region, then after region becomes dirty current code > will REMOVE dirty region from map and NOTIFY backend. > > While this patch is fine for new approach as memmap is build from scratch, > it doesn't look correct for current code since region that just became > dirty will not be removed from memap and might be used backend. > Whether it would really break something or not I'm not sure, > maybe Michael may Ack this patch ii using dirty region is fine. That's OK; I can add the check to vhost_region_add_section when I initially create it, and then swing this change around nearer to the end of the patchset, so that now we're using the new code. Dave > > > > Dave > > > > > > + > > > > + trace_vhost_section(section->mr->name, result); > > > > + return result; > > > > } > > > > > > > > static void vhost_begin(MemoryListener *listener) > > > > > -- > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events index 775461ae98..4a493bcd46 100644 --- a/hw/virtio/trace-events +++ b/hw/virtio/trace-events @@ -1,5 +1,8 @@ # See docs/devel/tracing.txt for syntax documentation. +# hw/virtio/vhost.c +vhost_section(const char *name, int r) "%s:%d" + # hw/virtio/virtio.c virtqueue_alloc_element(void *elem, size_t sz, unsigned in_num, unsigned out_num) "elem %p size %zd in_num %u out_num %u" virtqueue_fill(void *vq, const void *elem, unsigned int len, unsigned int idx) "vq %p elem %p len %u idx %u" diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c index e4290ce93d..e923219e63 100644 --- a/hw/virtio/vhost.c +++ b/hw/virtio/vhost.c @@ -27,6 +27,7 @@ #include "hw/virtio/virtio-access.h" #include "migration/blocker.h" #include "sysemu/dma.h" +#include "trace.h" /* enabled until disconnected backend stabilizes */ #define _VHOST_DEBUG 1 @@ -567,18 +568,12 @@ static void vhost_set_memory(MemoryListener *listener, memory_listener); hwaddr start_addr = section->offset_within_address_space; ram_addr_t size = int128_get64(section->size); - bool log_dirty = - memory_region_get_dirty_log_mask(section->mr) & ~(1 << DIRTY_MEMORY_MIGRATION); int s = offsetof(struct vhost_memory, regions) + (dev->mem->nregions + 1) * sizeof dev->mem->regions[0]; void *ram; dev->mem = g_realloc(dev->mem, s); - if (log_dirty) { - add = false; - } - assert(size); /* Optimize no-change case. At least cirrus_vga does this a lot at this time. */ @@ -611,8 +606,19 @@ static void vhost_set_memory(MemoryListener *listener, static bool vhost_section(MemoryRegionSection *section) { - return memory_region_is_ram(section->mr) && + bool result; + bool log_dirty = memory_region_get_dirty_log_mask(section->mr) & + ~(1 << DIRTY_MEMORY_MIGRATION); + result = memory_region_is_ram(section->mr) && !memory_region_is_rom(section->mr); + + /* Vhost doesn't handle any block which is doing dirty-tracking other + * than migration; this typically fires on VGA areas. + */ + result &= !log_dirty; + + trace_vhost_section(section->mr->name, result); + return result; } static void vhost_begin(MemoryListener *listener)