Re: [Qemu-devel] [PATCH V3] vhost: logs sharing

[Michael S. Tsirkin]

On Mon, Jun 01, 2015 at 01:53:35PM +0800, Jason Wang wrote:
> 
> 
> On 06/01/2015 02:12 AM, Michael S. Tsirkin wrote:
> > On Tue, May 26, 2015 at 01:54:09AM -0400, Jason Wang wrote:
> >> We allocate one vhost log per vhost device. This is sub optimal when:
> >>
> >> - Guest has several device with vhost as backend
> >> - Guest has multiqueue devices
> >>
> >> Since each vhost device will allocate and use their private log, this
> >> could cause very huge unnecessary memory allocation. We can in fact
> >> avoid extra cost by sharing a single vhost log among all the vhost
> >> devices. This is feasible since:
> >>
> >> - All vhost devices for a vm should see a consistent memory layout
> >>   (memory slots).
> >> - Vhost log were design to be shared, all access function were
> >>   synchronized.
> >>
> >> So this patch tries to implement the sharing through
> >>
> >> - Introducing a new vhost_log structure and refcnt it.
> >> - Using a global pointer of vhost_log structure to keep track the
> >>   current log used.
> >> - If there's no resize, next vhost device will just use this log and
> >>   increase the refcnt.
> >> - If resize happens, a new vhost_log structure will be allocated and
> >>   each vhost device will use the new log then drop the refcnt of old log.
> >> - The old log will be synced and freed when reference count drops to zero.
> >>
> >> Tested by doing scp during migration for a 2 queues virtio-net-pci.
> >>
> >> Cc: Michael S. Tsirkin <address@hidden>
> >> Signed-off-by: Jason Wang <address@hidden>
> > Almost there I think.
> >
> >> ---
> >> Changes from V3:
> >> - only sync the old log on put
> >> Changes from V2:
> >> - rebase to HEAD
> >> - drop unused node field from vhost_log structure
> >> Changes from V1:
> >> - Drop the list of vhost log, instead, using a global pointer instead
> >> ---
> >>  hw/virtio/vhost.c         | 78 
> >> ++++++++++++++++++++++++++++++++++++-----------
> >>  include/hw/virtio/vhost.h |  8 ++++-
> >>  2 files changed, 68 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> >> index 54851b7..fef28d9 100644
> >> --- a/hw/virtio/vhost.c
> >> +++ b/hw/virtio/vhost.c
> >> @@ -22,15 +22,19 @@
> >>  #include "hw/virtio/virtio-bus.h"
> >>  #include "migration/migration.h"
> >>  
> >> +static struct vhost_log *vhost_log;
> >> +
> >>  static void vhost_dev_sync_region(struct vhost_dev *dev,
> >>                                    MemoryRegionSection *section,
> >>                                    uint64_t mfirst, uint64_t mlast,
> >>                                    uint64_t rfirst, uint64_t rlast)
> >>  {
> >> +    vhost_log_chunk_t *log = dev->log->log;
> >> +
> >>      uint64_t start = MAX(mfirst, rfirst);
> >>      uint64_t end = MIN(mlast, rlast);
> >> -    vhost_log_chunk_t *from = dev->log + start / VHOST_LOG_CHUNK;
> >> -    vhost_log_chunk_t *to = dev->log + end / VHOST_LOG_CHUNK + 1;
> >> +    vhost_log_chunk_t *from = log + start / VHOST_LOG_CHUNK;
> >> +    vhost_log_chunk_t *to = log + end / VHOST_LOG_CHUNK + 1;
> >>      uint64_t addr = (start / VHOST_LOG_CHUNK) * VHOST_LOG_CHUNK;
> >>  
> >>      if (end < start) {
> >> @@ -280,22 +284,57 @@ static uint64_t vhost_get_log_size(struct vhost_dev 
> >> *dev)
> >>      }
> >>      return log_size;
> >>  }
> >> +static struct vhost_log *vhost_log_alloc(uint64_t size)
> >> +{
> >> +    struct vhost_log *log = g_malloc0(sizeof *log + size * 
> >> sizeof(*(log->log)));
> >> +
> >> +    log->size = size;
> >> +    log->refcnt = 1;
> >> +
> >> +    return log;
> >> +}
> >> +
> >> +static struct vhost_log *vhost_log_get(uint64_t size)
> >> +{
> >> +    if (!vhost_log || vhost_log->size != size) {
> >> +        vhost_log = vhost_log_alloc(size);
> >> +    } else {
> >> +        ++vhost_log->refcnt;
> >> +    }
> >> +
> >> +    return vhost_log;
> >> +}
> >> +
> >> +static void vhost_log_put(struct vhost_dev *dev, bool sync)
> >> +{
> >> +    struct vhost_log *log = dev->log;
> >> +
> >> +    if (!log) {
> >> +        return;
> >> +    }
> >> +
> >> +    --log->refcnt;
> >> +    if (log->refcnt == 0) {
> >> +        /* Sync only the range covered by the old log */
> >> +        if (dev->log_size && sync) {
> >> +            vhost_log_sync_range(dev, 0, dev->log_size * VHOST_LOG_CHUNK 
> >> - 1);
> >> +        }
> >> +        if (vhost_log == log) {
> >> +            vhost_log = NULL;
> >> +        }
> >> +        g_free(log);
> >> +    }
> >> +}
> >>  
> >>  static inline void vhost_dev_log_resize(struct vhost_dev* dev, uint64_t 
> >> size)
> >>  {
> >> -    vhost_log_chunk_t *log;
> >> -    uint64_t log_base;
> >> +    struct vhost_log *log = vhost_log_get(size);
> >> +    uint64_t log_base = (uintptr_t)log->log;
> >>      int r;
> >>  
> >> -    log = g_malloc0(size * sizeof *log);
> >> -    log_base = (uintptr_t)log;
> >>      r = dev->vhost_ops->vhost_call(dev, VHOST_SET_LOG_BASE, &log_base);
> >>      assert(r >= 0);
> >> -    /* Sync only the range covered by the old log */
> >> -    if (dev->log_size) {
> >> -        vhost_log_sync_range(dev, 0, dev->log_size * VHOST_LOG_CHUNK - 1);
> >> -    }
> >> -    g_free(dev->log);
> >> +    vhost_log_put(dev, true);
> >>      dev->log = log;
> >>      dev->log_size = size;
> >>  }
> >> @@ -601,7 +640,7 @@ static int vhost_migration_log(MemoryListener 
> >> *listener, int enable)
> >>          if (r < 0) {
> >>              return r;
> >>          }
> >> -        g_free(dev->log);
> >> +        vhost_log_put(dev, false);
> >>          dev->log = NULL;
> >>          dev->log_size = 0;
> >>      } else {
> >> @@ -1060,10 +1099,12 @@ int vhost_dev_start(struct vhost_dev *hdev, 
> >> VirtIODevice *vdev)
> >>          uint64_t log_base;
> >>  
> >>          hdev->log_size = vhost_get_log_size(hdev);
> >> -        hdev->log = hdev->log_size ?
> >> -            g_malloc0(hdev->log_size * sizeof *hdev->log) : NULL;
> >> -        log_base = (uintptr_t)hdev->log;
> >> -        r = hdev->vhost_ops->vhost_call(hdev, VHOST_SET_LOG_BASE, 
> >> &log_base);
> >> +        if (hdev->log_size) {
> >> +            hdev->log = vhost_log_get(hdev->log_size);
> >> +        }
> > Why is this conditional on hdev->log_size?
> > What will the value be for log_size == 0?
> 
> This is used to save an unnecessary allocation for a dummy vhost_log
> structure without any log.

Then you need to go over all uses and make sure they
are safe. A dummy vhost_log structure might be easier.

> >> +        log_base = (uintptr_t)hdev->log->log;
> > especially since we de-reference it here.
> 
> Yes, this seems unsafe, will change this to
> 
> log_base = hdev->log_size ? (uintptr_t) hdev->log->log : NULL
> >> +        r = hdev->vhost_ops->vhost_call(hdev, VHOST_SET_LOG_BASE,
> >> +                                        hdev->log_size ? &log_base : 
> >> NULL);
> >>          if (r < 0) {
> >>              r = -errno;
> >>              goto fail_log;
> >> @@ -1072,6 +1113,9 @@ int vhost_dev_start(struct vhost_dev *hdev, 
> >> VirtIODevice *vdev)
> >>  
> >>      return 0;
> >>  fail_log:
> >> +    if (hdev->log_size) {
> >> +        vhost_log_put(hdev, false);
> >> +    }
> >>  fail_vq:
> >>      while (--i >= 0) {
> >>          vhost_virtqueue_stop(hdev,
> >> @@ -1101,7 +1145,7 @@ void vhost_dev_stop(struct vhost_dev *hdev, 
> >> VirtIODevice *vdev)
> >>      vhost_log_sync_range(hdev, 0, ~0x0ull);
> >>  
> >>      hdev->started = false;
> >> -    g_free(hdev->log);
> >> +    vhost_log_put(hdev, false);
> >>      hdev->log = NULL;
> >>      hdev->log_size = 0;
> >>  }
> > Why false? We better sync the dirty bitmap since log is getting
> > cleared.
> 
> We did a vhost_log_sync_range(hdev, 0, ~0x0ull) before. And we only sync
> 0 to dev->log_size * VHOST_LOG_CHUNK - 1 in vhost_log_put(). But looks
> like there's no difference, will remove vhost_log_sync_range() and use
> true for vhost_log_put() here.
> >
> > And if it's always true, we can just drop this flag.
> 
> There's still other usage, e.g when fail to setting log base in
> vhost_dev_start() or migration end. In those cases, no need to sync.

We definitely must sync on migration end.

> >
> >> diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> >> index 8f04888..816a2e8 100644
> >> --- a/include/hw/virtio/vhost.h
> >> +++ b/include/hw/virtio/vhost.h
> >> @@ -28,6 +28,12 @@ typedef unsigned long vhost_log_chunk_t;
> >>  #define VHOST_LOG_CHUNK (VHOST_LOG_PAGE * VHOST_LOG_BITS)
> >>  #define VHOST_INVALID_FEATURE_BIT   (0xff)
> >>  
> >> +struct vhost_log {
> >> +    unsigned long long size;
> >> +    int refcnt;
> >> +    vhost_log_chunk_t log[0];
> >> +};
> >> +
> >>  struct vhost_memory;
> >>  struct vhost_dev {
> >>      MemoryListener memory_listener;
> >> @@ -43,7 +49,6 @@ struct vhost_dev {
> >>      unsigned long long backend_features;
> >>      bool started;
> >>      bool log_enabled;
> >> -    vhost_log_chunk_t *log;
> >>      unsigned long long log_size;
> >>      Error *migration_blocker;
> >>      bool force;
> >> @@ -52,6 +57,7 @@ struct vhost_dev {
> >>      hwaddr mem_changed_end_addr;
> >>      const VhostOps *vhost_ops;
> >>      void *opaque;
> >> +    struct vhost_log *log;
> >>  };
> >>  
> >>  int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
> >> -- 
> >> 1.8.3.1
> >>