On Mon, Aug 9, 2021 at 3:58 AM Stefano Garzarella <sgarz...@redhat.com> wrote:
>
> On Thu, Aug 05, 2021 at 12:07:02PM -0700, Jiang Wang . wrote:
> >On Wed, Aug 4, 2021 at 1:13 AM Stefano Garzarella <sgarz...@redhat.com>
> >wrote:
> >>
> >> On Tue, Aug 03, 2021 at 11:41:32PM +0000, Jiang Wang wrote:
> >> >Datagram sockets are connectionless and unreliable.
> >> >The sender does not know the capacity of the receiver
> >> >and may send more packets than the receiver can handle.
> >> >
> >> >Add two more dedicate virtqueues for datagram sockets,
> >> >so that it will not unfairly steal resources from
> >> >stream and future connection-oriented sockets.
> >> >
> >> >Signed-off-by: Jiang Wang <jiang.w...@bytedance.com>
> >> >---
> >> >v1 -> v2: use qemu cmd option to control number of queues,
> >> > removed configuration settings for dgram.
> >> >v2 -> v3: use ioctl to get features and decide number of
> >> > virt queues, instead of qemu cmd option.
> >> >v3 -> v4: change DGRAM feature bit value to 2. Add an argument
> >> > in vhost_vsock_common_realize to indicate dgram is supported or
> >> > not.
> >> >
> >> > hw/virtio/vhost-user-vsock.c | 2 +-
> >> > hw/virtio/vhost-vsock-common.c | 58 ++++++++++++++++++-
> >> > hw/virtio/vhost-vsock.c | 5 +-
> >> > include/hw/virtio/vhost-vsock-common.h | 6 +-
> >> > include/hw/virtio/vhost-vsock.h | 4 ++
> >> > include/standard-headers/linux/virtio_vsock.h | 1 +
> >> > 6 files changed, 69 insertions(+), 7 deletions(-)
> >> >
> >> >diff --git a/hw/virtio/vhost-user-vsock.c b/hw/virtio/vhost-user-vsock.c
> >> >index 6095ed7349..e9ec0e1c00 100644
> >> >--- a/hw/virtio/vhost-user-vsock.c
> >> >+++ b/hw/virtio/vhost-user-vsock.c
> >> >@@ -105,7 +105,7 @@ static void vuv_device_realize(DeviceState *dev,
> >> >Error **errp)
> >> > return;
> >> > }
> >> >
> >> >- vhost_vsock_common_realize(vdev, "vhost-user-vsock");
> >> >+ vhost_vsock_common_realize(vdev, "vhost-user-vsock", false);
> >> >
> >> > vhost_dev_set_config_notifier(&vvc->vhost_dev, &vsock_ops);
> >> >
> >> >diff --git a/hw/virtio/vhost-vsock-common.c
> >> >b/hw/virtio/vhost-vsock-common.c
> >> >index 4ad6e234ad..c78536911a 100644
> >> >--- a/hw/virtio/vhost-vsock-common.c
> >> >+++ b/hw/virtio/vhost-vsock-common.c
> >> >@@ -17,6 +17,8 @@
> >> > #include "hw/virtio/vhost-vsock.h"
> >> > #include "qemu/iov.h"
> >> > #include "monitor/monitor.h"
> >> >+#include <sys/ioctl.h>
> >> >+#include <linux/vhost.h>
> >> >
> >> > int vhost_vsock_common_start(VirtIODevice *vdev)
> >> > {
> >> >@@ -196,9 +198,39 @@ int vhost_vsock_common_post_load(void *opaque, int
> >> >version_id)
> >> > return 0;
> >> > }
> >> >
> >> >-void vhost_vsock_common_realize(VirtIODevice *vdev, const char *name)
> >> >+static int vhost_vsock_get_max_qps(bool enable_dgram)
> >> >+{
> >> >+ uint64_t features;
> >> >+ int ret;
> >> >+ int fd = -1;
> >> >+
> >> >+ if (!enable_dgram)
> >> >+ return MAX_VQS_WITHOUT_DGRAM;
> >> >+
> >> >+ fd = qemu_open_old("/dev/vhost-vsock", O_RDONLY);
> >>
> >>
> >> As I said in the previous version, we cannot directly open
> >> /dev/vhost-vsock, for two reasons:
> >>
> >> 1. this code is common with vhost-user-vsock which does not use
> >> /dev/vhost-vsock.
> >>
> >> 2. the fd may have been passed from the management layer and qemu may
> >> not be able to directly open /dev/vhost-vsock.
> >>
> >> I think is better to move this function in hw/virtio/vhost-vsock.c,
> >> using the `vhostfd`, returning true or false if dgram is supported, then
> >> you can use it for `enable_dgram` param ...
> >>
> >
> >Yes, you are right. Now I remember you said that before but I forgot about
> >that
> >when I changed the code. I will fix it. Sorry about that.
>
> No problem :-)
>
> >
> >> >+ if (fd == -1) {
> >> >+ error_report("vhost-vsock: failed to open device. %s",
> >> >strerror(errno));
> >> >+ return -1;
> >> >+ }
> >> >+
> >> >+ ret = ioctl(fd, VHOST_GET_FEATURES, &features);
> >> >+ if (ret) {
> >> >+ error_report("vhost-vsock: failed to read device. %s",
> >> >strerror(errno));
> >> >+ qemu_close(fd);
> >> >+ return ret;
> >> >+ }
> >> >+
> >> >+ qemu_close(fd);
> >> >+ if (features & (1 << VIRTIO_VSOCK_F_DGRAM))
> >> >+ return MAX_VQS_WITH_DGRAM;
> >> >+
> >> >+ return MAX_VQS_WITHOUT_DGRAM;
> >> >+}
> >> >+
> >> >+void vhost_vsock_common_realize(VirtIODevice *vdev, const char *name,
> >> >bool enable_dgram)
> >> > {
> >> > VHostVSockCommon *vvc = VHOST_VSOCK_COMMON(vdev);
> >> >+ int nvqs = MAX_VQS_WITHOUT_DGRAM;
> >> >
> >> > virtio_init(vdev, name, VIRTIO_ID_VSOCK,
> >> > sizeof(struct virtio_vsock_config));
> >> >@@ -209,12 +241,24 @@ void vhost_vsock_common_realize(VirtIODevice
> >> >*vdev, const char *name)
> >> > vvc->trans_vq = virtio_add_queue(vdev, VHOST_VSOCK_QUEUE_SIZE,
> >> > vhost_vsock_common_handle_output);
> >> >
> >> >+ nvqs = vhost_vsock_get_max_qps(enable_dgram);
> >> >+
> >> >+ if (nvqs < 0)
> >> >+ nvqs = MAX_VQS_WITHOUT_DGRAM;
> >>
> >> ... and here, if `enable_dgram` is true, you can set `nvqs =
> >> MAX_VQS_WITH_DGRAM``
> >>
> >sure.
> >
> >> >+
> >> >+ if (nvqs == MAX_VQS_WITH_DGRAM) {
> >> >+ vvc->dgram_recv_vq = virtio_add_queue(vdev,
> >> >VHOST_VSOCK_QUEUE_SIZE,
> >> >+
> >> >vhost_vsock_common_handle_output);
> >> >+ vvc->dgram_trans_vq = virtio_add_queue(vdev,
> >> >VHOST_VSOCK_QUEUE_SIZE,
> >> >+
> >> >vhost_vsock_common_handle_output);
> >> >+ }
> >> >+
> >>
> >> I'm still concerned about compatibility with guests that don't
> >> support
> >> dgram, as I mentioned in the previous email.
> >>
> >> I need to do some testing, but my guess is it won't work if the host
> >> (QEMU and vhost-vsock) supports it and the guest doesn't.
> >>
> >> I still think that we should allocate an array of queues and then decide
> >> at runtime which one to use for events (third or fifth) after the guest
> >> has acked the features.
> >>
> >Agree. I will check where the guest ack the feature. If you have any
>
> I'm not sure we should delete them, I think we can allocate 5 queues and
> then use queue 3 or 5 for events in vhost_vsock_common_start(), when the
> guest already acked the features.
>
Got it.
> >pointers,
> >just let me know. Also, could we just remove the vq allocation in
> >common_realize
> >and do it at a later time? Or need to delete and add again as I mentioned in
> >the
> >previous email?
>
> Instead of having 'recv_vq', 'trans_vq', 'event_vq' fields, we can have
> an array of VirtQueue pointers and a field that indicates what the index
> of the event queue is. (or a boolean that indicates if we are enabling
> dgram or not).
>
> This should simplify the management.
>
I see. I will dig more. thanks.
> Thanks,
> Stefano
>
