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

[Gautam Dawar]

-----Original Message-----
From: Jason Wang <jasowang@redhat.com> 
Sent: Thursday, February 17, 2022 12:46 PM
To: Gautam Dawar <gdawar@xilinx.com>
Cc: mst <mst@redhat.com>; qemu-devel <qemu-devel@nongnu.org>; eperezma 
<eperezma@redhat.com>; Martin Petrus Hubertus Habets <martinh@xilinx.com>; 
Harpreet Singh Anand <hanand@xilinx.com>; Gautam Dawar <gdawar@xilinx.com>; 
Tanuj Murlidhar Kamde <tanujk@xilinx.com>; Pablo Cascon <pabloc@xilinx.com>
Subject: Re: [PATCH 2/2] Allow VIRTIO_F_IN_ORDER to be negotiated for vdpa 
devices

On Tue, Feb 15, 2022 at 3:23 PM Gautam Dawar <gautam.dawar@xilinx.com> wrote:
>
> 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 aid in reaping performance benefits with 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.
>
> Signed-off-by: Gautam Dawar <gdawar@xilinx.com>
> ---
>  hw/net/virtio-net.c | 10 ++++++++++
>  net/vhost-vdpa.c    |  1 +
>  2 files changed, 11 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;

This looks like a hack, considering we will finally support in_order.
[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, unless we know that this work is already in progress & will be up-streamed 
soon and if you don’t see any side-effects of this patch, we can get it 
integrated first and then this can be reverted when the complete solution is 
available. This would help in benchmarking performance boosts achieved with 
IN_ORDER feature.

I wonder if it's better to

1) introduce command line parameters "in_order"
2) fail without vhost-vdpa

?
[GD>>] I think this patch is taking care of both conditions above with the  if 
(nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) check. With this, the 
VIRTIO_F_IN_ORDER feature bit will be exposed by QEMU only when the underlying 
device supports it. However, it will be negotiated and acked only when the 
virtio_net driver also supports. Accordingly, in my testing, Linux kernel's 
virtio_net driver doesn't send it back to the supporting device while DPDK 
virtio_net driver does.

Thanks

>      }
> +
>      QTAILQ_INIT(&n->rsc_chains);
>      n->qdev = dev;
>
> 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
>