On Fri, Nov 15, 2024 at 4:03 PM Zuo boqun <zuobo...@baidu.com> wrote:
>
> From: zuoboqun <zuobo...@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 <zuobo...@baidu.com>
> Reported-by: Gao Shiyuan <gaoshiy...@baidu.com>
I think we need cc stable for this.
>
> ---
>
> 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?
> + }
> +
> /*
> * 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
