Re: [PATCH v2] vhost_net: fix assertion triggered by batch of host notifiers processing

[Zuo,Boqun]

On Mon, Nov 25, 2024 11:53 AM Jason Wang <jasowang@redhat.com> wrote:
> On Fri, Nov 15, 2024 at 4:03 PM Zuo boqun <zuoboqun@baidu.com> wrote:
> >
> > From: zuoboqun <zuoboqun@baidu.com>
> >
> > When the backend of vhost_net restarts during the vm is running,
> > vhost_net is stopped and started. The virtio_device_grab_ioeventfd()
> > fucntion in
> > vhost_net_enable_notifiers() will result in a call to
> > virtio_bus_set_host_notifier()(assign=false).
> >
> > And now virtio_device_grab_ioeventfd() is batched in a single
> > transaction with virtio_bus_set_host_notifier()(assign=true).
> >
> > This triggers the following assertion:
> >
> > kvm_mem_ioeventfd_del: error deleting ioeventfd: Bad file descriptor
> >
> > This patch moves virtio_device_grab_ioeventfd() out of the batch to
> > fix this problem.
> >
> > To be noted that the for loop to release ioeventfd should start from
> > i+1, not i, because the i-th ioeventfd has already been released in
> > vhost_dev_disable_notifiers_nvqs().
> >
> > Fixes: 6166799f6 ("vhost_net: configure all host notifiers in a single
> > MR transaction")
> > Signed-off-by: Zuo Boqun <zuoboqun@baidu.com>
> > Reported-by: Gao Shiyuan <gaoshiyuan@baidu.com>
> 
> I think we need cc stable for this.

OK

> 
> >
> > ---
> >
> > v1->v2:
> >     *To explain why the for loop to release ioeventfd starts from i+1:
> >       1) add a comment in the code
> >       2) describe it in the commit message
> > ---
> >  hw/net/vhost_net.c | 35 ++++++++++++++++++++++++-----------
> >  1 file changed, 24 insertions(+), 11 deletions(-)
> >
> > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c index
> > 997aab0557..891f235a0a 100644
> > --- a/hw/net/vhost_net.c
> > +++ b/hw/net/vhost_net.c
> > @@ -229,9 +229,24 @@ static int vhost_net_enable_notifiers(VirtIODevice
> *dev,
> >      int nvhosts = data_queue_pairs + cvq;
> >      struct vhost_net *net;
> >      struct vhost_dev *hdev;
> > -    int r, i, j;
> > +    int r, i, j, k;
> >      NetClientState *peer;
> >
> > +    /*
> > +     * We will pass the notifiers to the kernel, make sure that QEMU
> > +     * doesn't interfere.
> > +     */
> > +    for (i = 0; i < nvhosts; i++) {
> > +        r = virtio_device_grab_ioeventfd(dev);
> > +        if (r < 0) {
> > +            error_report("vhost %d binding does not support host 
> > notifiers", i);
> > +            for (k = 0; k < i; k++) {
> > +                virtio_device_release_ioeventfd(dev);
> > +            }
> > +            return r;
> > +        }
> 
> Could we tweak the code to reuse the fail_nvhosts?

OK, I will make this change and cc stable in the next version

> 
> > +    }
> > +
> >      /*
> >       * Batch all the host notifiers in a single transaction to avoid
> >       * quadratic time complexity in address_space_update_ioeventfds().
> > @@ -247,16 +262,6 @@ static int
> > vhost_net_enable_notifiers(VirtIODevice *dev,
> >
> >          net = get_vhost_net(peer);
> >          hdev = &net->dev;
> > -        /*
> > -         * We will pass the notifiers to the kernel, make sure that QEMU
> > -         * doesn't interfere.
> > -         */
> > -        r = virtio_device_grab_ioeventfd(dev);
> > -        if (r < 0) {
> > -            error_report("binding does not support host notifiers");
> > -            memory_region_transaction_commit();
> > -            goto fail_nvhosts;
> > -        }
> >
> >          for (j = 0; j < hdev->nvqs; j++) {
> >              r = virtio_bus_set_host_notifier(VIRTIO_BUS(qbus),
> > @@ -277,6 +282,14 @@ static int vhost_net_enable_notifiers(VirtIODevice
> *dev,
> >      return 0;
> >  fail_nvhosts:
> >      vhost_net_disable_notifiers_nvhosts(dev, ncs, data_queue_pairs,
> > i);
> > +    /*
> > +     * This for loop starts from i+1, not i, because the i-th ioeventfd
> > +     * has already been released in vhost_dev_disable_notifiers_nvqs().
> > +     */
> > +    for (k = i + 1; k < nvhosts; k++) {
> > +        virtio_device_release_ioeventfd(dev);
> > +    }
> > +
> >      return r;
> >  }
> >
> > --
> > 2.42.0.windows.2
> >
> 
> Thanks