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

[Stefano Garzarella]

On Thu, Jul 01, 2021 at 09:49:10PM +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.
>
>
> hw/virtio/vhost-user-vsock.c                  |  6 +++---
> hw/virtio/vhost-vsock-common.c                | 23 ++++++++++++++++++-----
> hw/virtio/vhost-vsock.c                       | 11 ++++++++---
> include/hw/virtio/vhost-vsock-common.h        |  8 +++++---
> include/hw/virtio/vhost-vsock.h               |  1 +
> include/standard-headers/linux/virtio_vsock.h |  3 +++
> 6 files changed, 38 insertions(+), 14 deletions(-)
>
> diff --git a/hw/virtio/vhost-user-vsock.c b/hw/virtio/vhost-user-vsock.c
> index a6f08c26b9..409286aa8d 100644
> --- a/hw/virtio/vhost-user-vsock.c
> +++ b/hw/virtio/vhost-user-vsock.c
> @@ -103,7 +103,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);
>
> @@ -126,7 +126,7 @@ static void vuv_device_realize(DeviceState *dev, Error 
> **errp)
> err_vhost_dev:
>     vhost_dev_cleanup(&vvc->vhost_dev);
> err_virtio:
> -    vhost_vsock_common_unrealize(vdev);
> +    vhost_vsock_common_unrealize(vdev, false);
>     vhost_user_cleanup(&vsock->vhost_user);
>     return;
> }
> @@ -142,7 +142,7 @@ static void vuv_device_unrealize(DeviceState *dev)
>
>     vhost_dev_cleanup(&vvc->vhost_dev);
>
> -    vhost_vsock_common_unrealize(vdev);
> +    vhost_vsock_common_unrealize(vdev, false);
>
>     vhost_user_cleanup(&vsock->vhost_user);
>
> diff --git a/hw/virtio/vhost-vsock-common.c b/hw/virtio/vhost-vsock-common.c
> index 4ad6e234ad..a36cb36496 100644
> --- a/hw/virtio/vhost-vsock-common.c
> +++ b/hw/virtio/vhost-vsock-common.c
> @@ -196,9 +196,10 @@ int vhost_vsock_common_post_load(void *opaque, int 
> version_id)
>     return 0;
> }
>
> -void vhost_vsock_common_realize(VirtIODevice *vdev, const char *name)
> +void vhost_vsock_common_realize(VirtIODevice *vdev, const char *name, bool 
> enable_dgram)
> {
>     VHostVSockCommon *vvc = VHOST_VSOCK_COMMON(vdev);
> +    int nvq = 2;
>
>     virtio_init(vdev, name, VIRTIO_ID_VSOCK,
>                 sizeof(struct virtio_vsock_config));
> @@ -209,17 +210,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);
>
> -    /* The event queue belongs to QEMU */
> +    if (enable_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);
> +       nvq = 4;
> +    }
> +           /* The event queue belongs to QEMU */
>     vvc->event_vq = virtio_add_queue(vdev, VHOST_VSOCK_QUEUE_SIZE,
>                                        vhost_vsock_common_handle_output);

What happen if the guest doesn't support dgram?

I think we should dynamically use the 3rd queue or the 5th queue for the 
events at runtime after the guest acked the features.

Maybe better to switch to an array of VirtQueue.

> -    vvc->vhost_dev.nvqs = ARRAY_SIZE(vvc->vhost_vqs);
> -    vvc->vhost_dev.vqs = vvc->vhost_vqs;
> +    vvc->vhost_dev.nvqs = nvq;
> +    vvc->vhost_dev.vqs = g_new0(struct vhost_virtqueue, vvc->vhost_dev.nvqs);

Where do we free this?

>     vvc->post_load_timer = NULL;
> }
>
> -void vhost_vsock_common_unrealize(VirtIODevice *vdev)
> +void vhost_vsock_common_unrealize(VirtIODevice *vdev, bool enable_dgram)
> {
>     VHostVSockCommon *vvc = VHOST_VSOCK_COMMON(vdev);
>
> @@ -227,6 +235,11 @@ void vhost_vsock_common_unrealize(VirtIODevice *vdev)
>
>     virtio_delete_queue(vvc->recv_vq);
>     virtio_delete_queue(vvc->trans_vq);
> +    if (enable_dgram) {
> +        virtio_delete_queue(vvc->dgram_recv_vq);
> +        virtio_delete_queue(vvc->dgram_trans_vq);
> +    }
> +
>     virtio_delete_queue(vvc->event_vq);
>     virtio_cleanup(vdev);
> }
> diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c
> index 5eaa207504..d15c672c38 100644
> --- a/hw/virtio/vhost-vsock.c
> +++ b/hw/virtio/vhost-vsock.c
> @@ -23,6 +23,7 @@
>
> const int feature_bits[] = {
>     VIRTIO_VSOCK_F_SEQPACKET,
> +    VIRTIO_VSOCK_F_DGRAM,
>     VHOST_INVALID_FEATURE_BIT
> };
>
> @@ -116,6 +117,8 @@ static uint64_t vhost_vsock_get_features(VirtIODevice *vdev,
>     VHostVSockCommon *vvc = VHOST_VSOCK_COMMON(vdev);
>
>     virtio_add_feature(&requested_features, VIRTIO_VSOCK_F_SEQPACKET);
> +    if (vvc->vhost_dev.nvqs == 4) /* 4 means has dgram support */
> +        virtio_add_feature(&requested_features, VIRTIO_VSOCK_F_DGRAM);
>     return vhost_get_features(&vvc->vhost_dev, feature_bits,
>                                 requested_features);
> }
> @@ -175,7 +178,7 @@ static void vhost_vsock_device_realize(DeviceState *dev, 
> Error **errp)
>         qemu_set_nonblock(vhostfd);
>     }
>
> -    vhost_vsock_common_realize(vdev, "vhost-vsock");
> +    vhost_vsock_common_realize(vdev, "vhost-vsock", vsock->conf.enable_dgram);
>
>     ret = vhost_dev_init(&vvc->vhost_dev, (void *)(uintptr_t)vhostfd,
>                          VHOST_BACKEND_TYPE_KERNEL, 0);
> @@ -197,7 +200,7 @@ err_vhost_dev:
>     /* vhost_dev_cleanup() closes the vhostfd passed to vhost_dev_init() */
>     vhostfd = -1;
> err_virtio:
> -    vhost_vsock_common_unrealize(vdev);
> +    vhost_vsock_common_unrealize(vdev, vsock->conf.enable_dgram);
>     if (vhostfd >= 0) {
>         close(vhostfd);
>     }
> @@ -208,17 +211,19 @@ static void vhost_vsock_device_unrealize(DeviceState *dev)
> {
>     VHostVSockCommon *vvc = VHOST_VSOCK_COMMON(dev);
>     VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> +    VHostVSock *vsock = VHOST_VSOCK(dev);
>
>     /* This will stop vhost backend if appropriate. */
>     vhost_vsock_set_status(vdev, 0);
>
>     vhost_dev_cleanup(&vvc->vhost_dev);
> -    vhost_vsock_common_unrealize(vdev);
> +    vhost_vsock_common_unrealize(vdev, vsock->conf.enable_dgram);
> }
>
> static Property vhost_vsock_properties[] = {
>     DEFINE_PROP_UINT64("guest-cid", VHostVSock, conf.guest_cid, 0),
>     DEFINE_PROP_STRING("vhostfd", VHostVSock, conf.vhostfd),
> +    DEFINE_PROP_BOOL("enable_dgram", VHostVSock, conf.enable_dgram, 
> false),

I think we can avoid this and query the vhost device features before 
vhost_vsock_common_realize().

Take a look at vhost_vdpa_get_max_qps() implemented by Jason here:
https://lore.kernel.org/qemu-devel/20210621041650.5826-19-jasowang@redhat.com/

We can do something similar to discover if the vhost-vsock device 
supports 2 or 4 queues checking if VIRTIO_VSOCK_F_DGRAM is set or not.

Thanks,
Stefano