On Wed, Jul 7, 2021 at 1:33 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
On Tue, Jul 06, 2021 at 10:26:07PM +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.
>---
>v1 -> v2: use qemu cmd option to control number of queues,
> removed configuration settings for dgram.
>v2 -> v3: use ioctl to get features and decie numbr of
> virt queues, instead of qemu cmd option.
>
>btw: this patch is based on Arseny's SEQPACKET patch.
>
> hw/virtio/vhost-vsock-common.c | 53 ++++++++++++++++++++++++++-
> hw/virtio/vhost-vsock.c | 3 ++
> include/hw/virtio/vhost-vsock-common.h | 3 +-
> include/hw/virtio/vhost-vsock.h | 4 ++
> include/standard-headers/linux/virtio_vsock.h | 3 ++
> 5 files changed, 63 insertions(+), 3 deletions(-)
>
>diff --git a/hw/virtio/vhost-vsock-common.c b/hw/virtio/vhost-vsock-common.c
>index 4ad6e234ad..8164e09445 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,36 @@ int vhost_vsock_common_post_load(void *opaque, int
version_id)
> return 0;
> }
>
>+static int vhost_vsock_get_max_qps(void)
>+{
>+ uint64_t features;
>+ int ret;
>+ int fd = -1;
>+
>+ fd = qemu_open_old("/dev/vhost-vsock", O_RDONLY);
>+ if (fd == -1) {
>+ error_report("vhost-vsock: failed to open device. %s",
strerror(errno));
>+ return -1;
>+ }
You should use the `vhostfd` already opened in
vhost_vsock_device_realize(), since QEMU may not have permission to
access to the device, and the file descriptor can be passed from the
management layer.
Sure. Will do.
>+
>+ 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)
> {
> 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 +238,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();
You can't do this here, since the vhost-vsock-common.c functions are
used also by vhost-user-vsock, that doesn't use the /dev/vhost-vsock
device since the device is emulated in a separate user process.
I think you can use something similar to what you did in v2, where you
passed a parameter to vhost_vsock_common_realize() to enable or not the
datagram queues.
Just to make sure, the qemu parameter will only be used for vhost-user-vsock,
right? I think for the vhost-vsock kernel module, we will use ioctl and ignore
the qemu parameter?
>+
>+ if (nvqs < 0)
>+ nvqs = MAX_VQS_WITHOUT_DGRAM;
>+
>+ 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);
>+ }
>+
> /* The event queue belongs to QEMU */
> vvc->event_vq = virtio_add_queue(vdev, VHOST_VSOCK_QUEUE_SIZE,
> vhost_vsock_common_handle_output);
Did you do a test with a guest that doesn't support datagram with QEMU
and hosts that do?
Yes, and it works.
I repost my thoughts that I had on v2:
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.
I think in current V3, it already dynamically use 3rd or 5th queue
depending
on the feature bit.
>
>- vvc->vhost_dev.nvqs = ARRAY_SIZE(vvc->vhost_vqs);
>- vvc->vhost_dev.vqs = vvc->vhost_vqs;
>+ vvc->vhost_dev.nvqs = nvqs;
>+ vvc->vhost_dev.vqs = g_new0(struct vhost_virtqueue, vvc->vhost_dev.nvqs);
>
> vvc->post_load_timer = NULL;
> }
>@@ -227,6 +268,14 @@ void vhost_vsock_common_unrealize(VirtIODevice *vdev)
>
> virtio_delete_queue(vvc->recv_vq);
> virtio_delete_queue(vvc->trans_vq);
>+ if (vvc->vhost_dev.nvqs == MAX_VQS_WITH_DGRAM) {
>+ virtio_delete_queue(vvc->dgram_recv_vq);
>+ virtio_delete_queue(vvc->dgram_trans_vq);
>+ }
>+
>+ if (vvc->vhost_dev.vqs)
g_free() already handles NULL pointers, so you can remove this check.
Got it.
>+ g_free(vvc->vhost_dev.vqs);
>+
> virtio_delete_queue(vvc->event_vq);
> virtio_cleanup(vdev);
> }
>diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c
>index 1b1a5c70ed..33bbe16983 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 == MAX_VQS_WITH_DGRAM)
>+ virtio_add_feature(&requested_features, VIRTIO_VSOCK_F_DGRAM);
> return vhost_get_features(&vvc->vhost_dev, feature_bits,
> requested_features);
> }
>diff --git a/include/hw/virtio/vhost-vsock-common.h
b/include/hw/virtio/vhost-vsock-common.h
>index e412b5ee98..798715241f 100644
>--- a/include/hw/virtio/vhost-vsock-common.h
>+++ b/include/hw/virtio/vhost-vsock-common.h
>@@ -27,12 +27,13 @@ enum {
> struct VHostVSockCommon {
> VirtIODevice parent;
>
>- struct vhost_virtqueue vhost_vqs[2];
> struct vhost_dev vhost_dev;
>
> VirtQueue *event_vq;
> VirtQueue *recv_vq;
> VirtQueue *trans_vq;
>+ VirtQueue *dgram_recv_vq;
>+ VirtQueue *dgram_trans_vq;
>
> QEMUTimer *post_load_timer;
> };
>diff --git a/include/hw/virtio/vhost-vsock.h b/include/hw/virtio/vhost-vsock.h
>index 84f4e727c7..e10319785d 100644
>--- a/include/hw/virtio/vhost-vsock.h
>+++ b/include/hw/virtio/vhost-vsock.h
>@@ -23,6 +23,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(VHostVSock, VHOST_VSOCK)
> typedef struct {
> uint64_t guest_cid;
> char *vhostfd;
>+ bool enable_dgram;
Leftover?
Right, but to support vhost-vsock-user, I think I will add this
parameter back?
> } VHostVSockConf;
>
> struct VHostVSock {
>@@ -33,4 +34,7 @@ struct VHostVSock {
> /*< public >*/
> };
>
>+#define MAX_VQS_WITHOUT_DGRAM 2
>+#define MAX_VQS_WITH_DGRAM 4
>+
> #endif /* QEMU_VHOST_VSOCK_H */
>diff --git a/include/standard-headers/linux/virtio_vsock.h
b/include/standard-headers/linux/virtio_vsock.h
>index 5eac522ee2..6ff8c5084c 100644
>--- a/include/standard-headers/linux/virtio_vsock.h
>+++ b/include/standard-headers/linux/virtio_vsock.h
>@@ -41,6 +41,9 @@
> /* The feature bitmap for virtio vsock */
> #define VIRTIO_VSOCK_F_SEQPACKET 1 /* SOCK_SEQPACKET
> supported */
>
>+/* Feature bits */
>+#define VIRTIO_VSOCK_F_DGRAM 0 /*Does vsock support dgram */
Bit 0 is reserved for STREAM, IIRC also in the virtio-spec proposal you
used bit 2 for DGRAM.
My bad, I did not update the code here yet.