On Tue, Apr 28, 2015 at 05:58:28PM +0800, Jason Wang wrote:
>
>
> On Tue, Apr 28, 2015 at 5:37 PM, Michael S. Tsirkin <m...@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 <m...@redhat.com>
> >> Signed-off-by: Jason Wang <jasow...@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.
You need a pointer to reference-count it.
You return pointer to new object, no one references the old one.
> >
> >> + } 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?
But old log is lost and is not synced.
> >
> >
> >> + 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)
You are right here, you can keep it as is for now.
> >
> >
> >> 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
