[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v4] virtio/vsock: add two more queues for datagram types
|
From: |
Stefano Garzarella |
|
Subject: |
Re: [PATCH v4] virtio/vsock: add two more queues for datagram types |
|
Date: |
Wed, 4 Aug 2021 10:13:49 +0200 |
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 ...
+ 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``
+
+ 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.
/* The event queue belongs to QEMU */
vvc->event_vq = virtio_add_queue(vdev, VHOST_VSOCK_QUEUE_SIZE,
vhost_vsock_common_handle_output);
- 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 +271,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(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..f73523afaf 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);
}
@@ -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", true);
ret = vhost_dev_init(&vvc->vhost_dev, (void *)(uintptr_t)vhostfd,
VHOST_BACKEND_TYPE_KERNEL, 0, errp);
diff --git a/include/hw/virtio/vhost-vsock-common.h
b/include/hw/virtio/vhost-vsock-common.h
index e412b5ee98..6669d24714 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;
};
@@ -41,7 +42,8 @@ int vhost_vsock_common_start(VirtIODevice *vdev);
void vhost_vsock_common_stop(VirtIODevice *vdev);
int vhost_vsock_common_pre_save(void *opaque);
int vhost_vsock_common_post_load(void *opaque, int version_id);
-void vhost_vsock_common_realize(VirtIODevice *vdev, const char *name);
+void vhost_vsock_common_realize(VirtIODevice *vdev, const char *name,
+ bool enable_dgram);
void vhost_vsock_common_unrealize(VirtIODevice *vdev);
#endif /* _QEMU_VHOST_VSOCK_COMMON_H */
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;
This seems unused.
Thanks,
Stefano