Re: [Qemu-devel] [PATCH v2 2/2] vhost: double check used memslots number

[Zhoujian (jay)]

> -----Original Message-----
> From: Igor Mammedov [mailto:address@hidden
> Sent: Saturday, December 23, 2017 2:49 AM
> To: Zhoujian (jay) <address@hidden>
> Cc: address@hidden; address@hidden; Huangweidong (C)
> <address@hidden>; Gonglei (Arei) <address@hidden>;
> wangxin (U) <address@hidden>; Liuzhe (Cloud Open Labs, NFV)
> <address@hidden>; address@hidden
> Subject: Re: [PATCH v2 2/2] vhost: double check used memslots number
> 
> On Fri, 15 Dec 2017 16:45:55 +0800
> Jay Zhou <address@hidden> wrote:
> 
> > If the VM already has N(N>8) available memory slots for vhost user,
> > the VM will be crashed in vhost_user_set_mem_table if we try to
> > hotplug the first vhost user NIC.
> > This patch checks if memslots number exceeded or not after updating
> > vhost_user_used_memslots.
> Can't understand commit message, pls rephrase (what is being fixed, and
> how it's fixed) also include reproducing steps for crash and maybe
> describe call flow/backtrace that triggers crash.

Sorry about my pool english

> 
> PS:
> I wasn't able to reproduce crash

Steps to reproduce:
(1) start up a VM successfully without any vhost device
(2) hotplug 8 DIMM memory successfully
(3) hotplug a vhost-user NIC, the VM crashed, it asserted
    at the line
        assert(fd_num < VHOST_MEMORY_MAX_NREGIONS);
    in vhost_user_set_mem_table()

Regards,
Jay

> >
> > Signed-off-by: Jay Zhou <address@hidden>
> > ---
> >  hw/virtio/vhost.c | 27 +++++++++++++++++++++++----
> >  1 file changed, 23 insertions(+), 4 deletions(-)
> >
> > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c index
> > 59a32e9..e45f5e2 100644
> > --- a/hw/virtio/vhost.c
> > +++ b/hw/virtio/vhost.c
> > @@ -1234,6 +1234,18 @@ static void vhost_virtqueue_cleanup(struct
> vhost_virtqueue *vq)
> >      event_notifier_cleanup(&vq->masked_notifier);
> >  }
> >
> > +static bool vhost_dev_used_memslots_is_exceeded(struct vhost_dev
> > +*hdev) {
> > +    if (hdev->vhost_ops->vhost_get_used_memslots() >
> > +        hdev->vhost_ops->vhost_backend_memslots_limit(hdev)) {
> > +        error_report("vhost backend memory slots limit is less"
> > +                " than current number of present memory slots");
> > +        return true;
> > +    }
> > +
> > +    return false;
> > +}
> > +
> >  int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
> >                     VhostBackendType backend_type, uint32_t
> > busyloop_timeout)  { @@ -1252,10 +1264,7 @@ int vhost_dev_init(struct
> > vhost_dev *hdev, void *opaque,
> >          goto fail;
> >      }
> >
> > -    if (hdev->vhost_ops->vhost_get_used_memslots() >
> > -        hdev->vhost_ops->vhost_backend_memslots_limit(hdev)) {
> > -        error_report("vhost backend memory slots limit is less"
> > -                " than current number of present memory slots");
> > +    if (vhost_dev_used_memslots_is_exceeded(hdev)) {
> why do you keep this check?
> it seems always be false
> 
> 
> 
> >          r = -1;
> >          goto fail;
> >      }
> > @@ -1341,6 +1350,16 @@ int vhost_dev_init(struct vhost_dev *hdev, void
> *opaque,
> >      hdev->memory_changed = false;
> >      memory_listener_register(&hdev->memory_listener,
> &address_space_memory);
> >      QLIST_INSERT_HEAD(&vhost_devices, hdev, entry);
> > +
> > +    if (vhost_dev_used_memslots_is_exceeded(hdev)) {
> > +        r = -1;
> > +        if (busyloop_timeout) {
> > +            goto fail_busyloop;
> > +        } else {
> > +            goto fail;
> > +        }
> > +    }
> seem to be right thing to do, since after registering listener for the
> first time used_memslots will be updated to actual value.
> 
> 
> I did some testing and without this hunk/patch
> 
> on 'device_add  virtio-net-pci,netdev=net0' qemu prints:
> 
> qemu-system-x86_64: vhost_set_mem_table failed: Argument list too long (7)
> qemu-system-x86_64: unable to start vhost net: 7: falling back on
> userspace virtio
> 
> and network is operational in guest, but with this patch
> 
> "netdev_add ...,vhost-on" prints:
> 
> vhost backend memory slots limit is less than current number of present
> memory slots vhost-net requested but could not be initialized
> 
> and following "device_add  virtio-net-pci,netdev=net0" prints:
> 
> TUNSETOFFLOAD ioctl() failed: Bad file descriptor TUNSETOFFLOAD ioctl()
> failed: Bad file descriptor
> 
> adapter is still hot-plugged but guest networking is broken (can't get IP
> address via DHCP)
> 
> so patch seems introduces a regression or something broken elsewhere and
> this exposes issue, not sure what qemu reaction should be in this case
> i.e. when netdev_add fails
>     1: should we fail followed up device_add or
>     2: make it fall back to userspace virtio
> 
> I'd go for #2,
> Michael what's your take on it?
> 
> > +
> >      return 0;
> >
> >  fail_busyloop: