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

[Jason Wang]

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.
>> +        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.
>
>> 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
>>