From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33232) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z0htU-0008QT-3k for qemu-devel@nongnu.org; Thu, 04 Jun 2015 23:02:05 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z0htQ-0005RW-PA for qemu-devel@nongnu.org; Thu, 04 Jun 2015 23:02:04 -0400 Received: from mx1.redhat.com ([209.132.183.28]:49955) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z0htQ-0005RS-HD for qemu-devel@nongnu.org; Thu, 04 Jun 2015 23:02:00 -0400 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (Postfix) with ESMTPS id 7958E19F210 for ; Fri, 5 Jun 2015 03:01:59 +0000 (UTC) Message-ID: <55711124.9010109@redhat.com> Date: Fri, 05 Jun 2015 11:01:56 +0800 From: Jason Wang MIME-Version: 1.0 References: <1433410126-28787-1-git-send-email-jasowang@redhat.com> <20150604123017-mutt-send-email-mst@redhat.com> In-Reply-To: <20150604123017-mutt-send-email-mst@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH V5] vhost: logs sharing List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: qemu-devel@nongnu.org On 06/04/2015 06:44 PM, Michael S. Tsirkin wrote: > On Thu, Jun 04, 2015 at 05:28:46AM -0400, 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 >> > Signed-off-by: Jason Wang >> > --- >> > Changes from V4: >> > - leave a dummy vhost_log structure if log size is zero >> > - use vhost_log_put() to sync in vhost_dev_stop() >> > 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 | 77 ++++++++++++++++++++++++++++++++++++----------- >> > include/hw/virtio/vhost.h | 8 ++++- >> > 2 files changed, 66 insertions(+), 19 deletions(-) >> > >> > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c >> > index 54851b7..01f1e04 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,10 @@ 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); >> > + hdev->log = vhost_log_get(hdev->log_size); >> > + log_base = (uintptr_t)hdev->log->log; >> > + 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 +1111,9 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev) >> > >> > return 0; >> > fail_log: >> > + if (hdev->log_size) { > OK this one now can be unconditional, right? > I'll apply as is, pls fix with a patch on top. > Right. Ok.