|
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 devicesIn the above cases, we can avoid the memory allocation by sharing asingle 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 wasused.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 therefcnt 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 insteadI 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.cindex 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
[Prev in Thread] | Current Thread | [Next in Thread] |