Re: [Qemu-devel] [PATCH v2 4/9] vhost: alloc shareable log

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Marc-André Lureau (mlureau@redhat.com) wrote:
> Hi
> 
> ----- Original Message -----
> > * Marc-André Lureau (marcandre.lureau@redhat.com) 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         | 38 +++++++++++++++++++++++++++++++-------
> > >  include/hw/virtio/vhost.h |  3 ++-
> > >  2 files changed, 33 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > > index 2712c6f..862e786 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 <linux/vhost.h>
> > >  #include "exec/address-spaces.h"
> > >  #include "hw/virtio/virtio-bus.h"
> > > @@ -286,20 +287,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);
> > 
> > qemu_memfd_alloc can return NULL can't it - so that needs checking?
> > 
> > > +    } else {
> > > +        log->log = g_malloc0(logsize);
> > 
> > I know the old code also used g_malloc0, but if the log isn't 'small'
> > then g_try_malloc0 is possibly safer and properly return errors
> > if it can't be allocated.
> 
> Yeah, I agree it's better to check for the return value here (as you pointed out, I followed the existing pattern).
> 
> Maybe we are just screwed if it happens, live migration shouldn't succeed if it can't be done properly imho.

Probably; we try to be careful particularly on the source of memory allocations
during migration, since the VM is running just fine, and it's unfortunate
to kill it at that point.

Dave

> What's your take on this Michael?
> 
> cheers
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK