From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55424) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yn2PB-0007UF-Ud for qemu-devel@nongnu.org; Tue, 28 Apr 2015 06:07:31 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Yn2IS-0001jR-JZ for qemu-devel@nongnu.org; Tue, 28 Apr 2015 05:59:55 -0400 Received: from mx1.redhat.com ([209.132.183.28]:52965) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yn2IS-0001i3-8Y for qemu-devel@nongnu.org; Tue, 28 Apr 2015 05:59:20 -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 734A38E71A for ; Tue, 28 Apr 2015 09:59:19 +0000 (UTC) Date: Tue, 28 Apr 2015 17:58:28 +0800 From: Jason Wang Message-Id: <1430215108.5354.1@smtp.corp.redhat.com> In-Reply-To: <20150428113027-mutt-send-email-mst@redhat.com> References: <1428658415-5049-1-git-send-email-jasowang@redhat.com> <20150428113027-mutt-send-email-mst@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Subject: Re: [Qemu-devel] [PATCH V2] 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 Tue, Apr 28, 2015 at 5:37 PM, Michael S. Tsirkin 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 >> Signed-off-by: Jason Wang >> --- >> 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