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

[Jason Wang <jasowang@redhat.com>]

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.
> 
>>  +    } 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?
> 
> 
>>  +    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)
> 
> 
>>   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