Re: [Qemu-devel] [PATCH v4 1/6] vhost: Move log_dirty check

[Igor Mammedov <imammedo@redhat.com>]

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