* Re: [RFC v5] virtio/vsock: add two more queues for datagram types
2021-09-12 18:46 [RFC v5] virtio/vsock: add two more queues for datagram types Jiang Wang
@ 2021-09-13 13:28 ` Stefano Garzarella
0 siblings, 0 replies; 2+ messages in thread
From: Stefano Garzarella @ 2021-09-13 13:28 UTC (permalink / raw)
To: Jiang Wang; +Cc: arseny.krasnov, jasowang, qemu-devel, stefanha, mst
On Sun, Sep 12, 2021 at 06:46:03PM +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
>
> hw/virtio/vhost-user-vsock.c | 2 +-
> hw/virtio/vhost-vsock-common.c | 27 ++++++++++++++++---
> hw/virtio/vhost-vsock.c | 27 ++++++++++++++++++-
> 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, 59 insertions(+), 7 deletions(-)
Please run scripts/checkpatch.pl before send a patch:
$ ./scripts/checkpatch.pl ./v5_20210912_jiang_wang_virtio_vsock_add_two_more_queues_for_datagram_types.mbx
WARNING: line over 80 characters
#73: FILE: hw/virtio/vhost-vsock-common.c:201:
+void vhost_vsock_common_realize(VirtIODevice *vdev, const char *name, bool enable_dgram)
WARNING: line over 80 characters
#91: FILE: hw/virtio/vhost-vsock-common.c:222:
+ vhost_vsock_common_handle_output);
ERROR: braces {} are necessary for all arms of this statement
#114: FILE: hw/virtio/vhost-vsock-common.c:248:
+ if (vvc->vhost_dev.vqs)
[...]
ERROR: g_free(NULL) is safe this check is probably not required
#115: FILE: hw/virtio/vhost-vsock-common.c:249:
+ if (vvc->vhost_dev.vqs)
+ g_free(vvc->vhost_dev.vqs);
ERROR: braces {} are necessary for all arms of this statement
#141: FILE: hw/virtio/vhost-vsock.c:122:
+ if (vvc->vhost_dev.nvqs == MAX_VQS_WITH_DGRAM)
[...]
WARNING: line over 80 characters
#157: FILE: hw/virtio/vhost-vsock.c:147:
+ error_report("vhost-vsock: failed to read device. %s", strerror(errno));
ERROR: braces {} are necessary for all arms of this statement
#162: FILE: hw/virtio/vhost-vsock.c:152:
+ if (features & (1 << VIRTIO_VSOCK_F_DGRAM))
[...]
ERROR: code indent should never use tabs
#214: FILE: include/hw/virtio/vhost-vsock-common.h:46:
+^I^I^I bool enable_dgram);$
total: 5 errors, 3 warnings, 162 lines checked
>
>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..f48b5a69df 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,10 @@ int vhost_vsock_common_post_load(void *opaque, int version_id)
> return 0;
> }
>
>-void vhost_vsock_common_realize(VirtIODevice *vdev, const char *name)
>+void vhost_vsock_common_realize(VirtIODevice *vdev, const char *name, bool enable_dgram)
> {
> VHostVSockCommon *vvc = VHOST_VSOCK_COMMON(vdev);
>+ int nvqs = MAX_VQS_WITHOUT_DGRAM;
>
> virtio_init(vdev, name, VIRTIO_ID_VSOCK,
> sizeof(struct virtio_vsock_config));
>@@ -209,12 +212,22 @@ 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 = MAX_VQS_WITHOUT_DGRAM;
>+ else {
The previous branch is not needed since we initialize nvqs =
MAX_VQS_WITHOUT_DGRAM.
So you can skip it and do:
if (enable_dgram) {
>+ 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);
>
>- 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 +240,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..1fee25f144 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,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);
> }
>@@ -132,6 +137,24 @@ static const VMStateDescription vmstate_virtio_vhost_vsock = {
> .post_load = vhost_vsock_common_post_load,
> };
>
>+static bool vhost_vsock_dgram_supported(int vhostfd)
>+{
>+ uint64_t features;
>+ int ret;
>+
>+ ret = ioctl(vhostfd, VHOST_GET_FEATURES, &features);
>+ if (ret) {
>+ error_report("vhost-vsock: failed to read device. %s", strerror(errno));
>+ qemu_close(vhostfd);
>+ return ret;
We are returning an error code here, that is `true`.
I think is better to pass Error **errp to this function, set it using
error_setg_errno(), and return false in this path since we don't know if
F_DGRAM is set.
Then in vhost_vsock_device_realize() you can check if errp is set and go
to the error path (look at include/qapi/error.h about ERRP_GUARD).
Something like this:
static void vhost_vsock_device_realize(DeviceState *dev, Error **errp)
{
ERRP_GUARD();
...
enable_dgram = vhost_vsock_dgram_supported(vhostfd, errp);
if (*errp) {
goto err_dgram;
}
...
}
And add `err_dgram` label before closing `vhostfd` in the error path.
>+ }
>+
>+ if (features & (1 << VIRTIO_VSOCK_F_DGRAM))
>+ return true;
>+
>+ return false;
>+}
>+
> static void vhost_vsock_device_realize(DeviceState *dev, Error **errp)
> {
> VHostVSockCommon *vvc = VHOST_VSOCK_COMMON(dev);
>@@ -139,6 +162,7 @@ static void vhost_vsock_device_realize(DeviceState
>*dev, Error **errp)
> 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 +199,8 @@ 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);
>+ 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);
>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..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
Maybe I'd rename in:
#define VHOST_VSOCK_NVQS 2
#define VHOST_VSOCK_NVQS_DGRAM 4
Thanks,
Stefano
>+
> #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] 2+ messages in thread