Re: [Qemu-devel] [RFC PATCH] vhost-user: quit infinite loop while used memslots is more than the backend limit

[Jason Wang]

On 2017年12月19日 18:21, Zhoujian (jay) wrote:
> > -----Original Message-----
> > From: Jason Wang [mailto:address@hidden
> > Sent: Tuesday, December 19, 2017 5:24 PM
> > To: Zhoujian (jay) <address@hidden>; address@hidden
> > Cc: address@hidden; Gonglei (Arei) <address@hidden>;
> > address@hidden; Huangweidong (C) <address@hidden>;
> > wangxin (U) <address@hidden>
> > Subject: Re: [RFC PATCH] vhost-user: quit infinite loop while used
> > memslots is more than the backend limit
> >
> >
> >
> > On 2017年12月13日 15:04, Jay Zhou wrote:
> > > Steps to 100% reproduce:
> > > (1) start a VM with two VFIO 82599 PCI NICs successfully
> > > (2) hot plug a RAM, which is attached successfully
> > > (3) hot plug another RAM, which is attached successfully
> > > (4) hot plug a vhost-user NIC, which is attached successfully
> > > (5) hot plug another vhost-user NIC, which is hanged
> > >
> > > then the qemu process infinitely and quickly print the logs below:
> > >       [...]
> > >       vhost backend memory slots limit is less than current number of
> > present memory slots
> > >       failed to init vhost_net for queue 0
> > >       vhost backend memory slots limit is less than current number of
> > present memory slots
> > >       failed to init vhost_net for queue 0
> > >       vhost backend memory slots limit is less than current number of
> > present memory slots
> > >       failed to init vhost_net for queue 0
> > >       [...]
> > >
> > > and the cpu utilization of the systemd-journal process on host is
> > > 100%, the reason is clear:
> > >       "used_memslots > hdev->vhost_ops-
> > > vhost_backend_memslots_limit(hdev)"
> > > is true in vhost_dev_init, and the main thread in the infinite do and
> > > while loop in net_vhost_user_init.
> > >
> > > One way is to increase the vhost backend memory slots limit, some
> > > discussions are here:
> > >
> > > https://lists.gnu.org/archive/html/qemu-devel/2017-11/msg04572.html
> > >
> > > I think if
> > >       "used_memslots > hdev->vhost_ops-
> > > vhost_backend_memslots_limit(hdev)"
> > > is true, it does not make sense to try to initialize the vhost-user
> > > again and again, because the main thread can not handle other qmps at
> > > all, which means it can not reduce the number of memslots, it must be
> > > failed at last.
> > You should include the reason why we hit infinite loop in the commit log,
> > and what's more important, is this issue specific to memslots only?
> > What if you hit errors other than this in vhost_dev_init() for vhost-user?
> If the new hot plugged vhost-user NIC failed to initialize in vhost_dev_init,
> the s->started in net_vhost_user_init is always false, so we hit infinite loop
> in net_vhost_user_init().
> It is not specific to memslots only, e.g. if the backend lacks some feature
> masks, it will hit infinite loop too.
>
> Is it available to identify the cases that MAY be failed and MUST be failed
> at initialization?
>      - If it must be failed, quit the loop at once.
>      - If it may be failed, then we should do the loop to wait the connection
>        established, i.e. wait the s->started becomes true. And some sleep time
>        may be added to reduce the CPU utilization.

Then I think you probably need another flag e.g s->error and set it on 
fatal error.

> > Is this better to disable event source in this case?
> The "event source" you mean is the logs printed?
> If it is disabled, the reason why initialization failed is not explicit,
> this is the drawback.

No I mean the thing that triggers the loop which is s->started here.

Thanks

> Regards,
> Jay
>
> > Thanks
> >
> > > Signed-off-by: Jay Zhou <address@hidden>
> > > ---
> > >    hw/virtio/vhost.c         | 3 +++
> > >    include/hw/virtio/vhost.h | 2 ++
> > >    net/vhost-user.c          | 5 +++++
> > >    3 files changed, 10 insertions(+)
> > >
> > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c index
> > > e4290ce..1cc4a18 100644
> > > --- a/hw/virtio/vhost.c
> > > +++ b/hw/virtio/vhost.c
> > > @@ -47,6 +47,8 @@ static unsigned int used_memslots;
> > >    static QLIST_HEAD(, vhost_dev) vhost_devices =
> > >        QLIST_HEAD_INITIALIZER(vhost_devices);
> > >
> > > +bool used_memslots_exceeded;
> > > +
> > >    bool vhost_has_free_slot(void)
> > >    {
> > >        unsigned int slots_limit = ~0U;
> > > @@ -1254,6 +1256,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void
> > *opaque,
> > >        if (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");
> > > +        used_memslots_exceeded = true;
> > >            r = -1;
> > >            goto fail;
> > >        }
> > > diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> > > index 467dc77..bcc66d9 100644
> > > --- a/include/hw/virtio/vhost.h
> > > +++ b/include/hw/virtio/vhost.h
> > > @@ -6,6 +6,8 @@
> > >    #include "hw/virtio/virtio.h"
> > >    #include "exec/memory.h"
> > >
> > > +extern bool used_memslots_exceeded;
> > > +
> > >    /* Generic structures common for any vhost based device. */
> > >    struct vhost_virtqueue {
> > >        int kick;
> > > diff --git a/net/vhost-user.c b/net/vhost-user.c index
> > > c23927c..8b7bfff 100644
> > > --- a/net/vhost-user.c
> > > +++ b/net/vhost-user.c
> > > @@ -17,6 +17,7 @@
> > >    #include "qemu/error-report.h"
> > >    #include "qmp-commands.h"
> > >    #include "trace.h"
> > > +#include "include/hw/virtio/vhost.h"
> > >
> > >    typedef struct VhostUserState {
> > >        NetClientState nc;
> > > @@ -311,6 +312,10 @@ static int net_vhost_user_init(NetClientState *peer,
> > const char *device,
> > >            qemu_chr_fe_set_handlers(&s->chr, NULL, NULL,
> > >                                     net_vhost_user_event, NULL, nc0->name,
> > NULL,
> > >                                     true);
> > > +        if (used_memslots_exceeded) {
> > > +            error_report("used memslots exceeded the backend limit,
> > quit loop");
> > > +            return -1;
> > > +        }
> > >        } while (!s->started);
> > >
> > >        assert(s->vhost_net);