From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35505) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ze6mR-0000QX-84 for qemu-devel@nongnu.org; Mon, 21 Sep 2015 15:29:40 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Ze6mM-0002Q2-8W for qemu-devel@nongnu.org; Mon, 21 Sep 2015 15:29:39 -0400 Received: from mx1.redhat.com ([209.132.183.28]:52228) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ze6mM-0002Oo-1L for qemu-devel@nongnu.org; Mon, 21 Sep 2015 15:29:34 -0400 Date: Mon, 21 Sep 2015 22:29:29 +0300 From: "Michael S. Tsirkin" Message-ID: <20150921222715-mutt-send-email-mst@redhat.com> References: <1442657533-13030-1-git-send-email-marcandre.lureau@redhat.com> <1442657533-13030-8-git-send-email-marcandre.lureau@redhat.com> <20150921113812-mutt-send-email-mst@redhat.com> <2063208980.14557622.1442844122026.JavaMail.zimbra@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <2063208980.14557622.1442844122026.JavaMail.zimbra@redhat.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v4 07/22] vhost: alloc shareable log List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?iso-8859-1?Q?Marc-Andr=E9?= Lureau Cc: haifeng lin , thibaut collet , jasowang@redhat.com, qemu-devel@nongnu.org, pbonzini@redhat.com, marcandre lureau On Mon, Sep 21, 2015 at 10:02:02AM -0400, Marc-Andr=E9 Lureau wrote: > Hi >=20 > ----- Original Message ----- > > On Sat, Sep 19, 2015 at 12:11:58PM +0200, marcandre.lureau@redhat.com= wrote: > > > From: Marc-Andr=E9 Lureau > > >=20 > > > If the backend is of type VHOST_BACKEND_TYPE_USER, allocate > > > shareable memory. Next patch will only allocate when the backend > > > has the required feature. > > >=20 > > > Note: vhost_log_get() can use a global "vhost_log" that can be shar= ed by > > > several vhost devices. We may want instead a common shareable log a= nd a > > > common non-shareable one. > > >=20 > > > Signed-off-by: Marc-Andr=E9 Lureau > >=20 > > Well you do at least need to count the number of times the log is > > shared. Otherwise, if you share the log, then unshare it, you are le= ft > > with a shared one. >=20 > Do you mean that if the device is removed (or stopped, so that the log > is unref) and a shm log is no longer needed, it should replace the log > of other devices with a non-shm log? Yes. Basically, you have code to swicth non-shared -> shared but not back. That's asymmetrical. > In this case, wouldn't it make > more sense to have a log shm per device, because replacing other > devices log do not seem simple without more tracking of devices > sharing the log. A per device log slows down syncs. Jason coded the shared one specifically to avoid the need to do that. > Can this be considered a future enhancement? What's the big issue? Just count the devices that need a shared one, if that count is 0 reallocate with shared =3D=3D false. > >=20 > > > --- > > > hw/virtio/vhost.c | 38 +++++++++++++++++++++++++++++++----= --- > > > include/hw/virtio/vhost.h | 3 ++- > > > 2 files changed, 33 insertions(+), 8 deletions(-) > > >=20 > > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > > > index a08c36b..cd3af16 100644 > > > --- a/hw/virtio/vhost.c > > > +++ b/hw/virtio/vhost.c > > > @@ -18,6 +18,7 @@ > > > #include "qemu/atomic.h" > > > #include "qemu/range.h" > > > #include "qemu/error-report.h" > > > +#include "qemu/memfd.h" > > > #include > > > #include "exec/address-spaces.h" > > > #include "hw/virtio/virtio-bus.h" > > > @@ -286,20 +287,34 @@ static uint64_t vhost_get_log_size(struct vho= st_dev > > > *dev) > > > } > > > return log_size; > > > } > > > -static struct vhost_log *vhost_log_alloc(uint64_t size) > > > + > > > +static struct vhost_log *vhost_log_alloc(uint64_t size, bool share= ) > > > { > > > - struct vhost_log *log =3D g_malloc0(sizeof *log + size * > > > sizeof(*(log->log))); > > > + struct vhost_log *log; > > > + uint64_t logsize =3D size * sizeof(*(log->log)); > > > + int fd =3D -1; > > > + > > > + log =3D g_new0(struct vhost_log, 1); > > > + if (share) { > > > + log->log =3D qemu_memfd_alloc("vhost-log", logsize, > > > + F_SEAL_GROW|F_SEAL_SHRINK|F_SE= AL_SEAL, > > > &fd); > > > + memset(log->log, 0, logsize); > > > + } else { > > > + log->log =3D g_malloc0(logsize); > > > + } > > > =20 > > > log->size =3D size; > > > log->refcnt =3D 1; > > > + log->fd =3D fd; > > > =20 > > > return log; > > > } > > > =20 > > > -static struct vhost_log *vhost_log_get(uint64_t size) > > > +static struct vhost_log *vhost_log_get(uint64_t size, bool share) > > > { > > > - if (!vhost_log || vhost_log->size !=3D size) { > > > - vhost_log =3D vhost_log_alloc(size); > > > + if (!vhost_log || vhost_log->size !=3D size || > > > + (share && vhost_log->fd =3D=3D -1)) { > > > + vhost_log =3D vhost_log_alloc(size, share); > > > } else { > > > ++vhost_log->refcnt; > > > } > > > @@ -324,13 +339,21 @@ static void vhost_log_put(struct vhost_dev *d= ev, bool > > > sync) > > > if (vhost_log =3D=3D log) { > > > vhost_log =3D NULL; > > > } > > > + > > > + if (log->fd =3D=3D -1) { > > > + g_free(log->log); > > > + } else { > > > + qemu_memfd_free(log->log, log->size * sizeof(*(log->lo= g)), > > > + log->fd); > > > + } > > > g_free(log); > > > } > > > } > > > =20 > > > static inline void vhost_dev_log_resize(struct vhost_dev* dev, uin= t64_t > > > size) > > > { > > > - struct vhost_log *log =3D vhost_log_get(size); > > > + bool share =3D dev->vhost_ops->backend_type =3D=3D VHOST_BACKE= ND_TYPE_USER; > > > + struct vhost_log *log =3D vhost_log_get(size, share); > > > uint64_t log_base =3D (uintptr_t)log->log; > > > int r; > > > =20 > > > @@ -1136,9 +1159,10 @@ int vhost_dev_start(struct vhost_dev *hdev, > > > VirtIODevice *vdev) > > > =20 > > > if (hdev->log_enabled) { > > > uint64_t log_base; > > > + bool share =3D hdev->vhost_ops->backend_type =3D=3D > > > VHOST_BACKEND_TYPE_USER; > > > =20 > > > hdev->log_size =3D vhost_get_log_size(hdev); > > > - hdev->log =3D vhost_log_get(hdev->log_size); > > > + hdev->log =3D vhost_log_get(hdev->log_size, share); > > > log_base =3D (uintptr_t)hdev->log->log; > > > r =3D hdev->vhost_ops->vhost_call(hdev, VHOST_SET_LOG_BASE= , > > > hdev->log_size ? &log_base= : > > > NULL); > > > diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h > > > index 6467c73..ab1dcac 100644 > > > --- a/include/hw/virtio/vhost.h > > > +++ b/include/hw/virtio/vhost.h > > > @@ -31,7 +31,8 @@ typedef unsigned long vhost_log_chunk_t; > > > struct vhost_log { > > > unsigned long long size; > > > int refcnt; > > > - vhost_log_chunk_t log[0]; > > > + int fd; > > > + vhost_log_chunk_t *log; > > > }; > > > =20 > > > struct vhost_memory; > > > -- > > > 2.4.3 > >=20