Re: [PATCH v4] virtio/vsock: add two more queues for datagram types

[Stefano Garzarella]

On Thu, Aug 05, 2021 at 12:07:02PM -0700, Jiang Wang . wrote:
> On Wed, Aug 4, 2021 at 1:13 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
> > On Tue, Aug 03, 2021 at 11:41:32PM +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.
> > >
> > >Signed-off-by: Jiang Wang <jiang.wang@bytedance.com>
> > >---
> > >v1 -> v2: use qemu cmd option to control number of queues,
> > >       removed configuration settings for dgram.
> > >v2 -> v3: use ioctl to get features and decide number of
> > >        virt queues, instead of qemu cmd option.
> > >v3 -> v4: change DGRAM feature bit value to 2. Add an argument
> > >       in vhost_vsock_common_realize to indicate dgram is supported or not.
> > >
> > > hw/virtio/vhost-user-vsock.c                  |  2 +-
> > > hw/virtio/vhost-vsock-common.c                | 58 ++++++++++++++++++-
> > > hw/virtio/vhost-vsock.c                       |  5 +-
> > > include/hw/virtio/vhost-vsock-common.h        |  6 +-
> > > include/hw/virtio/vhost-vsock.h               |  4 ++
> > > include/standard-headers/linux/virtio_vsock.h |  1 +
> > > 6 files changed, 69 insertions(+), 7 deletions(-)
> > >
> > >diff --git a/hw/virtio/vhost-user-vsock.c b/hw/virtio/vhost-user-vsock.c
> > >index 6095ed7349..e9ec0e1c00 100644
> > >--- a/hw/virtio/vhost-user-vsock.c
> > >+++ b/hw/virtio/vhost-user-vsock.c
> > >@@ -105,7 +105,7 @@ static void vuv_device_realize(DeviceState *dev, Error 
> > **errp)
> > >         return;
> > >     }
> > >
> > >-    vhost_vsock_common_realize(vdev, "vhost-user-vsock");
> > >+    vhost_vsock_common_realize(vdev, "vhost-user-vsock", false);
> > >
> > >     vhost_dev_set_config_notifier(&vvc->vhost_dev, &vsock_ops);
> > >
> > >diff --git a/hw/virtio/vhost-vsock-common.c b/hw/virtio/vhost-vsock-common.c
> > >index 4ad6e234ad..c78536911a 100644
> > >--- a/hw/virtio/vhost-vsock-common.c
> > >+++ b/hw/virtio/vhost-vsock-common.c
> > >@@ -17,6 +17,8 @@
> > > #include "hw/virtio/vhost-vsock.h"
> > > #include "qemu/iov.h"
> > > #include "monitor/monitor.h"
> > >+#include <sys/ioctl.h>
> > >+#include <linux/vhost.h>
> > >
> > > int vhost_vsock_common_start(VirtIODevice *vdev)
> > > {
> > >@@ -196,9 +198,39 @@ int vhost_vsock_common_post_load(void *opaque, int 
> > version_id)
> > >     return 0;
> > > }
> > >
> > >-void vhost_vsock_common_realize(VirtIODevice *vdev, const char *name)
> > >+static int vhost_vsock_get_max_qps(bool enable_dgram)
> > >+{
> > >+    uint64_t features;
> > >+    int ret;
> > >+    int fd = -1;
> > >+
> > >+    if (!enable_dgram)
> > >+        return MAX_VQS_WITHOUT_DGRAM;
> > >+
> > >+    fd = qemu_open_old("/dev/vhost-vsock", O_RDONLY);
> >
> >
> > As I said in the previous version, we cannot directly open
> > /dev/vhost-vsock, for two reasons:
> >
> >    1. this code is common with vhost-user-vsock which does not use
> >    /dev/vhost-vsock.
> >
> >    2. the fd may have been passed from the management layer and qemu may
> >    not be able to directly open /dev/vhost-vsock.
> >
> > I think is better to move this function in hw/virtio/vhost-vsock.c,
> > using the `vhostfd`, returning true or false if dgram is supported, then
> > you can use it for `enable_dgram` param ...
>
> Yes, you are right. Now I remember you said that before but I forgot about that
> when I changed the code. I will fix it. Sorry about that.

No problem :-)

> > >+    if (fd == -1) {
> > >+        error_report("vhost-vsock: failed to open device. %s", 
> > strerror(errno));
> > >+        return -1;
> > >+    }
> > >+
> > >+    ret = ioctl(fd, VHOST_GET_FEATURES, &features);
> > >+    if (ret) {
> > >+        error_report("vhost-vsock: failed to read  device. %s", 
> > strerror(errno));
> > >+        qemu_close(fd);
> > >+        return ret;
> > >+    }
> > >+
> > >+    qemu_close(fd);
> > >+    if (features & (1 << VIRTIO_VSOCK_F_DGRAM))
> > >+        return MAX_VQS_WITH_DGRAM;
> > >+
> > >+    return MAX_VQS_WITHOUT_DGRAM;
> > >+}
> > >+
> > >+void vhost_vsock_common_realize(VirtIODevice *vdev, const char *name, bool 
> > enable_dgram)
> > > {
> > >     VHostVSockCommon *vvc = VHOST_VSOCK_COMMON(vdev);
> > >+    int nvqs = MAX_VQS_WITHOUT_DGRAM;
> > >
> > >     virtio_init(vdev, name, VIRTIO_ID_VSOCK,
> > >                 sizeof(struct virtio_vsock_config));
> > >@@ -209,12 +241,24 @@ void vhost_vsock_common_realize(VirtIODevice
> > >*vdev, const char *name)
> > >     vvc->trans_vq = virtio_add_queue(vdev, VHOST_VSOCK_QUEUE_SIZE,
> > >                                        vhost_vsock_common_handle_output);
> > >
> > >+    nvqs = vhost_vsock_get_max_qps(enable_dgram);
> > >+
> > >+    if (nvqs < 0)
> > >+        nvqs = MAX_VQS_WITHOUT_DGRAM;
> >
> > ... and here, if `enable_dgram` is true, you can set `nvqs =
> > MAX_VQS_WITH_DGRAM``
> sure.
>
> > >+
> > >+    if (nvqs == MAX_VQS_WITH_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);
> > >+    }
> > >+
> >
> > I'm still concerned about compatibility with guests that don't 
> > support
> > dgram, as I mentioned in the previous email.
> >
> > I need to do some testing, but my guess is it won't work if the host
> > (QEMU and vhost-vsock) supports it and the guest doesn't.
> >
> > I still think that we should allocate an array of queues and then decide
> > at runtime which one to use for events (third or fifth) after the guest
> > has acked the features.
> Agree. I will check where the guest ack the feature. If you have any 

I'm not sure we should delete them, I think we can allocate 5 queues and 
then use queue 3 or 5 for events in vhost_vsock_common_start(), when the 
guest already acked the features.

> pointers,
> just let me know. Also, could we just remove the vq allocation in common_realize
> and do it at a later time? Or need to delete and add again as I mentioned in the
> previous email?

Instead of having 'recv_vq', 'trans_vq', 'event_vq' fields, we can have 
an array of VirtQueue pointers and a field that indicates what the index 
of the event queue is. (or a boolean that indicates if we are enabling 
dgram or not).

This should simplify the management.

Thanks,
Stefano