From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50463) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YnjTl-0005Gp-Uh for qemu-devel@nongnu.org; Thu, 30 Apr 2015 04:05:55 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YnjTg-0007cI-UL for qemu-devel@nongnu.org; Thu, 30 Apr 2015 04:05:53 -0400 Received: from mx1.redhat.com ([209.132.183.28]:46878) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YnjTg-0007bz-Mp for qemu-devel@nongnu.org; Thu, 30 Apr 2015 04:05:48 -0400 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (Postfix) with ESMTPS id 94C9BA10C0 for ; Thu, 30 Apr 2015 08:05:47 +0000 (UTC) Date: Thu, 30 Apr 2015 16:05:09 +0800 From: Jason Wang Message-Id: <1430381109.20407.1@smtp.corp.redhat.com> In-Reply-To: <20150428122611-mutt-send-email-mst@redhat.com> References: <1428658415-5049-1-git-send-email-jasowang@redhat.com> <20150428113027-mutt-send-email-mst@redhat.com> <1430215108.5354.1@smtp.corp.redhat.com> <20150428122611-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 6:30 PM, Michael S. Tsirkin wrote: > 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 >> 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. > > You need a pointer to reference-count it. > You return pointer to new object, no one references the old one. The pointer is just vhost_log->log. The old pointer will be kept in vhost_dev_log_resize() until 1) new log was set through ioctl and 2) the old log was synced. Then vhost_log_put() will drop the reference count to the old log. > >> > >> >> + } 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. As I replied above, it was not lost and kept in vhost_log->log.