On Thu, Nov 14, 2024 at 11:08:35AM +0000, Zuo,Boqun wrote:
On Thursday, November 14, 2024 6:54 PM, Stefano Garzarella wrote:
>
>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.
>
>Fixes: 6166799f6 ("vhost_net: configure all host notifiers in a single
>MR transaction")
>Reported-by: Gao Shiyuan <gaoshiy...@baidu.com>
>Signed-off-by: Zuo Boqun <zuobo...@baidu.com>
>---
> hw/net/vhost_net.c | 31 ++++++++++++++++++++-----------
> 1 file changed, 20 insertions(+), 11 deletions(-)
>
>diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c index
>997aab0557..eb3e92ca0d 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;
>+ }
>+ }
>+
> /*
> * 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,10 @@ static int vhost_net_enable_notifiers(VirtIODevice
*dev,
> return 0;
> fail_nvhosts:
> vhost_net_disable_notifiers_nvhosts(dev, ncs, data_queue_pairs,
>i);
IIUC this call is disabling notifiers from 0 to `i - 1` ...
>+ for (k = i + 1; k < nvhosts; k++) {
... so, should we start from `i`, instead of `i + 1`?
Thanks,
Stefano
The notifier `i` has been released when vhost_net_enable_notifiers() call
vhost_dev_disable_notifiers_nvqs():
for (j = 0; j < hdev->nvqs; j++) {
r = virtio_bus_set_host_notifier(VIRTIO_BUS(qbus),
hdev->vq_index + j,
true);
if (r < 0) {
error_report("vhost %d VQ %d notifier binding failed: %d",
i, j, -r);
memory_region_transaction_commit();
=> vhost_dev_disable_notifiers_nvqs(hdev, dev, j);
goto fail_nvhosts;
}
}
So we should start from `i+1`...
Ok, I see now.
That is really hidden, so please describe it in the commit message and
also add a comment in the code.
Thanks,
Stefano
>+ virtio_device_release_ioeventfd(dev);
>+ }
>+
> return r;
> }
>
>--
>2.42.0.windows.2
>
