* [RFC v7] virtio/vsock: add two more queues for datagram types
@ 2021-09-22 0:00 Jiang Wang
2021-09-22 9:23 ` Stefano Garzarella
0 siblings, 1 reply; 3+ messages in thread
From: Jiang Wang @ 2021-09-22 0:00 UTC (permalink / raw)
To: mst, qemu-devel; +Cc: jasowang, arseny.krasnov, stefanha, sgarzare
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 two new virtqueues are enabled by default and will
be removed if the guest does not support. This will help
migration work.
btw: enable_dgram argument in vhost_vsock_common_realize
is redundant for now, but will be used later when we
want to disable DGRAM feature bit for old versions.
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.
v4 -> v5: don't open dev to get vhostfd. Removed leftover definition of
enable_dgram
v5 -> v6: fix style errors. Imporve error handling of
vhost_vsock_dgram_supported. Rename MAX_VQS_WITH_DGRAM and another one.
v6 -> v7: Always enable dgram for vhost-user and vhost kernel.
Delete unused virtqueues at the beginning of
vhost_vsock_common_start for migration. Otherwise, migration will fail.
hw/virtio/vhost-user-vsock.c | 2 +-
hw/virtio/vhost-vsock-common.c | 32 +++++++++++++++++--
hw/virtio/vhost-vsock.c | 6 +++-
include/hw/virtio/vhost-vsock-common.h | 6 ++--
include/hw/virtio/vhost-vsock.h | 3 ++
include/standard-headers/linux/virtio_vsock.h | 1 +
6 files changed, 43 insertions(+), 7 deletions(-)
diff --git a/hw/virtio/vhost-user-vsock.c b/hw/virtio/vhost-user-vsock.c
index 6095ed7349..9823a2f3bd 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", true);
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..7d89b4d242 100644
--- a/hw/virtio/vhost-vsock-common.c
+++ b/hw/virtio/vhost-vsock-common.c
@@ -26,6 +26,18 @@ int vhost_vsock_common_start(VirtIODevice *vdev)
int ret;
int i;
+ if (!virtio_has_feature(vdev->guest_features, VIRTIO_VSOCK_F_DGRAM)) {
+ struct vhost_virtqueue *vqs;
+ virtio_delete_queue(vvc->dgram_recv_vq);
+ virtio_delete_queue(vvc->dgram_trans_vq);
+
+ vqs = vvc->vhost_dev.vqs;
+ vvc->vhost_dev.nvqs = MAX_VQS_WITHOUT_DGRAM;
+ vvc->vhost_dev.vqs = g_new0(struct vhost_virtqueue,
+ vvc->vhost_dev.nvqs);
+ g_free(vqs);
+ }
+
if (!k->set_guest_notifiers) {
error_report("binding does not support guest notifiers");
return -ENOSYS;
@@ -196,9 +208,11 @@ 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 nvqs = MAX_VQS_WITH_DGRAM;
virtio_init(vdev, name, VIRTIO_ID_VSOCK,
sizeof(struct virtio_vsock_config));
@@ -209,12 +223,17 @@ 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);
+ 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);
- 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 +246,13 @@ 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);
+ }
+
+ 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..6e315ecf23 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,9 @@ 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 +179,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..80151aee35 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..7d16c0e218 100644
--- a/include/hw/virtio/vhost-vsock.h
+++ b/include/hw/virtio/vhost-vsock.h
@@ -33,4 +33,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 3a23488e42..7e35acf3d4 100644
--- a/include/standard-headers/linux/virtio_vsock.h
+++ b/include/standard-headers/linux/virtio_vsock.h
@@ -40,6 +40,7 @@
/* The feature bitmap for virtio vsock */
#define VIRTIO_VSOCK_F_SEQPACKET 1 /* SOCK_SEQPACKET supported */
+#define VIRTIO_VSOCK_F_DGRAM 2 /* SOCK_DGRAM supported */
struct virtio_vsock_config {
uint64_t guest_cid;
--
2.20.1
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [RFC v7] virtio/vsock: add two more queues for datagram types
2021-09-22 0:00 [RFC v7] virtio/vsock: add two more queues for datagram types Jiang Wang
@ 2021-09-22 9:23 ` Stefano Garzarella
0 siblings, 0 replies; 3+ messages in thread
From: Stefano Garzarella @ 2021-09-22 9:23 UTC (permalink / raw)
To: Jiang Wang; +Cc: arseny.krasnov, jasowang, qemu-devel, stefanha, mst
On Wed, Sep 22, 2021 at 12:00: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 two new virtqueues are enabled by default and will
>be removed if the guest does not support. This will help
>migration work.
>
>btw: enable_dgram argument in vhost_vsock_common_realize
>is redundant for now, but will be used later when we
>want to disable DGRAM feature bit for old versions.
>
>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.
>v4 -> v5: don't open dev to get vhostfd. Removed leftover definition of
> enable_dgram
>v5 -> v6: fix style errors. Imporve error handling of
> vhost_vsock_dgram_supported. Rename MAX_VQS_WITH_DGRAM and another one.
>v6 -> v7: Always enable dgram for vhost-user and vhost kernel.
> Delete unused virtqueues at the beginning of
> vhost_vsock_common_start for migration. Otherwise, migration will fail.
>
> hw/virtio/vhost-user-vsock.c | 2 +-
> hw/virtio/vhost-vsock-common.c | 32 +++++++++++++++++--
> hw/virtio/vhost-vsock.c | 6 +++-
> include/hw/virtio/vhost-vsock-common.h | 6 ++--
> include/hw/virtio/vhost-vsock.h | 3 ++
> include/standard-headers/linux/virtio_vsock.h | 1 +
> 6 files changed, 43 insertions(+), 7 deletions(-)
>
>diff --git a/hw/virtio/vhost-user-vsock.c b/hw/virtio/vhost-user-vsock.c
>index 6095ed7349..9823a2f3bd 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", true);
>
> 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..7d89b4d242 100644
>--- a/hw/virtio/vhost-vsock-common.c
>+++ b/hw/virtio/vhost-vsock-common.c
>@@ -26,6 +26,18 @@ int vhost_vsock_common_start(VirtIODevice *vdev)
> int ret;
> int i;
>
>+ if (!virtio_has_feature(vdev->guest_features, VIRTIO_VSOCK_F_DGRAM)) {
>+ struct vhost_virtqueue *vqs;
>+ virtio_delete_queue(vvc->dgram_recv_vq);
>+ virtio_delete_queue(vvc->dgram_trans_vq);
>+
Are you sure it works removing queues here?
The event_queue would always be #4, but the guest will use #2 which
we're removing.
>+ vqs = vvc->vhost_dev.vqs;
>+ vvc->vhost_dev.nvqs = MAX_VQS_WITHOUT_DGRAM;
>+ vvc->vhost_dev.vqs = g_new0(struct vhost_virtqueue,
>+ vvc->vhost_dev.nvqs);
>+ g_free(vqs);
>+ }
>+
> if (!k->set_guest_notifiers) {
> error_report("binding does not support guest notifiers");
> return -ENOSYS;
>@@ -196,9 +208,11 @@ 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)
^
It seems always true, and also unused.
> {
> VHostVSockCommon *vvc = VHOST_VSOCK_COMMON(vdev);
>+ int nvqs = MAX_VQS_WITH_DGRAM;
>
> virtio_init(vdev, name, VIRTIO_ID_VSOCK,
> sizeof(struct virtio_vsock_config));
>@@ -209,12 +223,17 @@ 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);
>
>+ 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);
>
>- 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 +246,13 @@ 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);
>+ }
>+
>+ 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..6e315ecf23 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,9 @@ 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);
>+ }
Take a look at
https://lore.kernel.org/qemu-devel/20210921161642.206461-1-sgarzare@redhat.com/
If it will be accepted, we should use something similar (e.g.
`datagram` prop) and handle this flag in vhost-vsock-common.
And we should change a bit the queue handling:
- if QEMU (new `datagram` prop is `on` or `auto`) and the kernel
supports F_DGRAM, we can allocate 5 queues.
- if the guest acked F_DGRAM we can use queue #4 for events, otherwise
switch back to queue #2, without removing them.
> return vhost_get_features(&vvc->vhost_dev, feature_bits,
> requested_features);
> }
>@@ -175,7 +179,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..80151aee35 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..7d16c0e218 100644
>--- a/include/hw/virtio/vhost-vsock.h
>+++ b/include/hw/virtio/vhost-vsock.h
>@@ -33,4 +33,7 @@ struct VHostVSock {
> /*< public >*/
> };
>
>+#define MAX_VQS_WITHOUT_DGRAM 2
>+#define MAX_VQS_WITH_DGRAM 4
I think was better the naming in the previous version.
Thanks,
Stefano
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: Re: [RFC v7] virtio/vsock: add two more queues for datagram types
@ 2021-09-22 17:36 Jiang Wang .
2021-09-23 9:18 ` Stefano Garzarella
0 siblings, 1 reply; 3+ messages in thread
From: Jiang Wang . @ 2021-09-22 17:36 UTC (permalink / raw)
To: Stefano Garzarella
Cc: Arseny Krasnov, Jason Wang, qemu devel list, Stefan Hajnoczi,
Michael S. Tsirkin
On Wed, Sep 22, 2021 at 2:23 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Wed, Sep 22, 2021 at 12:00: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 two new virtqueues are enabled by default and will
> >be removed if the guest does not support. This will help
> >migration work.
> >
> >btw: enable_dgram argument in vhost_vsock_common_realize
> >is redundant for now, but will be used later when we
> >want to disable DGRAM feature bit for old versions.
> >
> >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.
> >v4 -> v5: don't open dev to get vhostfd. Removed leftover definition of
> > enable_dgram
> >v5 -> v6: fix style errors. Imporve error handling of
> > vhost_vsock_dgram_supported. Rename MAX_VQS_WITH_DGRAM and another one.
> >v6 -> v7: Always enable dgram for vhost-user and vhost kernel.
> > Delete unused virtqueues at the beginning of
> > vhost_vsock_common_start for migration. Otherwise, migration will fail.
> >
> > hw/virtio/vhost-user-vsock.c | 2 +-
> > hw/virtio/vhost-vsock-common.c | 32 +++++++++++++++++--
> > hw/virtio/vhost-vsock.c | 6 +++-
> > include/hw/virtio/vhost-vsock-common.h | 6 ++--
> > include/hw/virtio/vhost-vsock.h | 3 ++
> > include/standard-headers/linux/virtio_vsock.h | 1 +
> > 6 files changed, 43 insertions(+), 7 deletions(-)
> >
> >diff --git a/hw/virtio/vhost-user-vsock.c b/hw/virtio/vhost-user-vsock.c
> >index 6095ed7349..9823a2f3bd 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", true);
> >
> > 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..7d89b4d242 100644
> >--- a/hw/virtio/vhost-vsock-common.c
> >+++ b/hw/virtio/vhost-vsock-common.c
> >@@ -26,6 +26,18 @@ int vhost_vsock_common_start(VirtIODevice *vdev)
> > int ret;
> > int i;
> >
> >+ if (!virtio_has_feature(vdev->guest_features, VIRTIO_VSOCK_F_DGRAM)) {
> >+ struct vhost_virtqueue *vqs;
> >+ virtio_delete_queue(vvc->dgram_recv_vq);
> >+ virtio_delete_queue(vvc->dgram_trans_vq);
> >+
>
> Are you sure it works removing queues here?
> The event_queue would always be #4, but the guest will use #2 which
> we're removing.
>
Yes, this works. In fact, I should include this in v6 and my tests done
in my previous emails have these changes. But before I submitted the patch,
I thought this code was redundant and removed it without further testing.
To explain it, I think the event queue number does not matter for the
vhost and qemu. The vhost-vsock kernel module does not allocate any
data structure for the event queue. Qemu also only allocates an array of
size 2 or 4 for the tx, rx queues. The event queue is handled separately.
The event queue number only shows up in the spec, but in real code, I don't
see anywhere that the event queue number is used explicitly or implicitly.
The Event queue looks independent of tx, rx queues.
> >+ vqs = vvc->vhost_dev.vqs;
> >+ vvc->vhost_dev.nvqs = MAX_VQS_WITHOUT_DGRAM;
> >+ vvc->vhost_dev.vqs = g_new0(struct vhost_virtqueue,
> >+ vvc->vhost_dev.nvqs);
> >+ g_free(vqs);
> >+ }
> >+
> > if (!k->set_guest_notifiers) {
> > error_report("binding does not support guest notifiers");
> > return -ENOSYS;
> >@@ -196,9 +208,11 @@ 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)
> ^
> It seems always true, and also unused.
>
Yes, I can remove it. I am just wondering when we modify the feature
bit as in your recent seqpacket patch, do we want to change it to false when
the feature is not supported and make the code logically more resonable？
Or do we still make it true even if the feature bit is not supported?
> > {
> > VHostVSockCommon *vvc = VHOST_VSOCK_COMMON(vdev);
> >+ int nvqs = MAX_VQS_WITH_DGRAM;
> >
> > virtio_init(vdev, name, VIRTIO_ID_VSOCK,
> > sizeof(struct virtio_vsock_config));
> >@@ -209,12 +223,17 @@ 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);
> >
> >+ 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);
> >
> >- 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 +246,13 @@ 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);
> >+ }
> >+
> >+ 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..6e315ecf23 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,9 @@ 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);
> >+ }
>
> Take a look at
> https://lore.kernel.org/qemu-devel/20210921161642.206461-1-sgarzare@redhat.com/
>
Yes, I noticed that email. Thanks for reminding me. I did not make
similar changes yet because I want to wait for that patch to be merged.
I can start to make similar changes in the next version.
> If it will be accepted, we should use something similar (e.g.
> `datagram` prop) and handle this flag in vhost-vsock-common.
>
> And we should change a bit the queue handling:
> - if QEMU (new `datagram` prop is `on` or `auto`) and the kernel
> supports F_DGRAM, we can allocate 5 queues.
Agree with the new prop. But when the kernel supports F_DGRAM, qemu
still only allocates 4 queues. As in the following code：
vvc->vhost_dev.nvqs = nvqs;
vvc->vhost_dev.vqs = g_new0(struct vhost_virtqueue, vvc->vhost_dev.nvqs);
// nvqs will be either 2 or 4. Will not be 5. btw, in the new version, it will
// always be 4.
> - if the guest acked F_DGRAM we can use queue #4 for events, otherwise
> switch back to queue #2, without removing them.
>
> > return vhost_get_features(&vvc->vhost_dev, feature_bits,
> > requested_features);
> > }
> >@@ -175,7 +179,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..80151aee35 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..7d16c0e218 100644
> >--- a/include/hw/virtio/vhost-vsock.h
> >+++ b/include/hw/virtio/vhost-vsock.h
> >@@ -33,4 +33,7 @@ struct VHostVSock {
> > /*< public >*/
> > };
> >
> >+#define MAX_VQS_WITHOUT_DGRAM 2
> >+#define MAX_VQS_WITH_DGRAM 4
>
> I think was better the naming in the previous version.
>
Sure, will change it back in the next version.
> Thanks,
> Stefano
>
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [RFC v7] virtio/vsock: add two more queues for datagram types
2021-09-22 17:36 Jiang Wang .
@ 2021-09-23 9:18 ` Stefano Garzarella
0 siblings, 0 replies; 3+ messages in thread
From: Stefano Garzarella @ 2021-09-23 9:18 UTC (permalink / raw)
To: Jiang Wang .
Cc: Arseny Krasnov, Jason Wang, qemu devel list, Stefan Hajnoczi,
Michael S. Tsirkin
On Wed, Sep 22, 2021 at 10:36:24AM -0700, Jiang Wang . wrote:
>On Wed, Sep 22, 2021 at 2:23 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
>>
>> On Wed, Sep 22, 2021 at 12:00: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 two new virtqueues are enabled by default and will
>> >be removed if the guest does not support. This will help
>> >migration work.
>> >
>> >btw: enable_dgram argument in vhost_vsock_common_realize
>> >is redundant for now, but will be used later when we
>> >want to disable DGRAM feature bit for old versions.
>> >
>> >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.
>> >v4 -> v5: don't open dev to get vhostfd. Removed leftover definition of
>> > enable_dgram
>> >v5 -> v6: fix style errors. Imporve error handling of
>> > vhost_vsock_dgram_supported. Rename MAX_VQS_WITH_DGRAM and another one.
>> >v6 -> v7: Always enable dgram for vhost-user and vhost kernel.
>> > Delete unused virtqueues at the beginning of
>> > vhost_vsock_common_start for migration. Otherwise, migration will fail.
>> >
>> > hw/virtio/vhost-user-vsock.c | 2 +-
>> > hw/virtio/vhost-vsock-common.c | 32 +++++++++++++++++--
>> > hw/virtio/vhost-vsock.c | 6 +++-
>> > include/hw/virtio/vhost-vsock-common.h | 6 ++--
>> > include/hw/virtio/vhost-vsock.h | 3 ++
>> > include/standard-headers/linux/virtio_vsock.h | 1 +
>> > 6 files changed, 43 insertions(+), 7 deletions(-)
>> >
>> >diff --git a/hw/virtio/vhost-user-vsock.c b/hw/virtio/vhost-user-vsock.c
>> >index 6095ed7349..9823a2f3bd 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", true);
>> >
>> > 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..7d89b4d242 100644
>> >--- a/hw/virtio/vhost-vsock-common.c
>> >+++ b/hw/virtio/vhost-vsock-common.c
>> >@@ -26,6 +26,18 @@ int vhost_vsock_common_start(VirtIODevice *vdev)
>> > int ret;
>> > int i;
>> >
>> >+ if (!virtio_has_feature(vdev->guest_features, VIRTIO_VSOCK_F_DGRAM)) {
>> >+ struct vhost_virtqueue *vqs;
>> >+ virtio_delete_queue(vvc->dgram_recv_vq);
>> >+ virtio_delete_queue(vvc->dgram_trans_vq);
>> >+
>>
>> Are you sure it works removing queues here?
>> The event_queue would always be #4, but the guest will use #2 which
>> we're removing.
>>
>Yes, this works. In fact, I should include this in v6 and my tests done
>in my previous emails have these changes. But before I submitted the patch,
>I thought this code was redundant and removed it without further testing.
Just tried on an host that doesn't support F_DGRAM and I have the
following errors:
qemu-system-x86_64: -device vhost-vsock-pci,guest-cid=6: vhost_set_vring_call failed: No buffer space available (105)
qemu-system-x86_64: -device vhost-vsock-pci,guest-cid=6: Failed to initialize virtqueue 2: No buffer space available
I thinks we should re-add the feature discovery before call
vhost_dev_init().
>
>To explain it, I think the event queue number does not matter for the
>vhost and qemu. The vhost-vsock kernel module does not allocate any
>data structure for the event queue. Qemu also only allocates an array of
>size 2 or 4 for the tx, rx queues. The event queue is handled separately.
>
>The event queue number only shows up in the spec, but in real code, I don't
>see anywhere that the event queue number is used explicitly or implicitly.
>The Event queue looks independent of tx, rx queues.
Yep, it is used in the linux driver. Look at
virtio_transport_event_work(), it uses VSOCK_VQ_EVENT (2).
The main test to do is to migrate a guest with active connections that
doesn't support F_DGRAM on an host that support it and check if, after
the migration, the connections are reset and the CID updated.
I think is not working now.
>
>> >+ vqs = vvc->vhost_dev.vqs;
>> >+ vvc->vhost_dev.nvqs = MAX_VQS_WITHOUT_DGRAM;
>> >+ vvc->vhost_dev.vqs = g_new0(struct vhost_virtqueue,
>> >+ vvc->vhost_dev.nvqs);
>> >+ g_free(vqs);
>> >+ }
>> >+
>> > if (!k->set_guest_notifiers) {
>> > error_report("binding does not support guest notifiers");
>> > return -ENOSYS;
>> >@@ -196,9 +208,11 @@ 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)
>> ^
>> It seems always true, and also unused.
>>
>Yes, I can remove it. I am just wondering when we modify the feature
>bit as in your recent seqpacket patch, do we want to change it to false when
>the feature is not supported and make the code logically more
>resonable？
>Or do we still make it true even if the feature bit is not supported?
Maybe we need to use it because now vhost_dev_init is failing if the
host doesn't support F_DGRAM since we are registering more queues than
the device can handle.
>
>> > {
>> > VHostVSockCommon *vvc = VHOST_VSOCK_COMMON(vdev);
>> >+ int nvqs = MAX_VQS_WITH_DGRAM;
>> >
>> > virtio_init(vdev, name, VIRTIO_ID_VSOCK,
>> > sizeof(struct virtio_vsock_config));
>> >@@ -209,12 +223,17 @@ 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);
>> >
>> >+ 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);
>> >
>> >- 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 +246,13 @@ 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);
>> >+ }
>> >+
>> >+ 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..6e315ecf23 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,9 @@ 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);
>> >+ }
>>
>> Take a look at
>> https://lore.kernel.org/qemu-devel/20210921161642.206461-1-sgarzare@redhat.com/
>>
>Yes, I noticed that email. Thanks for reminding me. I did not make
>similar changes yet because I want to wait for that patch to be merged.
>I can start to make similar changes in the next version.
Yep, better to wait comments on that series.
>
>> If it will be accepted, we should use something similar (e.g.
>> `datagram` prop) and handle this flag in vhost-vsock-common.
>>
>> And we should change a bit the queue handling:
>> - if QEMU (new `datagram` prop is `on` or `auto`) and the kernel
>> supports F_DGRAM, we can allocate 5 queues.
>Agree with the new prop. But when the kernel supports F_DGRAM, qemu
>still only allocates 4 queues. As in the following code：
>
>vvc->vhost_dev.nvqs = nvqs;
>vvc->vhost_dev.vqs = g_new0(struct vhost_virtqueue,
>vvc->vhost_dev.nvqs);
>
>// nvqs will be either 2 or 4. Will not be 5. btw, in the new version, it will
>// always be 4.
Sorry, with allocating I meant virtio_add_queue() calls.
Just to be clear, I think we should do something like this:
#define VHOST_VSOCK_NVQS 3
#define VHOST_VSOCK_NVQS_DGRAM 5
#define VHOST_VSOCK_MAX_VQS VHOST_VSOCK_NVQS_DGRAM
struct VHostVSockCommon {
...
VirtQueue *vqs[VHOST_VSOCK_MAX_VQS];
int event_vq_id;
}
void vhost_vsock_common_realize(...)
{
int nvqs = VHOST_VSOCK_NVQS;
...
if (enable_dgram) {
nvqs = VHOST_VSOCK_NVQS_DGRAM;
}
...
for (i = 0; i < nvqs; i++) {
vvc->vqs[i] = virtio_add_queue(vdev, VHOST_VSOCK_QUEUE_SIZE,
vhost_vsock_common_handle_output);
}
/* event queue is not handled by the vhost device */
vvc->vhost_dev.nvqs = nvqs - 1;
vvc->vhost_dev.vqs = g_new0(struct vhost_virtqueue, vvc->vhost_dev.nvqs);
...
}
int vhost_vsock_common_start(...)
{
...
if (virtio_has_feature(vdev->guest_features, VIRTIO_VSOCK_F_DGRAM)) {
vvc->event_vq_id = VHOST_VSOCK_NVQS_DGRAM - 1;
} else {
vvc->event_vq_id = VHOST_VSOCK_NVQS - 1;
}
...
}
Then use `vvc->event_vq_id` in :
- vhost_vsock_common_send_transport_reset()
- vhost_vsock_common_post_load() (instead of 2 wired in the code)
Maybe in vhost_vsock_common_send_transport_reset() we can skip the
message enqueueing if the device is not started
(vvc->vhost_dev.started).
Thanks,
Stefano
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-09-23 9:34 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-09-22 0:00 [RFC v7] virtio/vsock: add two more queues for datagram types Jiang Wang
2021-09-22 9:23 ` Stefano Garzarella
-- strict thread matches above, loose matches on Subject: below --
2021-09-22 17:36 Jiang Wang .
2021-09-23 9:18 ` Stefano Garzarella
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).