[RFC PATCH] Allow VIRTIO_F_IN_ORDER to be negotiated for vdpa devices

[RFC PATCH] Allow VIRTIO_F_IN_ORDER to be negotiated for vdpa devices

[gautam.dawar]

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;
     }
+
     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 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,
-- 
2.30.1

Re: [RFC PATCH] Allow VIRTIO_F_IN_ORDER to be negotiated for vdpa devices

[Jason Wang]

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?

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

> +
>  /*
>   * 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.

Thanks

> --
> 2.30.1
>

RE: [RFC PATCH] Allow VIRTIO_F_IN_ORDER to be negotiated for vdpa devices

[Gautam Dawar]

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
>