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

[Jason Wang]

On Mon, Mar 30, 2015 at 5:06 PM, Michael S. Tsirkin <address@hidden> 
wrote:
> On Fri, Mar 20, 2015 at 04:53:24PM +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 by
> >  introducing a global list for vhost logs and counting the reference
> >  the usage.
> >  
> >  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>
> >  ---
> >   hw/virtio/vhost.c         | 67 
> > ++++++++++++++++++++++++++++++++++++++---------
> >   include/hw/virtio/vhost.h |  9 ++++++-
> >   2 files changed, 63 insertions(+), 13 deletions(-)
> >  
> >  diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> >  index 5a12861..21d60cf 100644
> >  --- a/hw/virtio/vhost.c
> >  +++ b/hw/virtio/vhost.c
> >  @@ -22,15 +22,20 @@
> >   #include "hw/virtio/virtio-bus.h"
> >   #include "migration/migration.h"
> >   
> >  +static QLIST_HEAD(, vhost_log) vhost_logs =
> >  +      QLIST_HEAD_INITIALIZER(vhost_logs);
> >  +
> >   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 +285,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;
> >  +    QLIST_INSERT_HEAD(&vhost_logs, log, node);
> >  +
> >  +    return log;
> >  +}
> >  +
> >  +static struct vhost_log *vhost_log_get(uint64_t size)
>
> Needs comments - why do we look up log by size?

Because there may be more than one logs which were used by different 
vhost devices. The case is rare but happens during log resizing. In 
this case, when we have more than one listener for vhost, the resizing 
was done one by one. So two logs were in fact used at this time. One is 
used for vhost device that haven't finished the resizing. Another is 
used for vhost devices that have finished the resizing. The vhost 
devices whose size are equal could share a single log, so size is the 
only identifier here. 

> >  +{
> >  +    struct vhost_log *log;
> >  +
> >  +    QLIST_FOREACH(log, &vhost_logs, node) {
> >  +        if (log->size == size) {
> >  +            ++log->refcnt;
> >  +            return log;
> >  +        }
> >  +    }
> >  +    return vhost_log_alloc(size);
> >  +}
> >  +
> >  +static void vhost_log_put(struct vhost_log *log)
> >  +{
> >  +    if (!log)
> >  +        return;
> >  +
> >  +    --log->refcnt;
> >  +    if (log->refcnt == 0) {
> >  +        QLIST_REMOVE(log, node);
> >  +        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 = (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 +639,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 +1096,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 +1109,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 +1141,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..57dd849 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 {
>
> This should be struct VHostLog, with a typedef.

Well, I didn't see the typedefs for other structure like vhost_dev. Why 
is was only needed for vhost_log?
> >  +    QLIST_ENTRY(vhost_log) node;
>
> Slightly ugly. There are at most 2 entries here,
> ever. Maybe we can do something to make this less ugly.

Maybe, how about an array with just two elements?
> >  +    int size;
>
> Can't size overflow 32 bit?

Will correct this to unsigned long long.

Thanks
> >  +    int refcnt;
> >  +    vhost_log_chunk_t log[0];
> >  +};
> >  +
> >   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