[RFC,4/6] vhost: alloc shareable log

If the backend is of type VHOST_BACKEND_TYPE_USER, allocate
shareable memory.

Note: vhost_log_get() can use a global "vhost_log" that can be shared by
several vhost devices. We may want instead a common shareable log and a
common non-shareable one.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/virtio/vhost.c         | 42 +++++++++++++++++++++++++++++++++---------
 include/hw/virtio/vhost.h |  3 ++-
 2 files changed, 35 insertions(+), 10 deletions(-)

On 07/23/2015 09:36 AM, Marc-André Lureau wrote:
> If the backend is of type VHOST_BACKEND_TYPE_USER, allocate
> shareable memory.
>
> Note: vhost_log_get() can use a global "vhost_log" that can be shared by
> several vhost devices. We may want instead a common shareable log and a
> common non-shareable one.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  hw/virtio/vhost.c         | 42 +++++++++++++++++++++++++++++++++---------
>  include/hw/virtio/vhost.h |  3 ++-
>  2 files changed, 35 insertions(+), 10 deletions(-)
>
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 2712c6f..12dd644 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -286,20 +286,34 @@ static uint64_t vhost_get_log_size(struct vhost_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 = g_malloc0(sizeof *log + size * sizeof(*(log->log)));
> +    struct vhost_log *log;
> +    uint64_t logsize = size * sizeof(*(log->log));
> +    int fd = -1;
> +
> +    log = g_new0(struct vhost_log, 1);
> +    if (share) {
> +        log->log = qemu_memfd_alloc("vhost-log", logsize,
> +                                    F_SEAL_GROW|F_SEAL_SHRINK|F_SEAL_SEAL, &fd);
> +        memset(log->log, 0, logsize);
> +    } else {
> +        log->log = g_malloc0(logsize);
> +    }
>  
>      log->size = size;
>      log->refcnt = 1;
> +    log->fd = fd;
>  
>      return log;
>  }
>  
> -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 != size) {
> -        vhost_log = vhost_log_alloc(size);
> +    if (!vhost_log || vhost_log->size != size ||
> +        (share && vhost_log->fd == -1)) {
> +        vhost_log = vhost_log_alloc(size, share);
>      } else {
>          ++vhost_log->refcnt;
>      }
> @@ -324,21 +338,30 @@ static void vhost_log_put(struct vhost_dev *dev, bool sync)
>          if (vhost_log == log) {
>              vhost_log = NULL;
>          }
> +
> +        if (log->fd == -1) {
> +            g_free(log->log);
> +        } else {
> +            qemu_memfd_free(log->log, log->size * sizeof(*(log->log)),
> +                            log->fd);
> +        }
>          g_free(log);
>      }
>  }
>  
>  static inline void vhost_dev_log_resize(struct vhost_dev* dev, uint64_t size)
>  {
> -    struct vhost_log *log = vhost_log_get(size);
> +    bool share = dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_USER;
> +    struct vhost_log *log = vhost_log_get(size, share);
>      uint64_t log_base = (uintptr_t)log->log;
>      int r;
>  
> -    r = dev->vhost_ops->vhost_call(dev, VHOST_SET_LOG_BASE, &log_base);
> -    assert(r >= 0);
>      vhost_log_put(dev, true);
>      dev->log = log;
>      dev->log_size = size;
> +
> +    r = dev->vhost_ops->vhost_call(dev, VHOST_SET_LOG_BASE, &log_base);
> +    assert(r >= 0);
>  }

Why this change is needed?

>  
>  static int vhost_verify_ring_mappings(struct vhost_dev *dev,
> @@ -1136,9 +1159,10 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
>  
>      if (hdev->log_enabled) {
>          uint64_t log_base;
> +        bool share = hdev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_USER;
>  
>          hdev->log_size = vhost_get_log_size(hdev);
> -        hdev->log = vhost_log_get(hdev->log_size);
> +        hdev->log = vhost_log_get(hdev->log_size, share);
>          log_base = (uintptr_t)hdev->log->log;
>          r = 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;
>  };
>  
>  struct vhost_memory;

On Tue, Jul 28, 2015 at 01:28:05PM +0800, Jason Wang wrote:
> 
> 
> On 07/23/2015 09:36 AM, Marc-André Lureau wrote:
> > If the backend is of type VHOST_BACKEND_TYPE_USER, allocate
> > shareable memory.
> >
> > Note: vhost_log_get() can use a global "vhost_log" that can be shared by
> > several vhost devices. We may want instead a common shareable log and a
> > common non-shareable one.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  hw/virtio/vhost.c         | 42 +++++++++++++++++++++++++++++++++---------
> >  include/hw/virtio/vhost.h |  3 ++-
> >  2 files changed, 35 insertions(+), 10 deletions(-)
> >
> > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > index 2712c6f..12dd644 100644
> > --- a/hw/virtio/vhost.c
> > +++ b/hw/virtio/vhost.c
> > @@ -286,20 +286,34 @@ static uint64_t vhost_get_log_size(struct vhost_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 = g_malloc0(sizeof *log + size * sizeof(*(log->log)));
> > +    struct vhost_log *log;
> > +    uint64_t logsize = size * sizeof(*(log->log));
> > +    int fd = -1;
> > +
> > +    log = g_new0(struct vhost_log, 1);
> > +    if (share) {
> > +        log->log = qemu_memfd_alloc("vhost-log", logsize,
> > +                                    F_SEAL_GROW|F_SEAL_SHRINK|F_SEAL_SEAL, &fd);
> > +        memset(log->log, 0, logsize);
> > +    } else {
> > +        log->log = g_malloc0(logsize);
> > +    }
> >  
> >      log->size = size;
> >      log->refcnt = 1;
> > +    log->fd = fd;
> >  
> >      return log;
> >  }
> >  
> > -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 != size) {
> > -        vhost_log = vhost_log_alloc(size);
> > +    if (!vhost_log || vhost_log->size != size ||
> > +        (share && vhost_log->fd == -1)) {
> > +        vhost_log = vhost_log_alloc(size, share);
> >      } else {
> >          ++vhost_log->refcnt;
> >      }
> > @@ -324,21 +338,30 @@ static void vhost_log_put(struct vhost_dev *dev, bool sync)
> >          if (vhost_log == log) {
> >              vhost_log = NULL;
> >          }
> > +
> > +        if (log->fd == -1) {
> > +            g_free(log->log);
> > +        } else {
> > +            qemu_memfd_free(log->log, log->size * sizeof(*(log->log)),
> > +                            log->fd);
> > +        }
> >          g_free(log);
> >      }
> >  }
> >  
> >  static inline void vhost_dev_log_resize(struct vhost_dev* dev, uint64_t size)
> >  {
> > -    struct vhost_log *log = vhost_log_get(size);
> > +    bool share = dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_USER;
> > +    struct vhost_log *log = vhost_log_get(size, share);
> >      uint64_t log_base = (uintptr_t)log->log;
> >      int r;
> >  
> > -    r = dev->vhost_ops->vhost_call(dev, VHOST_SET_LOG_BASE, &log_base);
> > -    assert(r >= 0);
> >      vhost_log_put(dev, true);
> >      dev->log = log;
> >      dev->log_size = size;
> > +
> > +    r = dev->vhost_ops->vhost_call(dev, VHOST_SET_LOG_BASE, &log_base);
> > +    assert(r >= 0);
> >  }
> 
> Why this change is needed?

I know why it's needed :) But this needs to be stated in the commit log.
Also, it only makes sense if remote supports getting the logfd.

> >  
> >  static int vhost_verify_ring_mappings(struct vhost_dev *dev,
> > @@ -1136,9 +1159,10 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
> >  
> >      if (hdev->log_enabled) {
> >          uint64_t log_base;
> > +        bool share = hdev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_USER;
> >  
> >          hdev->log_size = vhost_get_log_size(hdev);
> > -        hdev->log = vhost_log_get(hdev->log_size);
> > +        hdev->log = vhost_log_get(hdev->log_size, share);
> >          log_base = (uintptr_t)hdev->log->log;
> >          r = 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;
> >  };
> >  
> >  struct vhost_memory;

Hi

On Tue, Jul 28, 2015 at 12:10 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> I know why it's needed :) But this needs to be stated in the commit log.
> Also, it only makes sense if remote supports getting the logfd.

Thanks for pointing out this change. Actually, I think the current log
overlap when resizing is on purpose: there shouldn't be any time
without log. I'll rework that to keep the same ordering, keeping a
gap-less log switching. I'll also comment this part, as this is easy
to overlook.