* [RFC v6] virtio/vsock: add two more queues for datagram types
@ 2021-09-13 22:18 Jiang Wang
2021-09-14 12:46 ` Stefan Hajnoczi
0 siblings, 1 reply; 6+ messages in thread
From: Jiang Wang @ 2021-09-13 22:18 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.
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.
hw/virtio/vhost-user-vsock.c | 2 +-
hw/virtio/vhost-vsock-common.c | 25 ++++++++++++--
hw/virtio/vhost-vsock.c | 34 ++++++++++++++++++-
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, 64 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..d94636e04e 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,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 = VHOST_VSOCK_NVQS;
virtio_init(vdev, name, VIRTIO_ID_VSOCK,
sizeof(struct virtio_vsock_config));
@@ -209,12 +213,20 @@ 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);
+ if (enable_dgram) {
+ nvqs = VHOST_VSOCK_NVQS_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);
- 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 +239,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 == VHOST_VSOCK_NVQS_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..891d38e226 100644
--- a/hw/virtio/vhost-vsock.c
+++ b/hw/virtio/vhost-vsock.c
@@ -20,9 +20,12 @@
#include "hw/qdev-properties.h"
#include "hw/virtio/vhost-vsock.h"
#include "monitor/monitor.h"
+#include <sys/ioctl.h>
+#include <linux/vhost.h>
const int feature_bits[] = {
VIRTIO_VSOCK_F_SEQPACKET,
+ VIRTIO_VSOCK_F_DGRAM,
VHOST_INVALID_FEATURE_BIT
};
@@ -116,6 +119,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 == VHOST_VSOCK_NVQS_DGRAM) {
+ virtio_add_feature(&requested_features, VIRTIO_VSOCK_F_DGRAM);
+ }
return vhost_get_features(&vvc->vhost_dev, feature_bits,
requested_features);
}
@@ -132,13 +138,34 @@ static const VMStateDescription vmstate_virtio_vhost_vsock = {
.post_load = vhost_vsock_common_post_load,
};
+static bool vhost_vsock_dgram_supported(int vhostfd, Error **errp)
+{
+ uint64_t features;
+ int ret;
+
+ ret = ioctl(vhostfd, VHOST_GET_FEATURES, &features);
+ if (ret) {
+ error_setg(errp, "vhost-vsock: failed to read device freatures. %s",
+ strerror(errno));
+ return false;
+ }
+
+ if (features & (1 << VIRTIO_VSOCK_F_DGRAM)) {
+ return true;
+ }
+
+ return false;
+}
+
static void vhost_vsock_device_realize(DeviceState *dev, Error **errp)
{
+ ERRP_GUARD();
VHostVSockCommon *vvc = VHOST_VSOCK_COMMON(dev);
VirtIODevice *vdev = VIRTIO_DEVICE(dev);
VHostVSock *vsock = VHOST_VSOCK(dev);
int vhostfd;
int ret;
+ bool enable_dgram;
/* Refuse to use reserved CID numbers */
if (vsock->conf.guest_cid <= 2) {
@@ -175,7 +202,11 @@ static void vhost_vsock_device_realize(DeviceState *dev, Error **errp)
qemu_set_nonblock(vhostfd);
}
- vhost_vsock_common_realize(vdev, "vhost-vsock");
+ enable_dgram = vhost_vsock_dgram_supported(vhostfd, errp);
+ if (*errp) {
+ goto err_dgram;
+ }
+ vhost_vsock_common_realize(vdev, "vhost-vsock", enable_dgram);
ret = vhost_dev_init(&vvc->vhost_dev, (void *)(uintptr_t)vhostfd,
VHOST_BACKEND_TYPE_KERNEL, 0, errp);
@@ -197,6 +228,7 @@ err_vhost_dev:
vhostfd = -1;
err_virtio:
vhost_vsock_common_unrealize(vdev);
+err_dgram:
if (vhostfd >= 0) {
close(vhostfd);
}
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..87b2019b5a 100644
--- a/include/hw/virtio/vhost-vsock.h
+++ b/include/hw/virtio/vhost-vsock.h
@@ -33,4 +33,7 @@ struct VHostVSock {
/*< public >*/
};
+#define VHOST_VSOCK_NVQS 2
+#define VHOST_VSOCK_NVQS_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] 6+ messages in thread* Re: [RFC v6] virtio/vsock: add two more queues for datagram types
2021-09-13 22:18 [RFC v6] virtio/vsock: add two more queues for datagram types Jiang Wang
@ 2021-09-14 12:46 ` Stefan Hajnoczi
2021-09-16 3:59 ` Jiang Wang .
0 siblings, 1 reply; 6+ messages in thread
From: Stefan Hajnoczi @ 2021-09-14 12:46 UTC (permalink / raw)
To: Jiang Wang; +Cc: arseny.krasnov, jasowang, sgarzare, qemu-devel, mst
[-- Attachment #1: Type: text/plain, Size: 10341 bytes --]
On Mon, Sep 13, 2021 at 10:18:43PM +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.
> 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.
>
> hw/virtio/vhost-user-vsock.c | 2 +-
> hw/virtio/vhost-vsock-common.c | 25 ++++++++++++--
> hw/virtio/vhost-vsock.c | 34 ++++++++++++++++++-
> 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, 64 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);
VIRTIO_VSOCK_F_DGRAM support should work equally well for
vhost-user-vsock. I don't think there is a need to disable it here.
>
> 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..d94636e04e 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,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 = VHOST_VSOCK_NVQS;
>
> virtio_init(vdev, name, VIRTIO_ID_VSOCK,
> sizeof(struct virtio_vsock_config));
> @@ -209,12 +213,20 @@ 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);
>
> + if (enable_dgram) {
> + nvqs = VHOST_VSOCK_NVQS_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 think the virtqueues can be added unconditionally.
> +
> /* 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);
IIUC the number of virtqueues needs to be the maximum supported number.
It's okay to have more virtqueues than needed. The feature bit is used
by the driver to detect the device's support for dgrams, not the number
of virtqueues.
>
> vvc->post_load_timer = NULL;
> }
> @@ -227,6 +239,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 == VHOST_VSOCK_NVQS_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..891d38e226 100644
> --- a/hw/virtio/vhost-vsock.c
> +++ b/hw/virtio/vhost-vsock.c
> @@ -20,9 +20,12 @@
> #include "hw/qdev-properties.h"
> #include "hw/virtio/vhost-vsock.h"
> #include "monitor/monitor.h"
> +#include <sys/ioctl.h>
> +#include <linux/vhost.h>
>
> const int feature_bits[] = {
> VIRTIO_VSOCK_F_SEQPACKET,
> + VIRTIO_VSOCK_F_DGRAM,
Stefano is currently working on a way to control live migration
compatibility when features like seqpacket or dgram aren't available. He
can suggest how to do this for dgram.
> VHOST_INVALID_FEATURE_BIT
> };
>
> @@ -116,6 +119,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 == VHOST_VSOCK_NVQS_DGRAM) {
> + virtio_add_feature(&requested_features, VIRTIO_VSOCK_F_DGRAM);
> + }
> return vhost_get_features(&vvc->vhost_dev, feature_bits,
> requested_features);
> }
> @@ -132,13 +138,34 @@ static const VMStateDescription vmstate_virtio_vhost_vsock = {
> .post_load = vhost_vsock_common_post_load,
> };
>
> +static bool vhost_vsock_dgram_supported(int vhostfd, Error **errp)
> +{
> + uint64_t features;
> + int ret;
> +
> + ret = ioctl(vhostfd, VHOST_GET_FEATURES, &features);
> + if (ret) {
> + error_setg(errp, "vhost-vsock: failed to read device freatures. %s",
> + strerror(errno));
> + return false;
> + }
ioctl(2) shouldn't be necessary. Let vhost detect features from the
device backend (vhost kernel or vhost-user) and don't worry about
conditionally adding the exact number of virtqueues.
> +
> + if (features & (1 << VIRTIO_VSOCK_F_DGRAM)) {
> + return true;
> + }
> +
> + return false;
> +}
> +
> static void vhost_vsock_device_realize(DeviceState *dev, Error **errp)
> {
> + ERRP_GUARD();
> VHostVSockCommon *vvc = VHOST_VSOCK_COMMON(dev);
> VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> VHostVSock *vsock = VHOST_VSOCK(dev);
> int vhostfd;
> int ret;
> + bool enable_dgram;
>
> /* Refuse to use reserved CID numbers */
> if (vsock->conf.guest_cid <= 2) {
> @@ -175,7 +202,11 @@ static void vhost_vsock_device_realize(DeviceState *dev, Error **errp)
> qemu_set_nonblock(vhostfd);
> }
>
> - vhost_vsock_common_realize(vdev, "vhost-vsock");
> + enable_dgram = vhost_vsock_dgram_supported(vhostfd, errp);
> + if (*errp) {
> + goto err_dgram;
> + }
> + vhost_vsock_common_realize(vdev, "vhost-vsock", enable_dgram);
>
> ret = vhost_dev_init(&vvc->vhost_dev, (void *)(uintptr_t)vhostfd,
> VHOST_BACKEND_TYPE_KERNEL, 0, errp);
> @@ -197,6 +228,7 @@ err_vhost_dev:
> vhostfd = -1;
> err_virtio:
> vhost_vsock_common_unrealize(vdev);
> +err_dgram:
> if (vhostfd >= 0) {
> close(vhostfd);
> }
> 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..87b2019b5a 100644
> --- a/include/hw/virtio/vhost-vsock.h
> +++ b/include/hw/virtio/vhost-vsock.h
> @@ -33,4 +33,7 @@ struct VHostVSock {
> /*< public >*/
> };
>
> +#define VHOST_VSOCK_NVQS 2
> +#define VHOST_VSOCK_NVQS_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
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: Re: [RFC v6] virtio/vsock: add two more queues for datagram types
2021-09-14 12:46 ` Stefan Hajnoczi
@ 2021-09-16 3:59 ` Jiang Wang .
2021-09-16 6:26 ` Stefano Garzarella
2021-09-16 8:28 ` Stefan Hajnoczi
0 siblings, 2 replies; 6+ messages in thread
From: Jiang Wang . @ 2021-09-16 3:59 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Arseny Krasnov, Jason Wang, Stefano Garzarella, qemu devel list,
Michael S. Tsirkin
On Tue, Sep 14, 2021 at 5:46 AM Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> On Mon, Sep 13, 2021 at 10:18:43PM +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.
> > 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.
> >
> > hw/virtio/vhost-user-vsock.c | 2 +-
> > hw/virtio/vhost-vsock-common.c | 25 ++++++++++++--
> > hw/virtio/vhost-vsock.c | 34 ++++++++++++++++++-
> > 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, 64 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);
>
> VIRTIO_VSOCK_F_DGRAM support should work equally well for
> vhost-user-vsock. I don't think there is a need to disable it here.
>
Stefano mentioned in previous email ( V3 ) that I can disable dgram
for vhost-user-vsock for now. I think we need some extra changes to
fully support vhost-vsock-user, like feature discovery?
> >
> > 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..d94636e04e 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,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 = VHOST_VSOCK_NVQS;
> >
> > virtio_init(vdev, name, VIRTIO_ID_VSOCK,
> > sizeof(struct virtio_vsock_config));
> > @@ -209,12 +213,20 @@ 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);
> >
> > + if (enable_dgram) {
> > + nvqs = VHOST_VSOCK_NVQS_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 think the virtqueues can be added unconditionally.
>
OK.
> > +
> > /* 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);
>
> IIUC the number of virtqueues needs to be the maximum supported number.
> It's okay to have more virtqueues than needed. The feature bit is used
> by the driver to detect the device's support for dgrams, not the number
> of virtqueues.
>
OK. I can just make these changes. But I am not able to test vhost-user-vsock
as of now. I tried to follow the steps on here:
https://patchew.org/QEMU/20200515152110.202917-1-sgarzare@redhat.com/
But the git repo for the vhost-user-vsock is kind of broken. I tried to
fix it but I am new to rust so it will take some time. Is there any updated
or an easier way to test vhost-user-vsock?
> >
> > vvc->post_load_timer = NULL;
> > }
> > @@ -227,6 +239,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 == VHOST_VSOCK_NVQS_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..891d38e226 100644
> > --- a/hw/virtio/vhost-vsock.c
> > +++ b/hw/virtio/vhost-vsock.c
> > @@ -20,9 +20,12 @@
> > #include "hw/qdev-properties.h"
> > #include "hw/virtio/vhost-vsock.h"
> > #include "monitor/monitor.h"
> > +#include <sys/ioctl.h>
> > +#include <linux/vhost.h>
> >
> > const int feature_bits[] = {
> > VIRTIO_VSOCK_F_SEQPACKET,
> > + VIRTIO_VSOCK_F_DGRAM,
>
> Stefano is currently working on a way to control live migration
> compatibility when features like seqpacket or dgram aren't available. He
> can suggest how to do this for dgram.
>
Yes. I watched that email thread. I can make similar changes to
DGRAM. I will do it for the next version.
> > VHOST_INVALID_FEATURE_BIT
> > };
> >
> > @@ -116,6 +119,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 == VHOST_VSOCK_NVQS_DGRAM) {
> > + virtio_add_feature(&requested_features, VIRTIO_VSOCK_F_DGRAM);
> > + }
> > return vhost_get_features(&vvc->vhost_dev, feature_bits,
> > requested_features);
> > }
> > @@ -132,13 +138,34 @@ static const VMStateDescription vmstate_virtio_vhost_vsock = {
> > .post_load = vhost_vsock_common_post_load,
> > };
> >
> > +static bool vhost_vsock_dgram_supported(int vhostfd, Error **errp)
> > +{
> > + uint64_t features;
> > + int ret;
> > +
> > + ret = ioctl(vhostfd, VHOST_GET_FEATURES, &features);
> > + if (ret) {
> > + error_setg(errp, "vhost-vsock: failed to read device freatures. %s",
> > + strerror(errno));
> > + return false;
> > + }
>
> ioctl(2) shouldn't be necessary. Let vhost detect features from the
> device backend (vhost kernel or vhost-user) and don't worry about
I did not get this part. What are the difference between vhost and
device backend? I thought vhost is the device backend? vhost kernel
is one kind of backend and vhost-user is another kind. Could you explain
more? Thanks.
> conditionally adding the exact number of virtqueues.
>
> > +
> > + if (features & (1 << VIRTIO_VSOCK_F_DGRAM)) {
> > + return true;
> > + }
> > +
> > + return false;
> > +}
> > +
> > static void vhost_vsock_device_realize(DeviceState *dev, Error **errp)
> > {
> > + ERRP_GUARD();
> > VHostVSockCommon *vvc = VHOST_VSOCK_COMMON(dev);
> > VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> > VHostVSock *vsock = VHOST_VSOCK(dev);
> > int vhostfd;
> > int ret;
> > + bool enable_dgram;
> >
> > /* Refuse to use reserved CID numbers */
> > if (vsock->conf.guest_cid <= 2) {
> > @@ -175,7 +202,11 @@ static void vhost_vsock_device_realize(DeviceState *dev, Error **errp)
> > qemu_set_nonblock(vhostfd);
> > }
> >
> > - vhost_vsock_common_realize(vdev, "vhost-vsock");
> > + enable_dgram = vhost_vsock_dgram_supported(vhostfd, errp);
> > + if (*errp) {
> > + goto err_dgram;
> > + }
> > + vhost_vsock_common_realize(vdev, "vhost-vsock", enable_dgram);
> >
> > ret = vhost_dev_init(&vvc->vhost_dev, (void *)(uintptr_t)vhostfd,
> > VHOST_BACKEND_TYPE_KERNEL, 0, errp);
> > @@ -197,6 +228,7 @@ err_vhost_dev:
> > vhostfd = -1;
> > err_virtio:
> > vhost_vsock_common_unrealize(vdev);
> > +err_dgram:
> > if (vhostfd >= 0) {
> > close(vhostfd);
> > }
> > 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..87b2019b5a 100644
> > --- a/include/hw/virtio/vhost-vsock.h
> > +++ b/include/hw/virtio/vhost-vsock.h
> > @@ -33,4 +33,7 @@ struct VHostVSock {
> > /*< public >*/
> > };
> >
> > +#define VHOST_VSOCK_NVQS 2
> > +#define VHOST_VSOCK_NVQS_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 [flat|nested] 6+ messages in thread* Re: [RFC v6] virtio/vsock: add two more queues for datagram types
2021-09-16 3:59 ` Jiang Wang .
@ 2021-09-16 6:26 ` Stefano Garzarella
2021-09-16 8:57 ` Stefano Garzarella
2021-09-16 8:28 ` Stefan Hajnoczi
1 sibling, 1 reply; 6+ messages in thread
From: Stefano Garzarella @ 2021-09-16 6:26 UTC (permalink / raw)
To: Jiang Wang .
Cc: Arseny Krasnov, Jason Wang, qemu devel list, Stefan Hajnoczi,
Michael S. Tsirkin
On Wed, Sep 15, 2021 at 08:59:17PM -0700, Jiang Wang . wrote:
>On Tue, Sep 14, 2021 at 5:46 AM Stefan Hajnoczi <stefanha@redhat.com> wrote:
>>
>> On Mon, Sep 13, 2021 at 10:18:43PM +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.
>> > 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.
>> >
>> > hw/virtio/vhost-user-vsock.c | 2 +-
>> > hw/virtio/vhost-vsock-common.c | 25 ++++++++++++--
>> > hw/virtio/vhost-vsock.c | 34 ++++++++++++++++++-
>> > 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, 64 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);
>>
>> VIRTIO_VSOCK_F_DGRAM support should work equally well for
>> vhost-user-vsock. I don't think there is a need to disable it here.
>>
>Stefano mentioned in previous email ( V3 ) that I can disable dgram
>for vhost-user-vsock for now. I think we need some extra changes to
>fully support vhost-vsock-user, like feature discovery?
I think Stefan is suggesting something similar of what we discussed
here:
https://lists.nongnu.org/archive/html/qemu-devel/2021-06/msg06689.html
We can allocate all the queues, and choose at runtime which queue to use
for events, that is the only queue used by QEMU.
We can check that in vhost_vsock_common_start(), just before starting
the device, where we know the features acked by the guest
(vdev->guest_features).
And I agree that would be the best approach, since we don't need
discovery anymore, and the same code works also for vhost-user-vsock as
is.
>
>> >
>> > 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..d94636e04e 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,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 = VHOST_VSOCK_NVQS;
>> >
>> > virtio_init(vdev, name, VIRTIO_ID_VSOCK,
>> > sizeof(struct virtio_vsock_config));
>> > @@ -209,12 +213,20 @@ 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);
>> >
>> > + if (enable_dgram) {
>> > + nvqs = VHOST_VSOCK_NVQS_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 think the virtqueues can be added unconditionally.
>>
>OK.
>> > +
>> > /* 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);
>>
>> IIUC the number of virtqueues needs to be the maximum supported number.
>> It's okay to have more virtqueues than needed. The feature bit is used
>> by the driver to detect the device's support for dgrams, not the number
>> of virtqueues.
>>
>OK. I can just make these changes. But I am not able to test
>vhost-user-vsock
>as of now. I tried to follow the steps on here:
>https://patchew.org/QEMU/20200515152110.202917-1-sgarzare@redhat.com/
>But the git repo for the vhost-user-vsock is kind of broken. I tried to
>fix it but I am new to rust so it will take some time. Is there any updated
>or an easier way to test vhost-user-vsock?
>
Yep, it was a PoC, please use the new vhost-user-vsock that Harsha is
developing here:
https://github.com/rust-vmm/vhost-device/pull/7
Feel free to reach me or Harsha if you have any issue.
>
>> >
>> > vvc->post_load_timer = NULL;
>> > }
>> > @@ -227,6 +239,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 == VHOST_VSOCK_NVQS_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..891d38e226 100644
>> > --- a/hw/virtio/vhost-vsock.c
>> > +++ b/hw/virtio/vhost-vsock.c
>> > @@ -20,9 +20,12 @@
>> > #include "hw/qdev-properties.h"
>> > #include "hw/virtio/vhost-vsock.h"
>> > #include "monitor/monitor.h"
>> > +#include <sys/ioctl.h>
>> > +#include <linux/vhost.h>
>> >
>> > const int feature_bits[] = {
>> > VIRTIO_VSOCK_F_SEQPACKET,
>> > + VIRTIO_VSOCK_F_DGRAM,
>>
>> Stefano is currently working on a way to control live migration
>> compatibility when features like seqpacket or dgram aren't available. He
>> can suggest how to do this for dgram.
>>
>Yes. I watched that email thread. I can make similar changes to
>DGRAM. I will do it for the next version.
I'll send a new patch for seqpacket (using on,off,auto prop) hopefully
tomorrow or early next week, then you can rebase on it. It should be
simple to add the dgram feature too.
I'll keep you in CC.
Thanks,
Stefano
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [RFC v6] virtio/vsock: add two more queues for datagram types
2021-09-16 6:26 ` Stefano Garzarella
@ 2021-09-16 8:57 ` Stefano Garzarella
0 siblings, 0 replies; 6+ messages in thread
From: Stefano Garzarella @ 2021-09-16 8:57 UTC (permalink / raw)
To: Jiang Wang .
Cc: Arseny Krasnov, Jason Wang, qemu devel list, Stefan Hajnoczi,
Michael S. Tsirkin
On Thu, Sep 16, 2021 at 08:26:15AM +0200, Stefano Garzarella wrote:
>On Wed, Sep 15, 2021 at 08:59:17PM -0700, Jiang Wang . wrote:
>>On Tue, Sep 14, 2021 at 5:46 AM Stefan Hajnoczi <stefanha@redhat.com> wrote:
>>>
>>>On Mon, Sep 13, 2021 at 10:18:43PM +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.
>>>> 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.
>>>>
>>>> hw/virtio/vhost-user-vsock.c | 2 +-
>>>> hw/virtio/vhost-vsock-common.c | 25 ++++++++++++--
>>>> hw/virtio/vhost-vsock.c | 34 ++++++++++++++++++-
>>>> 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, 64 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);
>>>
>>>VIRTIO_VSOCK_F_DGRAM support should work equally well for
>>>vhost-user-vsock. I don't think there is a need to disable it here.
>>>
>>Stefano mentioned in previous email ( V3 ) that I can disable dgram
>>for vhost-user-vsock for now. I think we need some extra changes to
>>fully support vhost-vsock-user, like feature discovery?
>
>I think Stefan is suggesting something similar of what we discussed
>here:
>https://lists.nongnu.org/archive/html/qemu-devel/2021-06/msg06689.html
>
>We can allocate all the queues, and choose at runtime which queue to
>use for events, that is the only queue used by QEMU.
>
>We can check that in vhost_vsock_common_start(), just before starting
>the device, where we know the features acked by the guest
>(vdev->guest_features).
>
>And I agree that would be the best approach, since we don't need
>discovery anymore, and the same code works also for vhost-user-vsock
>as is.
>
In second thought, I think we really need to know whether the guest has
acked the feature or not.
Otherwise if QEMU and host kernel support dgram, but guest kernel
doesn't, QEMU will use fifth queue instead of third for events, but
guest doesn't support it.
Thanks,
Stefano
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Re: [RFC v6] virtio/vsock: add two more queues for datagram types
2021-09-16 3:59 ` Jiang Wang .
2021-09-16 6:26 ` Stefano Garzarella
@ 2021-09-16 8:28 ` Stefan Hajnoczi
1 sibling, 0 replies; 6+ messages in thread
From: Stefan Hajnoczi @ 2021-09-16 8:28 UTC (permalink / raw)
To: Jiang Wang .
Cc: Michael S. Tsirkin, Jason Wang, qemu devel list, HarshaNavkis,
Arseny Krasnov, Stefano Garzarella
[-- Attachment #1: Type: text/plain, Size: 3959 bytes --]
On Wed, Sep 15, 2021 at 08:59:17PM -0700, Jiang Wang . wrote:
> On Tue, Sep 14, 2021 at 5:46 AM Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > On Mon, Sep 13, 2021 at 10:18:43PM +0000, Jiang Wang wrote:
> > > 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);
> >
> > VIRTIO_VSOCK_F_DGRAM support should work equally well for
> > vhost-user-vsock. I don't think there is a need to disable it here.
> >
> Stefano mentioned in previous email ( V3 ) that I can disable dgram
> for vhost-user-vsock for now. I think we need some extra changes to
> fully support vhost-vsock-user, like feature discovery?
I'm not sure why enabling it on the QEMU side poses a problem? VIRTIO
feature negotiation will disable it (if the feature bit is included in
user_feature_bits[]) if the vhost-user device backend lacks support.
> > > /* 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);
> >
> > IIUC the number of virtqueues needs to be the maximum supported number.
> > It's okay to have more virtqueues than needed. The feature bit is used
> > by the driver to detect the device's support for dgrams, not the number
> > of virtqueues.
> >
> OK. I can just make these changes. But I am not able to test vhost-user-vsock
> as of now. I tried to follow the steps on here:
> https://patchew.org/QEMU/20200515152110.202917-1-sgarzare@redhat.com/
> But the git repo for the vhost-user-vsock is kind of broken. I tried to
> fix it but I am new to rust so it will take some time. Is there any updated
> or an easier way to test vhost-user-vsock?
Harsha or Stefano can help with this.
> > > @@ -132,13 +138,34 @@ static const VMStateDescription vmstate_virtio_vhost_vsock = {
> > > .post_load = vhost_vsock_common_post_load,
> > > };
> > >
> > > +static bool vhost_vsock_dgram_supported(int vhostfd, Error **errp)
> > > +{
> > > + uint64_t features;
> > > + int ret;
> > > +
> > > + ret = ioctl(vhostfd, VHOST_GET_FEATURES, &features);
> > > + if (ret) {
> > > + error_setg(errp, "vhost-vsock: failed to read device freatures. %s",
> > > + strerror(errno));
> > > + return false;
> > > + }
> >
> > ioctl(2) shouldn't be necessary. Let vhost detect features from the
> > device backend (vhost kernel or vhost-user) and don't worry about
> I did not get this part. What are the difference between vhost and
> device backend? I thought vhost is the device backend? vhost kernel
> is one kind of backend and vhost-user is another kind. Could you explain
> more? Thanks.
>
> > conditionally adding the exact number of virtqueues.
By "Let vhost detect features" I meant let the QEMU "hw/virtio/vhost.h"
API detect the features from the backend (either vhost kernel or
vhost-user). vhost_dev_init() and the other functions already check the
device backend's features, so there should be no need to manually make
an ioctl() call from hw/vhost-vsock.c. The purpose of the QEMU vhost API
is to abstract the backend so device code like hw/vhost-vsock.c doesn't
need to know whether it is communicating with a vhost kernel or
vhost-user device backend.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-09-16 8:59 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-13 22:18 [RFC v6] virtio/vsock: add two more queues for datagram types Jiang Wang
2021-09-14 12:46 ` Stefan Hajnoczi
2021-09-16 3:59 ` Jiang Wang .
2021-09-16 6:26 ` Stefano Garzarella
2021-09-16 8:57 ` Stefano Garzarella
2021-09-16 8:28 ` Stefan Hajnoczi
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.