qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Jason Wang
Subject: Re: [Qemu-devel] [PATCH V2] vhost: logs sharing
Date: Tue, 28 Apr 2015 17:58:28 +0800



On Tue, Apr 28, 2015 at 5:37 PM, Michael S. Tsirkin <address@hidden> 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 <address@hidden>
 Signed-off-by: Jason Wang <address@hidden>
 ---
 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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]