From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36394) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ynkvb-0001gY-NC for qemu-devel@nongnu.org; Thu, 30 Apr 2015 05:38:49 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YnkvY-0007iw-62 for qemu-devel@nongnu.org; Thu, 30 Apr 2015 05:38:43 -0400 Received: from mx1.redhat.com ([209.132.183.28]:56355) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YnkvX-0007il-Us for qemu-devel@nongnu.org; Thu, 30 Apr 2015 05:38:40 -0400 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t3U9cac7004965 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Thu, 30 Apr 2015 05:38:38 -0400 Date: Thu, 30 Apr 2015 11:38:34 +0200 From: "Michael S. Tsirkin" Message-ID: <20150430113800-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> <20150430100841-mutt-send-email-mst@redhat.com> <1430385753.20407.5@smtp.corp.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1430385753.20407.5@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 05:22:33PM +0800, Jason Wang wrote: > > > On Thu, Apr 30, 2015 at 4:09 PM, Michael S. Tsirkin wrote: > >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? > > Yes. > > > > >So the log is synced in device 1, but not in device 2. > > But we will do resizing one by one for all listeners. So the sync of device > 2 will happen soon afterwards. yes but it will get the new log from vhost_log_get, not the old one, and sync the new buffer not the old one. -- MSt