From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51329) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YnjXL-00080y-9u for qemu-devel@nongnu.org; Thu, 30 Apr 2015 04:09:36 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YnjXG-0000dY-9f for qemu-devel@nongnu.org; Thu, 30 Apr 2015 04:09:35 -0400 Received: from mx1.redhat.com ([209.132.183.28]:47373) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YnjXG-0000dQ-1m for qemu-devel@nongnu.org; Thu, 30 Apr 2015 04:09:30 -0400 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (Postfix) with ESMTPS id C6C3F8F310 for ; Thu, 30 Apr 2015 08:09:29 +0000 (UTC) Date: Thu, 30 Apr 2015 10:09:27 +0200 From: "Michael S. Tsirkin" Message-ID: <20150430100841-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> <1430381109.20407.1@smtp.corp.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1430381109.20407.1@smtp.corp.redhat.com> Subject: Re: [Qemu-devel] [PATCH V2] vhost: logs sharing List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jason Wang Cc: qemu-devel@nongnu.org On Thu, Apr 30, 2015 at 04:05:09PM +0800, Jason Wang wrote: > > > 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. vhost_dev_log_resize is per device, isn't it? So the log is synced in device 1, but not in device 2. > 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.