[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
RE: [RFC PATCH] Allow VIRTIO_F_IN_ORDER to be negotiated for vdpa device
From: |
Gautam Dawar |
Subject: |
RE: [RFC PATCH] Allow VIRTIO_F_IN_ORDER to be negotiated for vdpa devices |
Date: |
Mon, 7 Feb 2022 09:43:52 +0000 |
Hi Jason,
Thanks for your feedback. Pls see my comments inline.
Regards,
Gautam
-----Original Message-----
From: Jason Wang <jasowang@redhat.com>
Sent: Monday, February 7, 2022 8:53 AM
To: Gautam Dawar <gdawar@xilinx.com>
Cc: mst <mst@redhat.com>; qemu-devel <qemu-devel@nongnu.org>; eperezma
<eperezma@redhat.com>; Gautam Dawar <gdawar@xilinx.com>; Martin Petrus Hubertus
Habets <martinh@xilinx.com>; Harpreet Singh Anand <hanand@xilinx.com>; Tanuj
Murlidhar Kamde <tanujk@xilinx.com>
Subject: Re: [RFC PATCH] Allow VIRTIO_F_IN_ORDER to be negotiated for vdpa
devices
On Sun, Feb 6, 2022 at 12:04 AM <gautam.dawar@xilinx.com> wrote:
>
> From: Gautam Dawar <gdawar@xilinx.com>
>
> Hi All,
>
> The VIRTIO_F_IN_ORDER feature is implemented by DPDK's virtio_net
> driver but not by the Linux kernel's virtio_net driver.
> However, this feature still can't be tested using vhost-vdpa with
> hardware devices that implement it as VIRTIO_F_IN_ORDER isn't defined
> in kernel's virtio_config.h header file.
> This patch adds the ability to negotiate VIRTIO_F_IN_ORDER bit for
> vhost-vdpa backend when the underlying device supports this feature.
> This would be useful for benchmarking the performance improvements for
> HW devices that implement this feature. At the same time, it shouldn't
> have any negative impact as vhost-vdpa backend doesn't involve any
> userspace virtqueue operations.
> In the final patch, instead of making a direct change in
> virtio_config.h, it will be pushed in the kernel and then QEMU's file
> will be synced with it, as usual.
>
> Signed-off-by: Gautam Dawar <gdawar@xilinx.com>
> ---
> hw/net/virtio-net.c | 10 ++++++++++
> include/standard-headers/linux/virtio_config.h | 6 ++++++
> net/vhost-vdpa.c | 1 +
> 3 files changed, 17 insertions(+)
>
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c index
> cf8ab0f8af..a1089d06f6 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -3507,11 +3507,21 @@ static void virtio_net_device_realize(DeviceState
> *dev, Error **errp)
> nc->rxfilter_notify_enabled = 1;
>
> if (nc->peer && nc->peer->info->type ==
> NET_CLIENT_DRIVER_VHOST_VDPA) {
> + uint64_t features = BIT_ULL(VIRTIO_F_IN_ORDER);
> struct virtio_net_config netcfg = {};
> +
> memcpy(&netcfg.mac, &n->nic_conf.macaddr, ETH_ALEN);
> vhost_net_set_config(get_vhost_net(nc->peer),
> (uint8_t *)&netcfg, 0, ETH_ALEN,
> VHOST_SET_CONFIG_TYPE_MASTER);
> +
> + /*
> + * For vhost-vdpa, if underlying device supports IN_ORDER feature,
> + * make it available for negotiation.
> + */
> + features = vhost_net_get_features(get_vhost_net(nc->peer), features);
> + n->host_features |= features;
I wonder, instead of doing hacks here, it would be better to implement IN_ORDER
in qemu?
[GD>>] Yes, I agree a complete solution that will support the emulated virtio
device with in_order rx/tx virtqueue functions will definitely be better but at
the same time it will take considerable amount of time and effort. I also
noticed that something similar
(https://patchew.org/QEMU/1533833677-27512-1-git-send-email-i.maximets@samsung.com/)
was proposed years ago which got dropped for similar reasons and it has been
status quo since then.
So, if you don’t see any side-effects of this patch, we can get it integrated
first and then someone can work on the complete solution. This would help in
benchmarking performance boosts achieved with IN_ORDER feature.
Note that DPDK has already supported IN_ORDER, so we don't even need to touch
kernel virtio drivers to test it.
> }
> +
> QTAILQ_INIT(&n->rsc_chains);
> n->qdev = dev;
>
> diff --git a/include/standard-headers/linux/virtio_config.h
> b/include/standard-headers/linux/virtio_config.h
> index 22e3a85f67..9ec3a8b54b 100644
> --- a/include/standard-headers/linux/virtio_config.h
> +++ b/include/standard-headers/linux/virtio_config.h
> @@ -80,6 +80,12 @@
> /* This feature indicates support for the packed virtqueue layout. */
> #define VIRTIO_F_RING_PACKED 34
>
> +/*
> + * Inorder feature indicates that all buffers are used by the device
> + * in the same order in which they have been made available.
> + */
> +#define VIRTIO_F_IN_ORDER 35
This need to be done in the following steps:
1) a kernel patch to just add this feature bit
2) sync the kernel header using scripts/update-linux-headers.sh
[GD>>] Yes, I acknowledged that in my patch with the following comment:
> In the final patch, instead of making a direct change in
> virtio_config.h, it will be pushed in the kernel and then QEMU's file
> will be synced with it, as usual.
> +
> /*
> * This feature indicates that memory accesses by the driver and the
> * device are ordered in a way described by the platform.
> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c index
> 25dd6dd975..2886cba5ec 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -62,6 +62,7 @@ const int vdpa_feature_bits[] = {
> VIRTIO_NET_F_CTRL_VQ,
> VIRTIO_F_IOMMU_PLATFORM,
> VIRTIO_F_RING_PACKED,
> + VIRTIO_F_IN_ORDER,
> VIRTIO_NET_F_RSS,
> VIRTIO_NET_F_HASH_REPORT,
> VIRTIO_NET_F_GUEST_ANNOUNCE,
This needs to be done in a separated patch.
[GD>>] Yes, I can do that but it would help my understanding if you can share
reasons for the same.
Thanks
> --
> 2.30.1
>