Re: [PATCH] resend slave fd to vhost when reconnect to vhost

Re: [PATCH] resend slave fd to vhost when reconnect to vhost

[Raphael Norwitz]

On Fri, Apr 17, 2020 at 01:14:00PM +0800, ni.xun.intel@gmail.com wrote:
> 
> From: Ni Xun <richardni@tencent.com>
> 
> when reconnecting to vhost server, it doesn't send slave fd to vhost
> as the slave fd is only sent in vhost_user_init. also resend the slave fd
> in vhost reconnect.
> 

I don’t think that’s correct. See vhost_user_init() here:

https://git.qemu.org/?p=qemu.git;a=blob;f=hw/virtio/vhost-user.c;h=08e7e63790e5bcfae6cd31bf9ccd32c3a7347f4e;hb=HEAD#l1898

Rather, vhost_setup_slave_channel() is called by vhost_user_backend_init(),
which is called on every reconnect inside vhost_dev_init().

see vhost_user_blk_connect(): https://git.qemu.org/?p=qemu.git;a=blob;f=hw/block/vhost-user-blk.c;h=17df5338e77c684175a86e882b508849c246e78a;hb=HEAD#l297
and vhost_dev_init():https://git.qemu.org/?p=qemu.git;a=blob;f=hw/virtio/vhost.c;h=01ebe12f28e9d7e3150375dda6f55b6b8f04a42a;hb=HEAD#l1224

Note that the dev->vq_index is also set to 0 right before the
vhost_dev_init() call.

I tested myself and saw a VHOST_USER_SET_SLAVE_REQ_FD message sent on each
reconnect with the vhost-user-blk sample.

Are you seeing different behavior? If so, please provide more details on
your setup?

> 
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index 17df5338e7..59650a570b 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -138,6 +138,12 @@ static int vhost_user_blk_start(VirtIODevice *vdev)
>              error_report("Error get inflight: %d", -ret);
>              goto err_guest_notifiers;
>          }

What's the justification for sending the slave fd here? Please elaborate.

> +    } else {
> +        ret = vhost_setup_slave_channel(&s->dev);
> +        if (ret < 0) {
> +            error_report("Error setting vhost slave channel: %d", -ret);
> +            return ret;
> +        }
>      }
> diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> index 085450c6f8..cad60ad521 100644
> --- a/include/hw/virtio/vhost.h
> +++ b/include/hw/virtio/vhost.h

Why expose this through vhost.h? This is vhost-user specific.

> @@ -100,6 +100,7 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev);
>  void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev);
>  int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev);
>  void vhost_dev_disable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev);
> +int vhost_setup_slave_channel(struct vhost_dev *hdev);
>  
>  /* Test and clear masked event pending status.
>   * Should be called after unmask to avoid losing events.
> -- 
> 2.24.1 (Apple Git-126)
> 
> 

[PATCH] resend slave fd to vhost when reconnect to vhost

[ni.xun.intel]

From: Ni Xun <richardni@tencent.com>

when reconnecting to vhost server, it doesn't send slave fd to vhost
as the slave fd is only sent in vhost_user_init. also resend the slave fd
in vhost reconnect.

Signed-off-by: Ni Xun <richardni@tencent.com>
Signed-off-by: Lu Zhigang <tonnylu@tencent.com>
Signed-off-by: Yan Miao <leomyan@tencent.com>
Signed-off-by: lucascye <lucascye@tencent.com>
---
 hw/block/vhost-user-blk.c | 6 ++++++
 hw/virtio/vhost-user.c    | 2 +-
 include/hw/virtio/vhost.h | 1 +
 3 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index 17df5338e7..59650a570b 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -138,6 +138,12 @@ static int vhost_user_blk_start(VirtIODevice *vdev)
             error_report("Error get inflight: %d", -ret);
             goto err_guest_notifiers;
         }
+    } else {
+        ret = vhost_setup_slave_channel(&s->dev);
+        if (ret < 0) {
+            error_report("Error setting vhost slave channel: %d", -ret);
+            return ret;
+        }
     }
 
     ret = vhost_dev_set_inflight(&s->dev, s->inflight);
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 08e7e63790..0da4a12787 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -1115,7 +1115,7 @@ err:
     return;
 }
 
-static int vhost_setup_slave_channel(struct vhost_dev *dev)
+int vhost_setup_slave_channel(struct vhost_dev *dev)
 {
     VhostUserMsg msg = {
         .hdr.request = VHOST_USER_SET_SLAVE_REQ_FD,
diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
index 085450c6f8..cad60ad521 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -100,6 +100,7 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev);
 void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev);
 int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev);
 void vhost_dev_disable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev);
+int vhost_setup_slave_channel(struct vhost_dev *hdev);
 
 /* Test and clear masked event pending status.
  * Should be called after unmask to avoid losing events.
-- 
2.24.1 (Apple Git-126)

Re: [PATCH] resend slave fd to vhost when reconnect to vhost

[xun ni]

oh, yes, there is some mis-config here, thanks for  pointing that out.
looks like the upstream has fixed it.

Thanks,
Xun

Raphael Norwitz <raphael.norwitz@nutanix.com> 于2020年4月18日周六 上午9:46写道：
>
> On Fri, Apr 17, 2020 at 01:14:00PM +0800, ni.xun.intel@gmail.com wrote:
> >
> > From: Ni Xun <richardni@tencent.com>
> >
> > when reconnecting to vhost server, it doesn't send slave fd to vhost
> > as the slave fd is only sent in vhost_user_init. also resend the slave fd
> > in vhost reconnect.
> >
>
> I don’t think that’s correct. See vhost_user_init() here:
>
> https://git.qemu.org/?p=qemu.git;a=blob;f=hw/virtio/vhost-user.c;h=08e7e63790e5bcfae6cd31bf9ccd32c3a7347f4e;hb=HEAD#l1898
>
> Rather, vhost_setup_slave_channel() is called by vhost_user_backend_init(),
> which is called on every reconnect inside vhost_dev_init().
>
> see vhost_user_blk_connect(): https://git.qemu.org/?p=qemu.git;a=blob;f=hw/block/vhost-user-blk.c;h=17df5338e77c684175a86e882b508849c246e78a;hb=HEAD#l297
> and vhost_dev_init():https://git.qemu.org/?p=qemu.git;a=blob;f=hw/virtio/vhost.c;h=01ebe12f28e9d7e3150375dda6f55b6b8f04a42a;hb=HEAD#l1224
>
> Note that the dev->vq_index is also set to 0 right before the
> vhost_dev_init() call.
>
> I tested myself and saw a VHOST_USER_SET_SLAVE_REQ_FD message sent on each
> reconnect with the vhost-user-blk sample.
>
> Are you seeing different behavior? If so, please provide more details on
> your setup?
>
> >
> > diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> > index 17df5338e7..59650a570b 100644
> > --- a/hw/block/vhost-user-blk.c
> > +++ b/hw/block/vhost-user-blk.c
> > @@ -138,6 +138,12 @@ static int vhost_user_blk_start(VirtIODevice *vdev)
> >              error_report("Error get inflight: %d", -ret);
> >              goto err_guest_notifiers;
> >          }
>
> What's the justification for sending the slave fd here? Please elaborate.
>
> > +    } else {
> > +        ret = vhost_setup_slave_channel(&s->dev);
> > +        if (ret < 0) {
> > +            error_report("Error setting vhost slave channel: %d", -ret);
> > +            return ret;
> > +        }
> >      }
> > diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> > index 085450c6f8..cad60ad521 100644
> > --- a/include/hw/virtio/vhost.h
> > +++ b/include/hw/virtio/vhost.h
>
> Why expose this through vhost.h? This is vhost-user specific.
>
> > @@ -100,6 +100,7 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev);
> >  void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev);
> >  int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev);
> >  void vhost_dev_disable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev);
> > +int vhost_setup_slave_channel(struct vhost_dev *hdev);
> >
> >  /* Test and clear masked event pending status.
> >   * Should be called after unmask to avoid losing events.
> > --
> > 2.24.1 (Apple Git-126)
> >
> >