From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42663) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YcVeY-0001dI-NR for qemu-devel@nongnu.org; Mon, 30 Mar 2015 05:06:40 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YcVeV-0006jz-3m for qemu-devel@nongnu.org; Mon, 30 Mar 2015 05:06:38 -0400 Received: from mx1.redhat.com ([209.132.183.28]:43422) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YcVeU-0006jD-Tc for qemu-devel@nongnu.org; Mon, 30 Mar 2015 05:06:35 -0400 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t2U96Xv7029663 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Mon, 30 Mar 2015 05:06:34 -0400 Date: Mon, 30 Mar 2015 11:06:31 +0200 From: "Michael S. Tsirkin" Message-ID: <20150330104902-mutt-send-email-mst@redhat.com> References: <1426841604-23425-1-git-send-email-jasowang@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1426841604-23425-1-git-send-email-jasowang@redhat.com> Subject: Re: [Qemu-devel] [PATCH] vhost: logs sharing List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jason Wang Cc: qemu-devel@nongnu.org On Fri, Mar 20, 2015 at 04:53:24PM +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 by > introducing a global list for vhost logs and counting the reference > the usage. > > Tested by doing scp during migration for a 2 queues virtio-net-pci. > > Cc: Michael S. Tsirkin > Signed-off-by: Jason Wang > --- > hw/virtio/vhost.c | 67 ++++++++++++++++++++++++++++++++++++++--------- > include/hw/virtio/vhost.h | 9 ++++++- > 2 files changed, 63 insertions(+), 13 deletions(-) > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > index 5a12861..21d60cf 100644 > --- a/hw/virtio/vhost.c > +++ b/hw/virtio/vhost.c > @@ -22,15 +22,20 @@ > #include "hw/virtio/virtio-bus.h" > #include "migration/migration.h" > > +static QLIST_HEAD(, vhost_log) vhost_logs = > + QLIST_HEAD_INITIALIZER(vhost_logs); > + > 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 +285,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; > + QLIST_INSERT_HEAD(&vhost_logs, log, node); > + > + return log; > +} > + > +static struct vhost_log *vhost_log_get(uint64_t size) Needs comments - why do we look up log by size? > +{ > + struct vhost_log *log; > + > + QLIST_FOREACH(log, &vhost_logs, node) { > + if (log->size == size) { > + ++log->refcnt; > + return log; > + } > + } > + return vhost_log_alloc(size); > +} > + > +static void vhost_log_put(struct vhost_log *log) > +{ > + if (!log) > + return; > + > + --log->refcnt; > + if (log->refcnt == 0) { > + QLIST_REMOVE(log, node); > + 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 = (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 +639,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 +1096,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 +1109,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 +1141,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..57dd849 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 { This should be struct VHostLog, with a typedef. > + QLIST_ENTRY(vhost_log) node; Slightly ugly. There are at most 2 entries here, ever. Maybe we can do something to make this less ugly. > + int size; Can't size overflow 32 bit? > + int refcnt; > + vhost_log_chunk_t log[0]; > +}; > + > 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