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

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Apr 28, 2015 at 05:58:28PM +0800, Jason Wang wrote:
> 
> 
> On Tue, Apr 28, 2015 at 5:37 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >On Fri, Apr 10, 2015 at 05:33:35PM +0800, Jason Wang wrote:
> >> Currently 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
> >> In the above cases, we can avoid the memory allocation by sharing a
> >> single vhost log among all the vhost devices. This is done through:
> >> - Introducing a new vhost_log structure with refcnt inside.
> >> - Using a global pointer to vhost_log structure that will be used. And
> >>   introduce helper to get the log with expected log size and helper to
> >> - drop the refcnt to the old log.
> >> - Each vhost device still keep track of a pointer to the log that was
> >>   used.
> >>   With above, if no resize happens, all vhost device will share a
> >>single
> >> vhost log. During resize, a new vhost_log structure will be allocated
> >> and made for the global pointer. And each vhost devices will drop the
> >> refcnt to the old log.
> >> Tested by doing scp during migration for a 2 queues virtio-net-pci.
> >> Cc: Michael S. Tsirkin <mst@redhat.com>
> >> Signed-off-by: Jason Wang <jasowang@redhat.com>
> >> ---
> >> Changes from V1:
> >> - Drop the list of vhost log, instead, using a global pointer instead
> >
> >I don't think it works like this. If you have a global pointer,
> >you also need a global listener, have that sync all devices.
> 
> It doesn't conflict, see my comments below.
> >
> >
> >
> >> ---
> >>  hw/virtio/vhost.c         | 66
> >>++++++++++++++++++++++++++++++++++++++---------
> >>  include/hw/virtio/vhost.h |  9 ++++++-
> >>  2 files changed, 62 insertions(+), 13 deletions(-)
> >> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> >> index 5a12861..e16c2db 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,55 @@ 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);
> >
> >This just leaks the old log if size != size.
> 
> But old log is reference counted and will be freed during vhost_log_put() if
> refcnt drops to zero.

You need a pointer to reference-count it.
You return pointer to new object, no one references the old one.

> >
> >> +    } else {
> >> +        ++vhost_log->refcnt;
> >> +    }
> >> +
> >> +    return vhost_log;
> >> +}
> >> +
> >> +static void vhost_log_put(struct vhost_log *log)
> >> +{
> >> +    if (!log) {
> >> +        return;
> >> +    }
> >> +
> >> +    --log->refcnt;
> >> +    if (log->refcnt == 0) {
> >> +        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);
> >
> >At this point next device will try to use the
> >new log, but there is nothing there.
> 
> vhost_log_get() will either allocate and return a new log if size is change
> or just increase the refcnt and use current log. So it works in fact?

But old log is lost and is not synced.

> >
> >
> >> +    uint64_t log_base = (uint64_t)(unsigned long)log->log;
> >>      int r;
> >> -    log = g_malloc0(size * sizeof *log);
> >> -    log_base = (uint64_t)(unsigned long)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->log);
> >>      dev->log = log;
> >>      dev->log_size = size;
> >>  }
> >> @@ -601,7 +638,7 @@ static int vhost_migration_log(MemoryListener
> >>*listener, int enable)
> >>          if (r < 0) {
> >>              return r;
> >>          }
> >> -        g_free(dev->log);
> >> +        vhost_log_put(dev->log);
> >>          dev->log = NULL;
> >>          dev->log_size = 0;
> >>      } else {
> >> @@ -1058,9 +1095,11 @@ int vhost_dev_start(struct vhost_dev *hdev,
> >>VirtIODevice *vdev)
> >>      if (hdev->log_enabled) {
> >>          hdev->log_size = vhost_get_log_size(hdev);
> >> -        hdev->log = hdev->log_size ?
> >> -            g_malloc0(hdev->log_size * sizeof *hdev->log) : NULL;
> >> -        r = hdev->vhost_ops->vhost_call(hdev, VHOST_SET_LOG_BASE,
> >>hdev->log);
> >> +        if (hdev->log_size) {
> >> +            hdev->log = vhost_log_get(hdev->log_size);
> >> +        }
> >> +        r = hdev->vhost_ops->vhost_call(hdev, VHOST_SET_LOG_BASE,
> >> +                                        hdev->log_size ?
> >>hdev->log->log : NULL);
> >>          if (r < 0) {
> >>              r = -errno;
> >>              goto fail_log;
> >> @@ -1069,6 +1108,9 @@ int vhost_dev_start(struct vhost_dev *hdev,
> >>VirtIODevice *vdev)
> >>      return 0;
> >>  fail_log:
> >> +    if (hdev->log_size) {
> >> +        vhost_log_put(hdev->log);
> >> +    }
> >>  fail_vq:
> >>      while (--i >= 0) {
> >>          vhost_virtqueue_stop(hdev,
> >> @@ -1098,7 +1140,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->log);
> >>      hdev->log = NULL;
> >>      hdev->log_size = 0;
> >>  }
> >> diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> >> index d5593d1..3879778 100644
> >> --- a/include/hw/virtio/vhost.h
> >> +++ b/include/hw/virtio/vhost.h
> >> @@ -28,6 +28,13 @@ 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 {
> >> +    QLIST_ENTRY(vhost_log) node;
> >> +    unsigned long long size;
> >> +    int refcnt;
> >> +    vhost_log_chunk_t log[0];
> >> +};
> >> +
> >
> >Pls fix coding style here: typedef ... VhostLog.
> 
> Is this a new style requirement? (I'm asking since e.g the blow vhost_dev
> structure does not have a typedef)

You are right here, you can keep it as is for now.

> >
> >
> >>  struct vhost_memory;
> >>  struct vhost_dev {
> >>      MemoryListener memory_listener;
> >> @@ -43,7 +50,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 +58,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,
> >> --  2.1.0