[V5] vhost: logs sharing

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 V4:
- leave a dummy vhost_log structure if log size is zero
- use vhost_log_put() to sync in vhost_dev_stop()
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         | 77 ++++++++++++++++++++++++++++++++++++-----------
 include/hw/virtio/vhost.h |  8 ++++-
 2 files changed, 66 insertions(+), 19 deletions(-)

On Thu, Jun 04, 2015 at 05:28:46AM -0400, 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 V4:
> - leave a dummy vhost_log structure if log size is zero
> - use vhost_log_put() to sync in vhost_dev_stop()
> 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         | 77 ++++++++++++++++++++++++++++++++++++-----------
>  include/hw/virtio/vhost.h |  8 ++++-
>  2 files changed, 66 insertions(+), 19 deletions(-)
> 
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 54851b7..01f1e04 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,10 @@ 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);
> +        hdev->log = vhost_log_get(hdev->log_size);
> +        log_base = (uintptr_t)hdev->log->log;
> +        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 +1111,9 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
>  
>      return 0;
>  fail_log:
> +    if (hdev->log_size) {

OK this one now can be unconditional, right?
I'll apply as is, pls fix with a patch on top.

> +        vhost_log_put(hdev, false);
> +    }
>  fail_vq:
>      while (--i >= 0) {
>          vhost_virtqueue_stop(hdev,
> @@ -1098,10 +1140,9 @@ void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev)
>                               hdev->vqs + i,
>                               hdev->vq_index + i);
>      }
> -    vhost_log_sync_range(hdev, 0, ~0x0ull);
>  
> +    vhost_log_put(hdev, true);
>      hdev->started = false;
> -    g_free(hdev->log);
>      hdev->log = NULL;
>      hdev->log_size = 0;
>  }
> 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

On 06/04/2015 06:44 PM, Michael S. Tsirkin wrote:
> On Thu, Jun 04, 2015 at 05:28:46AM -0400, 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 V4:
>> > - leave a dummy vhost_log structure if log size is zero
>> > - use vhost_log_put() to sync in vhost_dev_stop()
>> > 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         | 77 ++++++++++++++++++++++++++++++++++++-----------
>> >  include/hw/virtio/vhost.h |  8 ++++-
>> >  2 files changed, 66 insertions(+), 19 deletions(-)
>> > 
>> > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>> > index 54851b7..01f1e04 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,10 @@ 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);
>> > +        hdev->log = vhost_log_get(hdev->log_size);
>> > +        log_base = (uintptr_t)hdev->log->log;
>> > +        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 +1111,9 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
>> >  
>> >      return 0;
>> >  fail_log:
>> > +    if (hdev->log_size) {
> OK this one now can be unconditional, right?
> I'll apply as is, pls fix with a patch on top.
>

Right. Ok.