[RFC v1] virtio/vsock: add two more queues for datagram types

[RFC v1] virtio/vsock: add two more queues for datagram types

[Jiang Wang]

Datagram sockets are connectionless and unreliable.
The sender does not know the capacity of the receiver
and may send more packets than the receiver can handle.

Add two more dedicate virtqueues for datagram sockets,
so that it will not unfairly steal resources from
stream and future connection-oriented sockets.

The virtio spec patch is here: 
https://www.spinics.net/lists/linux-virtualization/msg50027.html

Here is the link for the linux kernel git repo with patches
to support dgram sockets:
https://github.com/Jiang1155/linux/tree/vsock-dgram-v1

Signed-off-by: Jiang Wang <jiang.wang@bytedance.com>
---
 configure                                     | 13 +++++++++++++
 hw/virtio/vhost-vsock-common.c                | 11 ++++++++++-
 hw/virtio/vhost-vsock.c                       |  8 +++++---
 include/hw/virtio/vhost-vsock-common.h        | 10 +++++++++-
 include/standard-headers/linux/virtio_vsock.h |  3 +++
 meson.build                                   |  1 +
 6 files changed, 41 insertions(+), 5 deletions(-)

diff --git a/configure b/configure
index 9f016b06b5..6455b283a5 100755
--- a/configure
+++ b/configure
@@ -343,6 +343,7 @@ vhost_net="$default_feature"
 vhost_crypto="$default_feature"
 vhost_scsi="$default_feature"
 vhost_vsock="$default_feature"
+vhost_vsock_dgram="no"
 vhost_user="no"
 vhost_user_blk_server="auto"
 vhost_user_fs="$default_feature"
@@ -1272,6 +1273,10 @@ for opt do
   ;;
   --enable-vhost-vsock) vhost_vsock="yes"
   ;;
+  --disable-vhost-vsock-dgram) vhost_vsock_dgram="no"
+  ;;
+  --enable-vhost-vsock-dgram) vhost_vsock_dgram="yes"
+  ;;
   --disable-vhost-user-blk-server) vhost_user_blk_server="disabled"
   ;;
   --enable-vhost-user-blk-server) vhost_user_blk_server="enabled"
@@ -1839,6 +1844,7 @@ disabled with --disable-FEATURE, default is enabled if available
   attr            attr and xattr support
   vhost-net       vhost-net kernel acceleration support
   vhost-vsock     virtio sockets device support
+  vhost-vsock-dgram     virtio sockets datagram type support
   vhost-scsi      vhost-scsi kernel target support
   vhost-crypto    vhost-user-crypto backend support
   vhost-kernel    vhost kernel backend support
@@ -2389,6 +2395,10 @@ test "$vhost_vsock" = "" && vhost_vsock=$vhost_kernel
 if test "$vhost_vsock" = "yes" && test "$vhost_kernel" != "yes"; then
   error_exit "--enable-vhost-vsock requires --enable-vhost-kernel"
 fi
+test "$vhost_vsock_dgram" = "" && vhost_vsock_dgram=$vhost_vsock
+if test "$vhost_vsock_dgram" = "yes" && test "$vhost_vsock" != "yes"; then
+  error_exit "--enable-vhost-vsock-dgram requires --enable-vhost-vsock"
+fi
 
 # vhost-user backends
 test "$vhost_net_user" = "" && vhost_net_user=$vhost_user
@@ -5810,6 +5820,9 @@ if test "$vhost_vsock" = "yes" ; then
   if test "$vhost_user" = "yes" ; then
     echo "CONFIG_VHOST_USER_VSOCK=y" >> $config_host_mak
   fi
+  if test "$vhost_vsock_dgram" = "yes" ; then
+    echo "CONFIG_VHOST_VSOCK_DGRAM=y" >> $config_host_mak
+  fi
 fi
 if test "$vhost_kernel" = "yes" ; then
   echo "CONFIG_VHOST_KERNEL=y" >> $config_host_mak
diff --git a/hw/virtio/vhost-vsock-common.c b/hw/virtio/vhost-vsock-common.c
index 4ad6e234ad..fff8d12d91 100644
--- a/hw/virtio/vhost-vsock-common.c
+++ b/hw/virtio/vhost-vsock-common.c
@@ -208,7 +208,12 @@ void vhost_vsock_common_realize(VirtIODevice *vdev, const char *name)
                                       vhost_vsock_common_handle_output);
     vvc->trans_vq = virtio_add_queue(vdev, VHOST_VSOCK_QUEUE_SIZE,
                                        vhost_vsock_common_handle_output);
-
+#ifdef CONFIG_VHOST_VSOCK_DGRAM
+    vvc->dgram_recv_vq = virtio_add_queue(vdev, VHOST_VSOCK_QUEUE_SIZE,
+                                      vhost_vsock_common_handle_output);
+    vvc->dgram_trans_vq = virtio_add_queue(vdev, VHOST_VSOCK_QUEUE_SIZE,
+                                       vhost_vsock_common_handle_output);
+#endif
     /* The event queue belongs to QEMU */
     vvc->event_vq = virtio_add_queue(vdev, VHOST_VSOCK_QUEUE_SIZE,
                                        vhost_vsock_common_handle_output);
@@ -227,6 +232,10 @@ void vhost_vsock_common_unrealize(VirtIODevice *vdev)
 
     virtio_delete_queue(vvc->recv_vq);
     virtio_delete_queue(vvc->trans_vq);
+#ifdef CONFIG_VHOST_VSOCK_DGRAM
+    virtio_delete_queue(vvc->dgram_recv_vq);
+    virtio_delete_queue(vvc->dgram_trans_vq);
+#endif
     virtio_delete_queue(vvc->event_vq);
     virtio_cleanup(vdev);
 }
diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c
index 8ddfb9abfe..f6066a69bd 100644
--- a/hw/virtio/vhost-vsock.c
+++ b/hw/virtio/vhost-vsock.c
@@ -105,11 +105,13 @@ static void vhost_vsock_set_status(VirtIODevice *vdev, uint8_t status)
 }
 
 static uint64_t vhost_vsock_get_features(VirtIODevice *vdev,
-                                         uint64_t requested_features,
+                                         uint64_t features,
                                          Error **errp)
 {
-    /* No feature bits used yet */
-    return requested_features;
+#ifdef CONFIG_VHOST_VSOCK_DGRAM
+    virtio_add_feature(&features, VIRTIO_VSOCK_F_DGRAM);
+#endif
+    return features;
 }
 
 static const VMStateDescription vmstate_virtio_vhost_vsock = {
diff --git a/include/hw/virtio/vhost-vsock-common.h b/include/hw/virtio/vhost-vsock-common.h
index e412b5ee98..647ec8c8b3 100644
--- a/include/hw/virtio/vhost-vsock-common.h
+++ b/include/hw/virtio/vhost-vsock-common.h
@@ -27,13 +27,21 @@ enum {
 struct VHostVSockCommon {
     VirtIODevice parent;
 
+#ifdef CONFIG_VHOST_VSOCK_DGRAM
+    struct vhost_virtqueue vhost_vqs[4];
+#else
     struct vhost_virtqueue vhost_vqs[2];
+#endif
+
     struct vhost_dev vhost_dev;
 
     VirtQueue *event_vq;
     VirtQueue *recv_vq;
     VirtQueue *trans_vq;
-
+#ifdef CONFIG_VHOST_VSOCK_DGRAM
+    VirtQueue *dgram_recv_vq;
+    VirtQueue *dgram_trans_vq;
+#endif
     QEMUTimer *post_load_timer;
 };
 
diff --git a/include/standard-headers/linux/virtio_vsock.h b/include/standard-headers/linux/virtio_vsock.h
index be443211ce..abcf2a8adf 100644
--- a/include/standard-headers/linux/virtio_vsock.h
+++ b/include/standard-headers/linux/virtio_vsock.h
@@ -38,6 +38,9 @@
 #include "standard-headers/linux/virtio_ids.h"
 #include "standard-headers/linux/virtio_config.h"
 
+/* Feature bits */
+#define VIRTIO_VSOCK_F_DGRAM 0 /*Does vsock support dgram */
+
 struct virtio_vsock_config {
 	uint64_t guest_cid;
 } QEMU_PACKED;
diff --git a/meson.build b/meson.build
index 3d889857a0..9d170e0476 100644
--- a/meson.build
+++ b/meson.build
@@ -2430,6 +2430,7 @@ summary_info += {'vhost-net support': config_host.has_key('CONFIG_VHOST_NET')}
 summary_info += {'vhost-crypto support': config_host.has_key('CONFIG_VHOST_CRYPTO')}
 summary_info += {'vhost-scsi support': config_host.has_key('CONFIG_VHOST_SCSI')}
 summary_info += {'vhost-vsock support': config_host.has_key('CONFIG_VHOST_VSOCK')}
+summary_info += {'vhost-vsock-dgram support': config_host.has_key('CONFIG_VHOST_VSOCK_DGRAM')}
 summary_info += {'vhost-user support': config_host.has_key('CONFIG_VHOST_USER')}
 summary_info += {'vhost-user-blk server support': have_vhost_user_blk_server}
 summary_info += {'vhost-user-fs support': config_host.has_key('CONFIG_VHOST_USER_FS')}
-- 
2.11.0

Re: [RFC v1] virtio/vsock: add two more queues for datagram types

[Stefano Garzarella]

On Thu, Jun 10, 2021 at 12:14:24AM +0000, Jiang Wang wrote:
>Datagram sockets are connectionless and unreliable.
>The sender does not know the capacity of the receiver
>and may send more packets than the receiver can handle.
>
>Add two more dedicate virtqueues for datagram sockets,
>so that it will not unfairly steal resources from
>stream and future connection-oriented sockets.
>
>The virtio spec patch is here:
>https://www.spinics.net/lists/linux-virtualization/msg50027.html
>
>Here is the link for the linux kernel git repo with patches
>to support dgram sockets:
>https://github.com/Jiang1155/linux/tree/vsock-dgram-v1
>
>Signed-off-by: Jiang Wang <jiang.wang@bytedance.com>
>---
> configure                                     | 13 +++++++++++++
> hw/virtio/vhost-vsock-common.c                | 11 ++++++++++-
> hw/virtio/vhost-vsock.c                       |  8 +++++---
> include/hw/virtio/vhost-vsock-common.h        | 10 +++++++++-
> include/standard-headers/linux/virtio_vsock.h |  3 +++
> meson.build                                   |  1 +
> 6 files changed, 41 insertions(+), 5 deletions(-)
>
>diff --git a/configure b/configure
>index 9f016b06b5..6455b283a5 100755
>--- a/configure
>+++ b/configure
>@@ -343,6 +343,7 @@ vhost_net="$default_feature"
> vhost_crypto="$default_feature"
> vhost_scsi="$default_feature"
> vhost_vsock="$default_feature"
>+vhost_vsock_dgram="no"
> vhost_user="no"
> vhost_user_blk_server="auto"
> vhost_user_fs="$default_feature"
>@@ -1272,6 +1273,10 @@ for opt do
>   ;;
>   --enable-vhost-vsock) vhost_vsock="yes"
>   ;;
>+  --disable-vhost-vsock-dgram) vhost_vsock_dgram="no"
>+  ;;
>+  --enable-vhost-vsock-dgram) vhost_vsock_dgram="yes"
>+  ;;

I don't think we should add a configuration option to enable/disable the 
dgram support at build time.

I think we should do it at runtime looking at the features negiotated.

Take a look at virtio_net_set_multiqueue().

Thanks,
Stefano

Re: Re: [RFC v1] virtio/vsock: add two more queues for datagram types

[Jiang Wang .]

On Thu, Jun 10, 2021 at 2:40 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Thu, Jun 10, 2021 at 12:14:24AM +0000, Jiang Wang wrote:
> >Datagram sockets are connectionless and unreliable.
> >The sender does not know the capacity of the receiver
> >and may send more packets than the receiver can handle.
> >
> >Add two more dedicate virtqueues for datagram sockets,
> >so that it will not unfairly steal resources from
> >stream and future connection-oriented sockets.
> >
> >The virtio spec patch is here:
> >https://www.spinics.net/lists/linux-virtualization/msg50027.html
> >
> >Here is the link for the linux kernel git repo with patches
> >to support dgram sockets:
> >https://github.com/Jiang1155/linux/tree/vsock-dgram-v1
> >
> >Signed-off-by: Jiang Wang <jiang.wang@bytedance.com>
> >---
> > configure                                     | 13 +++++++++++++
> > hw/virtio/vhost-vsock-common.c                | 11 ++++++++++-
> > hw/virtio/vhost-vsock.c                       |  8 +++++---
> > include/hw/virtio/vhost-vsock-common.h        | 10 +++++++++-
> > include/standard-headers/linux/virtio_vsock.h |  3 +++
> > meson.build                                   |  1 +
> > 6 files changed, 41 insertions(+), 5 deletions(-)
> >
> >diff --git a/configure b/configure
> >index 9f016b06b5..6455b283a5 100755
> >--- a/configure
> >+++ b/configure
> >@@ -343,6 +343,7 @@ vhost_net="$default_feature"
> > vhost_crypto="$default_feature"
> > vhost_scsi="$default_feature"
> > vhost_vsock="$default_feature"
> >+vhost_vsock_dgram="no"
> > vhost_user="no"
> > vhost_user_blk_server="auto"
> > vhost_user_fs="$default_feature"
> >@@ -1272,6 +1273,10 @@ for opt do
> >   ;;
> >   --enable-vhost-vsock) vhost_vsock="yes"
> >   ;;
> >+  --disable-vhost-vsock-dgram) vhost_vsock_dgram="no"
> >+  ;;
> >+  --enable-vhost-vsock-dgram) vhost_vsock_dgram="yes"
> >+  ;;
>
> I don't think we should add a configuration option to enable/disable the
> dgram support at build time.
>
> I think we should do it at runtime looking at the features negiotated.
>
> Take a look at virtio_net_set_multiqueue().

Got it. Will check. Thanks.

> Thanks,
> Stefano
>

Re: Re: [RFC v1] virtio/vsock: add two more queues for datagram types

[Jiang Wang .]

Hi Stefano,

I checked virtio_net_set_multiqueue(), which will help with following
changes in my patch:

#ifdef CONFIG_VHOST_VSOCK_DGRAM
vvc->dgram_recv_vq = virtio_add_queue(vdev, VHOST_VSOCK_QUEUE_SIZE,
vhost_vsock_common_handle_output);
vvc->dgram_trans_vq = virtio_add_queue(vdev, VHOST_VSOCK_QUEUE_SIZE,
vhost_vsock_common_handle_output);
#endif

But I think there is still an issue with the following lines, right?

#ifdef CONFIG_VHOST_VSOCK_DGRAM
struct vhost_virtqueue vhost_vqs[4];
#else
struct vhost_virtqueue vhost_vqs[2];
#endif

I think the problem with feature bits is that they are set and get after
vhost_vsock_common_realize() and after vhost_dev_init() in drivers/vhost/vsock.c
But those virtqueues need to be set up correctly beforehand.

I tried to test with the host kernel allocating 4 vqs, but qemu only
allocated 2 vqs, and
guest kernel will not be able to send even the vsock stream packets. I
think the host
kernel and the qemu have to agree on the number of vhost_vqs. Do you agree?
Did I miss something?

Another idea to make the setting in runtime instead of compiling time is to use
qemu cmd-line options, then qemu can allocate 2 or 4 queues depending on
the cmd line. This will solve the issue when the host kernel is an old
one( no dgram
support) and the qemu is a new one.

But there is still an issue when the host kernel is a new one, while the qemu
is an old one.  I am not sure how to make the virtqueues numbers to
change in run-time
for the host kernel. In another email thread, you mentioned removing kconfig
in the linux kernel, I believe that is related to this qemu patch,
right?  If so,
any ideas that I can make the host kernel change the number of vqs in
the run-time
or when starting up vsock? The only way I can think of is to use a
kernel module parameter
for the vsock_vhost module. Any other ideas? Thanks.

btw, I searched Linux kernel code but did not find any examples.

Regards,

Jiang

On Thu, Jun 10, 2021 at 10:29 AM Jiang Wang . <jiang.wang@bytedance.com> wrote:
>
> On Thu, Jun 10, 2021 at 2:40 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
> >
> > On Thu, Jun 10, 2021 at 12:14:24AM +0000, Jiang Wang wrote:
> > >Datagram sockets are connectionless and unreliable.
> > >The sender does not know the capacity of the receiver
> > >and may send more packets than the receiver can handle.
> > >
> > >Add two more dedicate virtqueues for datagram sockets,
> > >so that it will not unfairly steal resources from
> > >stream and future connection-oriented sockets.
> > >
> > >The virtio spec patch is here:
> > >https://www.spinics.net/lists/linux-virtualization/msg50027.html
> > >
> > >Here is the link for the linux kernel git repo with patches
> > >to support dgram sockets:
> > >https://github.com/Jiang1155/linux/tree/vsock-dgram-v1
> > >
> > >Signed-off-by: Jiang Wang <jiang.wang@bytedance.com>
> > >---
> > > configure                                     | 13 +++++++++++++
> > > hw/virtio/vhost-vsock-common.c                | 11 ++++++++++-
> > > hw/virtio/vhost-vsock.c                       |  8 +++++---
> > > include/hw/virtio/vhost-vsock-common.h        | 10 +++++++++-
> > > include/standard-headers/linux/virtio_vsock.h |  3 +++
> > > meson.build                                   |  1 +
> > > 6 files changed, 41 insertions(+), 5 deletions(-)
> > >
> > >diff --git a/configure b/configure
> > >index 9f016b06b5..6455b283a5 100755
> > >--- a/configure
> > >+++ b/configure
> > >@@ -343,6 +343,7 @@ vhost_net="$default_feature"
> > > vhost_crypto="$default_feature"
> > > vhost_scsi="$default_feature"
> > > vhost_vsock="$default_feature"
> > >+vhost_vsock_dgram="no"
> > > vhost_user="no"
> > > vhost_user_blk_server="auto"
> > > vhost_user_fs="$default_feature"
> > >@@ -1272,6 +1273,10 @@ for opt do
> > >   ;;
> > >   --enable-vhost-vsock) vhost_vsock="yes"
> > >   ;;
> > >+  --disable-vhost-vsock-dgram) vhost_vsock_dgram="no"
> > >+  ;;
> > >+  --enable-vhost-vsock-dgram) vhost_vsock_dgram="yes"
> > >+  ;;
> >
> > I don't think we should add a configuration option to enable/disable the
> > dgram support at build time.
> >
> > I think we should do it at runtime looking at the features negiotated.
> >
> > Take a look at virtio_net_set_multiqueue().
>
> Got it. Will check. Thanks.
>
> > Thanks,
> > Stefano
> >

Re: [RFC v1] virtio/vsock: add two more queues for datagram types

[Stefano Garzarella]

On Wed, Jun 23, 2021 at 11:50:33PM -0700, Jiang Wang . wrote:
>Hi Stefano,
>
>I checked virtio_net_set_multiqueue(), which will help with following
>changes in my patch:
>
>#ifdef CONFIG_VHOST_VSOCK_DGRAM
>vvc->dgram_recv_vq = virtio_add_queue(vdev, VHOST_VSOCK_QUEUE_SIZE,
>vhost_vsock_common_handle_output);
>vvc->dgram_trans_vq = virtio_add_queue(vdev, VHOST_VSOCK_QUEUE_SIZE,
>vhost_vsock_common_handle_output);
>#endif
>
>But I think there is still an issue with the following lines, right?

Yep, I think so.

>
>#ifdef CONFIG_VHOST_VSOCK_DGRAM
>struct vhost_virtqueue vhost_vqs[4];
>#else
>struct vhost_virtqueue vhost_vqs[2];
>#endif
>
>I think the problem with feature bits is that they are set and get after
>vhost_vsock_common_realize() and after vhost_dev_init() in drivers/vhost/vsock.c
>But those virtqueues need to be set up correctly beforehand.

I think we can follow net and scsi vhost devices, so we can set a 
VHOST_VSOCK_VQ_MAX(5), allocates all the queues in any case and then use 
only the queues acked by the guest.

>
>I tried to test with the host kernel allocating 4 vqs, but qemu only
>allocated 2 vqs, and
>guest kernel will not be able to send even the vsock stream packets. I
>think the host
>kernel and the qemu have to agree on the number of vhost_vqs. Do you agree?
>Did I miss something?

Mmm, I need to check, but for example vhost-net calls vhost_dev_init() 
with VHOST_NET_VQ_MAX, but then the guest can decide to use only one 
couple of TX and RX queues.

I'm not sure about qemu point of view, but I expected that QEMU can set 
less queues then queues allocated by the kernel. `vhost_dev.nvqs` should 
be set with the amount of queue that QEMU can handle.

>
>Another idea to make the setting in runtime instead of compiling time 
>is to use
>qemu cmd-line options, then qemu can allocate 2 or 4 queues depending 
>on
>the cmd line. This will solve the issue when the host kernel is an old
>one( no dgram
>support) and the qemu is a new one.

I don't think this is a good idea, at most we can add an ioctl that qemu 
can use to query the kernel about allocated queues, but I still need to 
understand better if we really we need this.

>
>But there is still an issue when the host kernel is a new one, while 
>the qemu
>is an old one.  I am not sure how to make the virtqueues numbers to
>change in run-time
>for the host kernel. In another email thread, you mentioned removing kconfig
>in the linux kernel, I believe that is related to this qemu patch,
>right? 

It was related to both, I don't think we should build QEMU and Linux 
with or without dgram support.

> If so,
>any ideas that I can make the host kernel change the number of vqs in
>the run-time
>or when starting up vsock? The only way I can think of is to use a
>kernel module parameter
>for the vsock_vhost module. Any other ideas? Thanks.

I need to check better, but we should be able to do all at run time 
looking at the features field. As I said, both QEMU and kernel can 
allocate the maximum number of queues that they can handle, then enable 
only the queues allocated by the guest (e.g. during 
vhost_vsock_common_start()).

>
>btw, I searched Linux kernel code but did not find any examples.
>

I'm a bit busy this week, I'll try to write some PoC next week if you 
can't find a working solution. (without any #ifdef :-)

Thanks,
Stefano

Re: [External] Re: [RFC v1] virtio/vsock: add two more queues for datagram types

[Jiang Wang .]

On Thu, Jun 24, 2021 at 7:31 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Wed, Jun 23, 2021 at 11:50:33PM -0700, Jiang Wang . wrote:
> >Hi Stefano,
> >
> >I checked virtio_net_set_multiqueue(), which will help with following
> >changes in my patch:
> >
> >#ifdef CONFIG_VHOST_VSOCK_DGRAM
> >vvc->dgram_recv_vq = virtio_add_queue(vdev, VHOST_VSOCK_QUEUE_SIZE,
> >vhost_vsock_common_handle_output);
> >vvc->dgram_trans_vq = virtio_add_queue(vdev, VHOST_VSOCK_QUEUE_SIZE,
> >vhost_vsock_common_handle_output);
> >#endif
> >
> >But I think there is still an issue with the following lines, right?
>
> Yep, I think so.
>
> >
> >#ifdef CONFIG_VHOST_VSOCK_DGRAM
> >struct vhost_virtqueue vhost_vqs[4];
> >#else
> >struct vhost_virtqueue vhost_vqs[2];
> >#endif
> >
> >I think the problem with feature bits is that they are set and get after
> >vhost_vsock_common_realize() and after vhost_dev_init() in drivers/vhost/vsock.c
> >But those virtqueues need to be set up correctly beforehand.
>
> I think we can follow net and scsi vhost devices, so we can set a
> VHOST_VSOCK_VQ_MAX(5), allocates all the queues in any case and then use
> only the queues acked by the guest.
>
Thanks for the advice. I checked both net and scsi and scsi is more helpful.

> >
> >I tried to test with the host kernel allocating 4 vqs, but qemu only
> >allocated 2 vqs, and
> >guest kernel will not be able to send even the vsock stream packets. I
> >think the host
> >kernel and the qemu have to agree on the number of vhost_vqs. Do you agree?
> >Did I miss something?
>
> Mmm, I need to check, but for example vhost-net calls vhost_dev_init()
> with VHOST_NET_VQ_MAX, but then the guest can decide to use only one
> couple of TX and RX queues.
>
> I'm not sure about qemu point of view, but I expected that QEMU can set
> less queues then queues allocated by the kernel. `vhost_dev.nvqs` should
> be set with the amount of queue that QEMU can handle.
>
I checked that vhost_dev.nvqs is still the maximum number of queues (4 queues).
But I found a way to workaround it. More details in the following text.

> >
> >Another idea to make the setting in runtime instead of compiling time
> >is to use
> >qemu cmd-line options, then qemu can allocate 2 or 4 queues depending
> >on
> >the cmd line. This will solve the issue when the host kernel is an old
> >one( no dgram
> >support) and the qemu is a new one.
>
> I don't think this is a good idea, at most we can add an ioctl that qemu
> can use to query the kernel about allocated queues, but I still need to
> understand better if we really we need this.
>

Hmm. Both net and scsi use the qemu cmd line option to configure
number of queues. Qemu cmdline is a runtime setting and flexible.
I think qemu cmdline is better than ioctl. I also make the qemu cmd
line option default to only allocate two queues to be compatible with
old versions.

> >
> >But there is still an issue when the host kernel is a new one, while
> >the qemu
> >is an old one.  I am not sure how to make the virtqueues numbers to
> >change in run-time
> >for the host kernel. In another email thread, you mentioned removing kconfig
> >in the linux kernel, I believe that is related to this qemu patch,
> >right?
>
> It was related to both, I don't think we should build QEMU and Linux
> with or without dgram support.
>
> > If so,
> >any ideas that I can make the host kernel change the number of vqs in
> >the run-time
> >or when starting up vsock? The only way I can think of is to use a
> >kernel module parameter
> >for the vsock_vhost module. Any other ideas? Thanks.
>
> I need to check better, but we should be able to do all at run time
> looking at the features field. As I said, both QEMU and kernel can
> allocate the maximum number of queues that they can handle, then enable
> only the queues allocated by the guest (e.g. during
> vhost_vsock_common_start()).
>

Yes. I checked the code and found there is an implementation bug ( or
limitation) in drivers/vhost/vsock.c. In vhost_vsock_start(), if a queue
failed to init, the code will clean up all previous successfully
allocated queues. That is why V1 code does not work when
host kernel is new,  but qemu and guest kernel is old. I made a change
there and it works now. I will clean up the patch a little bit and
send V2 soon.


> >
> >btw, I searched Linux kernel code but did not find any examples.
> >
>
> I'm a bit busy this week, I'll try to write some PoC next week if you
> can't find a working solution. (without any #ifdef :-)
>
> Thanks,
> Stefano
>

Re: [External] Re: [RFC v1] virtio/vsock: add two more queues for datagram types

[Stefano Garzarella]

On Wed, Jun 30, 2021 at 03:44:17PM -0700, Jiang Wang . wrote:
>On Thu, Jun 24, 2021 at 7:31 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
>>
>> On Wed, Jun 23, 2021 at 11:50:33PM -0700, Jiang Wang . wrote:
>> >Hi Stefano,
>> >
>> >I checked virtio_net_set_multiqueue(), which will help with following
>> >changes in my patch:
>> >
>> >#ifdef CONFIG_VHOST_VSOCK_DGRAM
>> >vvc->dgram_recv_vq = virtio_add_queue(vdev, VHOST_VSOCK_QUEUE_SIZE,
>> >vhost_vsock_common_handle_output);
>> >vvc->dgram_trans_vq = virtio_add_queue(vdev, VHOST_VSOCK_QUEUE_SIZE,
>> >vhost_vsock_common_handle_output);
>> >#endif
>> >
>> >But I think there is still an issue with the following lines, right?
>>
>> Yep, I think so.
>>
>> >
>> >#ifdef CONFIG_VHOST_VSOCK_DGRAM
>> >struct vhost_virtqueue vhost_vqs[4];
>> >#else
>> >struct vhost_virtqueue vhost_vqs[2];
>> >#endif
>> >
>> >I think the problem with feature bits is that they are set and get after
>> >vhost_vsock_common_realize() and after vhost_dev_init() in drivers/vhost/vsock.c
>> >But those virtqueues need to be set up correctly beforehand.
>>
>> I think we can follow net and scsi vhost devices, so we can set a
>> VHOST_VSOCK_VQ_MAX(5), allocates all the queues in any case and then use
>> only the queues acked by the guest.
>>
>Thanks for the advice. I checked both net and scsi and scsi is more helpful.

Yeah :-)

>
>> >
>> >I tried to test with the host kernel allocating 4 vqs, but qemu only
>> >allocated 2 vqs, and
>> >guest kernel will not be able to send even the vsock stream packets. I
>> >think the host
>> >kernel and the qemu have to agree on the number of vhost_vqs. Do you agree?
>> >Did I miss something?
>>
>> Mmm, I need to check, but for example vhost-net calls vhost_dev_init()
>> with VHOST_NET_VQ_MAX, but then the guest can decide to use only one
>> couple of TX and RX queues.
>>
>> I'm not sure about qemu point of view, but I expected that QEMU can set
>> less queues then queues allocated by the kernel. `vhost_dev.nvqs` should
>> be set with the amount of queue that QEMU can handle.
>>
>I checked that vhost_dev.nvqs is still the maximum number of queues (4 queues).
>But I found a way to workaround it. More details in the following text.
>
>> >
>> >Another idea to make the setting in runtime instead of compiling time
>> >is to use
>> >qemu cmd-line options, then qemu can allocate 2 or 4 queues depending
>> >on
>> >the cmd line. This will solve the issue when the host kernel is an old
>> >one( no dgram
>> >support) and the qemu is a new one.
>>
>> I don't think this is a good idea, at most we can add an ioctl that qemu
>> can use to query the kernel about allocated queues, but I still need to
>> understand better if we really we need this.
>>
>
>Hmm. Both net and scsi use the qemu cmd line option to configure
>number of queues. Qemu cmdline is a runtime setting and flexible.
>I think qemu cmdline is better than ioctl. I also make the qemu cmd
>line option default to only allocate two queues to be compatible with
>old versions.

Can we avoid both and allocate the maximum number of queue that QEMU can 
handle?

I'm not sure that adding a parameter to QEMU is a good idea. If possible 
it should be automatic.

>
>> >
>> >But there is still an issue when the host kernel is a new one, while
>> >the qemu
>> >is an old one.  I am not sure how to make the virtqueues numbers to
>> >change in run-time
>> >for the host kernel. In another email thread, you mentioned removing 
>> >kconfig
>> >in the linux kernel, I believe that is related to this qemu patch,
>> >right?
>>
>> It was related to both, I don't think we should build QEMU and Linux
>> with or without dgram support.
>>
>> > If so,
>> >any ideas that I can make the host kernel change the number of vqs 
>> >in
>> >the run-time
>> >or when starting up vsock? The only way I can think of is to use a
>> >kernel module parameter
>> >for the vsock_vhost module. Any other ideas? Thanks.
>>
>> I need to check better, but we should be able to do all at run time
>> looking at the features field. As I said, both QEMU and kernel can
>> allocate the maximum number of queues that they can handle, then 
>> enable
>> only the queues allocated by the guest (e.g. during
>> vhost_vsock_common_start()).
>>
>
>Yes. I checked the code and found there is an implementation bug ( or
>limitation) in drivers/vhost/vsock.c. In vhost_vsock_start(), if a 
>queue
>failed to init, the code will clean up all previous successfully
>allocated queues. That is why V1 code does not work when
>host kernel is new,  but qemu and guest kernel is old. I made a change
>there and it works now. I will clean up the patch a little bit and
>send V2 soon.

Great! I'll review the new version!

Thanks,
Stefano

Re: [External] Re: [RFC v1] virtio/vsock: add two more queues for datagram types

[Jiang Wang .]

On Wed, Jun 30, 2021 at 11:55 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Wed, Jun 30, 2021 at 03:44:17PM -0700, Jiang Wang . wrote:
> >On Thu, Jun 24, 2021 at 7:31 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
> >>
> >> On Wed, Jun 23, 2021 at 11:50:33PM -0700, Jiang Wang . wrote:
> >> >Hi Stefano,
> >> >
> >> >I checked virtio_net_set_multiqueue(), which will help with following
> >> >changes in my patch:
> >> >
> >> >#ifdef CONFIG_VHOST_VSOCK_DGRAM
> >> >vvc->dgram_recv_vq = virtio_add_queue(vdev, VHOST_VSOCK_QUEUE_SIZE,
> >> >vhost_vsock_common_handle_output);
> >> >vvc->dgram_trans_vq = virtio_add_queue(vdev, VHOST_VSOCK_QUEUE_SIZE,
> >> >vhost_vsock_common_handle_output);
> >> >#endif
> >> >
> >> >But I think there is still an issue with the following lines, right?
> >>
> >> Yep, I think so.
> >>
> >> >
> >> >#ifdef CONFIG_VHOST_VSOCK_DGRAM
> >> >struct vhost_virtqueue vhost_vqs[4];
> >> >#else
> >> >struct vhost_virtqueue vhost_vqs[2];
> >> >#endif
> >> >
> >> >I think the problem with feature bits is that they are set and get after
> >> >vhost_vsock_common_realize() and after vhost_dev_init() in drivers/vhost/vsock.c
> >> >But those virtqueues need to be set up correctly beforehand.
> >>
> >> I think we can follow net and scsi vhost devices, so we can set a
> >> VHOST_VSOCK_VQ_MAX(5), allocates all the queues in any case and then use
> >> only the queues acked by the guest.
> >>
> >Thanks for the advice. I checked both net and scsi and scsi is more helpful.
>
> Yeah :-)
>
> >
> >> >
> >> >I tried to test with the host kernel allocating 4 vqs, but qemu only
> >> >allocated 2 vqs, and
> >> >guest kernel will not be able to send even the vsock stream packets. I
> >> >think the host
> >> >kernel and the qemu have to agree on the number of vhost_vqs. Do you agree?
> >> >Did I miss something?
> >>
> >> Mmm, I need to check, but for example vhost-net calls vhost_dev_init()
> >> with VHOST_NET_VQ_MAX, but then the guest can decide to use only one
> >> couple of TX and RX queues.
> >>
> >> I'm not sure about qemu point of view, but I expected that QEMU can set
> >> less queues then queues allocated by the kernel. `vhost_dev.nvqs` should
> >> be set with the amount of queue that QEMU can handle.
> >>
> >I checked that vhost_dev.nvqs is still the maximum number of queues (4 queues).
> >But I found a way to workaround it. More details in the following text.
> >
> >> >
> >> >Another idea to make the setting in runtime instead of compiling time
> >> >is to use
> >> >qemu cmd-line options, then qemu can allocate 2 or 4 queues depending
> >> >on
> >> >the cmd line. This will solve the issue when the host kernel is an old
> >> >one( no dgram
> >> >support) and the qemu is a new one.
> >>
> >> I don't think this is a good idea, at most we can add an ioctl that qemu
> >> can use to query the kernel about allocated queues, but I still need to
> >> understand better if we really we need this.
> >>
> >
> >Hmm. Both net and scsi use the qemu cmd line option to configure
> >number of queues. Qemu cmdline is a runtime setting and flexible.
> >I think qemu cmdline is better than ioctl. I also make the qemu cmd
> >line option default to only allocate two queues to be compatible with
> >old versions.
>
> Can we avoid both and allocate the maximum number of queue that QEMU can
> handle?
>
I did not find any way to do that. When the host kernel is new, QEMU
can handle 4 queues. When the host kernel is an old one, QEMU can
only handle 2 queues.

> I'm not sure that adding a parameter to QEMU is a good idea. If possible
> it should be automatic.
>

OK. I will keep that in mind and dig more to see if there is a way to
do that.

Thanks.

> >
> >> >
> >> >But there is still an issue when the host kernel is a new one, while
> >> >the qemu
> >> >is an old one.  I am not sure how to make the virtqueues numbers to
> >> >change in run-time
> >> >for the host kernel. In another email thread, you mentioned removing
> >> >kconfig
> >> >in the linux kernel, I believe that is related to this qemu patch,
> >> >right?
> >>
> >> It was related to both, I don't think we should build QEMU and Linux
> >> with or without dgram support.
> >>
> >> > If so,
> >> >any ideas that I can make the host kernel change the number of vqs
> >> >in
> >> >the run-time
> >> >or when starting up vsock? The only way I can think of is to use a
> >> >kernel module parameter
> >> >for the vsock_vhost module. Any other ideas? Thanks.
> >>
> >> I need to check better, but we should be able to do all at run time
> >> looking at the features field. As I said, both QEMU and kernel can
> >> allocate the maximum number of queues that they can handle, then
> >> enable
> >> only the queues allocated by the guest (e.g. during
> >> vhost_vsock_common_start()).
> >>
> >
> >Yes. I checked the code and found there is an implementation bug ( or
> >limitation) in drivers/vhost/vsock.c. In vhost_vsock_start(), if a
> >queue
> >failed to init, the code will clean up all previous successfully
> >allocated queues. That is why V1 code does not work when
> >host kernel is new,  but qemu and guest kernel is old. I made a change
> >there and it works now. I will clean up the patch a little bit and
> >send V2 soon.
>
> Great! I'll review the new version!
>
> Thanks,
> Stefano
>