From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34290) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YnkgI-0000Cw-6w for qemu-devel@nongnu.org; Thu, 30 Apr 2015 05:22:55 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YnkgE-0001jj-5t for qemu-devel@nongnu.org; Thu, 30 Apr 2015 05:22:54 -0400 Received: from mx1.redhat.com ([209.132.183.28]:52985) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YnkgD-0001jV-UR for qemu-devel@nongnu.org; Thu, 30 Apr 2015 05:22:50 -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 t3U9Mn7t032414 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Thu, 30 Apr 2015 05:22:49 -0400 Date: Thu, 30 Apr 2015 17:22:33 +0800 From: Jason Wang Message-Id: <1430385753.20407.5@smtp.corp.redhat.com> In-Reply-To: <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> <20150430100841-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 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.