Re: FW: [PATCH] vhost-user: add VIRTIO_F_IN_ORDER and VIRTIO_F_NOTIFICATION_DATA feature

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Feb 01, 2024 at 10:47:56AM +0000, Wentao Jia wrote:
> Hi, Eugenio
> 
> Thanks for you comments
> 
> It is a dilemma, our team mainly work on smartNIC vDPA, features
> implementation in the QEMU emulated devices is a certain workload for
> us.

In this case the implementation is mostly trivial I have no idea
why argue about it - just do it.

Implementing it in software will make it possible to e.g. test
the driver without the device. Which is of value even to yourself.
IOW - tough, you want a feature in please make it consumable
by implementing a software fallback.





> I have a proposal, clear these features except vhost, it will not affect emulated devices, do you agree the change?
> 
> partial codes for clear these features
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 7a2846fa1c..f4cf8b74da 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -822,6 +822,8 @@ static uint64_t virtio_net_get_features(VirtIODevice *vdev, uint64_t features,
>      }
> 
>      if (!get_vhost_net(nc->peer)) {
> +        virtio_clear_feature(&features, VIRTIO_F_IN_ORDER);
> +        virtio_clear_feature(&features, VIRTIO_F_NOTIFICATION_DATA);
>          return features;
>      }
> 
> Best Regards
> Wentao Jia
> 
> -----Original Message-----
> From: Eugenio Perez Martin <eperezma@redhat.com> 
> Sent: Saturday, January 27, 2024 2:04 AM
> To: Wentao Jia <wentao.jia@nephogine.com>
> Cc: Rick Zhong <zhaoyong.zhong@nephogine.com>; qemu-devel@nongnu.org; mst@redhat.com; Jason Wang <jasowang@redhat.com>; Peter Xu <peterx@redhat.com>; Guo Zhi <qtxuning1999@sjtu.edu.cn>; Xinying Yu <xinying.yu@nephogine.com>
> Subject: Re: FW: [PATCH] vhost-user: add VIRTIO_F_IN_ORDER and VIRTIO_F_NOTIFICATION_DATA feature
> 
> On Fri, Jan 26, 2024 at 9:59 AM Wentao Jia <wentao.jia@nephogine.com> wrote:
> >
> > Hi, Eugenio
> >
> > Thanks for you comments, Our team has made new change about the patch, 
> > these features in hw/virtio/virtio.h:DEFINE_VIRTIO_COMMON_FEATURES,
> > they are turned off by default , and can be turned on from at qemu 
> > command line Do you have comments about this patch?
> >
> 
> If the commandline is set to =on on an emulated device, we're back at square one: The guest will try to use these features in the emulator device and the kick or the descriptors exchange will fail.
> 
> Maybe we can propose their implementation in the emulated devices on Google Summer of Code? Would you be interested in mentoring this? I can help with it for sure.
> 
> On the other hand I'm not sure about the benefits of notification_data for emulated devices or even vhost-kernel. My understanding is that the data written cannot be passed with the eventfd, so QEMU should fully vmexit to the iowrite (which probably is slower in the event of a lot of notifications). Unless we can transmit the avail idx, the device must read the avail ring anyway.
> 
> So the question for MST / Jason is, Is this enough justification to maybe fail the initialization of virtio-net-pci devices with backends different than vhost-user of vdpa if notification_data=on? Should this be backed by profiled data?
> 
> In my opinion the emulated device should implement it and be =off by default, just for testing the driver implementation. But maybe it can be done on top after the early failure?
> 
> Thanks!
> 
> > Best Regards
> > Wentao Jia
> >
> >
> > VIRTIO_F_IN_ORDER and VIRTIO_F_NOTIFICATION_DATA feature are important 
> > feature for dpdk vdpa packets transmitting performance, add these 
> > features at vhost-user front-end to negotiation with backend.
> >
> > In this patch, these features are turned off by default, turn on the 
> > features at qemu command line.
> > ... notification_data=on,in_order=on ...
> >
> > Signed-off-by: Wentao Jia <wentao.jia@corigine.com>
> > Signed-off-by: Xinying Yu <xinying.yu@corigine.com>
> > Signed-off-by: Kyle Xu <zhenbing.xu@corigine.com>
> > Reviewed-by:   Shujing Dong <shujing.dong@corigine.com>
> > Reviewed-by:   Rick Zhong <zhaoyong.zhong@corigine.com>
> > ---
> >  hw/core/machine.c          | 2 ++
> >  hw/net/vhost_net.c         | 2 ++
> >  include/hw/virtio/virtio.h | 6 +++++-
> >  3 files changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/core/machine.c b/hw/core/machine.c index 
> > fb5afdcae4..40489c23a6 100644
> > --- a/hw/core/machine.c
> > +++ b/hw/core/machine.c
> > @@ -40,6 +40,7 @@ GlobalProperty hw_compat_8_1[] = {
> >      { "ramfb", "x-migrate", "off" },
> >      { "vfio-pci-nohotplug", "x-ramfb-migrate", "off" },
> >      { "igb", "x-pcie-flr-init", "off" },
> > +    { "virtio-device", "notification_data", "off"},
> >  };
> >  const size_t hw_compat_8_1_len = G_N_ELEMENTS(hw_compat_8_1);
> >
> > @@ -65,6 +66,7 @@ GlobalProperty hw_compat_7_1[] = {
> >      { "virtio-rng-pci", "vectors", "0" },
> >      { "virtio-rng-pci-transitional", "vectors", "0" },
> >      { "virtio-rng-pci-non-transitional", "vectors", "0" },
> > +    { "virtio-device", "in_order", "off"},
> >  };
> >  const size_t hw_compat_7_1_len = G_N_ELEMENTS(hw_compat_7_1);
> >
> > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c index 
> > e8e1661646..211ca859a6 100644
> > --- a/hw/net/vhost_net.c
> > +++ b/hw/net/vhost_net.c
> > @@ -76,6 +76,8 @@ static const int user_feature_bits[] = {
> >      VIRTIO_F_IOMMU_PLATFORM,
> >      VIRTIO_F_RING_PACKED,
> >      VIRTIO_F_RING_RESET,
> > +    VIRTIO_F_IN_ORDER,
> > +    VIRTIO_F_NOTIFICATION_DATA,
> >      VIRTIO_NET_F_RSS,
> >      VIRTIO_NET_F_HASH_REPORT,
> >      VIRTIO_NET_F_GUEST_USO4,
> > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h 
> > index c8f72850bc..e6aa10f01b 100644
> > --- a/include/hw/virtio/virtio.h
> > +++ b/include/hw/virtio/virtio.h
> > @@ -368,7 +368,11 @@ typedef struct VirtIORNGConf VirtIORNGConf;
> >      DEFINE_PROP_BIT64("packed", _state, _field, \
> >                        VIRTIO_F_RING_PACKED, false), \
> >      DEFINE_PROP_BIT64("queue_reset", _state, _field, \
> > -                      VIRTIO_F_RING_RESET, true)
> > +                      VIRTIO_F_RING_RESET, true), \
> > +    DEFINE_PROP_BIT64("in_order", _state, _field, \
> > +                      VIRTIO_F_IN_ORDER, false), \
> > +    DEFINE_PROP_BIT64("notification_data", _state, _field, \
> > +                      VIRTIO_F_NOTIFICATION_DATA, false)
> >
> >  hwaddr virtio_queue_get_desc_addr(VirtIODevice *vdev, int n);  bool 
> > virtio_queue_enabled_legacy(VirtIODevice *vdev, int n);
> > --
> > 2.31.1
> >
> > -----Original Message-----
> > From: Rick Zhong <zhaoyong.zhong@nephogine.com>
> > Sent: Friday, January 19, 2024 6:39 PM
> > To: Eugenio Perez Martin <eperezma@redhat.com>; Wentao Jia 
> > <wentao.jia@nephogine.com>
> > Cc: qemu-devel@nongnu.org; mst@redhat.com; Jason Wang 
> > <jasowang@redhat.com>; Peter Xu <peterx@redhat.com>; Guo Zhi 
> > <qtxuning1999@sjtu.edu.cn>
> > Subject: 回复: FW: [PATCH] vhost-user: add VIRTIO_F_IN_ORDER and 
> > VIRTIO_F_NOTIFICATION_DATA feature
> >
> > Hi Eugenio,
> >
> > Thanks for your comments. Very helpful. Wentao and I will discuss and get back to you later.
> >
> > Also welcome for any comments from other guys.
> >
> > Best Regards,
> > Rick Zhong
> >
> > -----邮件原件-----
> > 发件人: Eugenio Perez Martin <eperezma@redhat.com>
> > 发送时间: 2024年1月19日 18:26
> > 收件人: Wentao Jia <wentao.jia@nephogine.com>
> > 抄送: qemu-devel@nongnu.org; mst@redhat.com; Rick Zhong 
> > <zhaoyong.zhong@nephogine.com>; Jason Wang <jasowang@redhat.com>; 
> > Peter Xu <peterx@redhat.com>; Guo Zhi <qtxuning1999@sjtu.edu.cn>
> > 主题: Re: FW: [PATCH] vhost-user: add VIRTIO_F_IN_ORDER and 
> > VIRTIO_F_NOTIFICATION_DATA feature
> >
> > On Fri, Jan 19, 2024 at 7:42 AM Wentao Jia <wentao.jia@nephogine.com> wrote:
> > >
> > >
> > > VIRTIO_F_IN_ORDER and VIRTIO_F_NOTIFICATION_DATA feature are 
> > > important feature for dpdk vdpa packets transmitting performance, 
> > > add the 2 features at vhost-user front-end to negotiation with backend.
> > >
> > > Signed-off-by: Kyle Xu <zhenbing.xu@corigine.com>
> > > Signed-off-by: Wentao Jia <wentao.jia@corigine.com>
> > > Reviewed-by:   Xinying Yu <xinying.yu@corigine.com>
> > > Reviewed-by:   Shujing Dong <shujing.dong@corigine.com>
> > > Reviewed-by:   Rick Zhong <zhaoyong.zhong@corigine.com>
> > > ---
> > >  hw/core/machine.c   | 2 ++
> > >  hw/net/vhost_net.c  | 2 ++
> > >  hw/net/virtio-net.c | 4 ++++
> > >  3 files changed, 8 insertions(+)
> > >
> > > diff --git a/hw/core/machine.c b/hw/core/machine.c index
> > > fb5afdcae4..e620f5e7d0 100644
> > > --- a/hw/core/machine.c
> > > +++ b/hw/core/machine.c
> > > @@ -40,6 +40,7 @@ GlobalProperty hw_compat_8_1[] = {
> > >      { "ramfb", "x-migrate", "off" },
> > >      { "vfio-pci-nohotplug", "x-ramfb-migrate", "off" },
> > >      { "igb", "x-pcie-flr-init", "off" },
> > > +    { TYPE_VIRTIO_NET, "notification_data", "off"},
> > >  };
> >
> > Assuming the default "true" in
> > hw/net/virtio-net.c:virtio_net_properties is valid, this needs to be appended to the array of the QEMU version that introduced the property in the virtio_net_properties array, not the one that imported the macro from the kernel. This allows QEMU to know that old versions have these features disabled although the default set in hw/net/virtio-net.c:virtio_net_properties is true when migrating from / to these versions.
> >
> > You can check that this is added properly by migrating from / to a previous version of QEMU, with the combinations of true and false.
> >
> > You have an example in [1] with blk devices multiqueue. CCing Peter Xu as he knows more than me about this.
> >
> > This is very easy to miss when adding new features. Somebody who knows perl should add a test in checkpath.pl similar to the warning "added, moved or deleted file(s), does MAINTAINERS need updating?" when virtio properties are modified :).
> >
> > >  const size_t hw_compat_8_1_len = G_N_ELEMENTS(hw_compat_8_1);
> > >
> > > @@ -65,6 +66,7 @@ GlobalProperty hw_compat_7_1[] = {
> > >      { "virtio-rng-pci", "vectors", "0" },
> > >      { "virtio-rng-pci-transitional", "vectors", "0" },
> > >      { "virtio-rng-pci-non-transitional", "vectors", "0" },
> > > +    { TYPE_VIRTIO_NET, "in_order", "off"},
> > >  };
> > >  const size_t hw_compat_7_1_len = G_N_ELEMENTS(hw_compat_7_1);
> > >
> > > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c index
> > > e8e1661646..211ca859a6 100644
> > > --- a/hw/net/vhost_net.c
> > > +++ b/hw/net/vhost_net.c
> > > @@ -76,6 +76,8 @@ static const int user_feature_bits[] = {
> > >      VIRTIO_F_IOMMU_PLATFORM,
> > >      VIRTIO_F_RING_PACKED,
> > >      VIRTIO_F_RING_RESET,
> > > +    VIRTIO_F_IN_ORDER,
> > > +    VIRTIO_F_NOTIFICATION_DATA,
> > >      VIRTIO_NET_F_RSS,
> > >      VIRTIO_NET_F_HASH_REPORT,
> > >      VIRTIO_NET_F_GUEST_USO4,
> > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c index
> > > 7a2846fa1c..dc0a028934 100644
> > > --- a/hw/net/virtio-net.c
> > > +++ b/hw/net/virtio-net.c
> > > @@ -3949,6 +3949,10 @@ static Property virtio_net_properties[] = {
> > >                        VIRTIO_NET_F_GUEST_USO6, true),
> > >      DEFINE_PROP_BIT64("host_uso", VirtIONet, host_features,
> > >                        VIRTIO_NET_F_HOST_USO, true),
> > > +    DEFINE_PROP_BIT64("in_order", VirtIONet, host_features,
> > > +                      VIRTIO_F_IN_ORDER, true),
> > > +    DEFINE_PROP_BIT64("notification_data", VirtIONet, host_features,
> > > +                      VIRTIO_F_NOTIFICATION_DATA, true),
> >
> > This default=true is wrong, and makes emulated devices show these features as available when they're not. You can test it by running qemu with the parameters:
> >
> > -netdev tap,id=hostnet0,vhost=off -device virtio-net-pci,netdev=hostnet0,...
> >
> > The emulated device must support both features before making them tunnables.
> >
> > On the other hand, all kinds of virtio devices can use in_order and notification_data, so they should be in include/hw/virtio/virtio.h:DEFINE_VIRTIO_COMMON_FEATURES. But not all of them benefit from in_order. One example of this is virtio-blk. It is usual that requests are completed out of order by the backend device, so my impression is that in_order will hurt its performance.
> > I've never profiled it though, so I may be wrong :).
> >
> > Long story short: Maybe in_order should be false by default, and enabled just in virtio-net?
> >
> > You can see previous attempts of implementing this feature in qemu in [2]. CCing Guo too, as I don't know if he plans to continue this work soon.
> >
> > Please let me know if you need any help with these!
> >
> > Thanks!
> >
> > [1] 
> > https://www.qemu.org/docs/master/devel/migration/compatibility.html#ho
> > w-backwards-compatibility-works [2] 
> > https://lists.gnu.org/archive/html/qemu-devel/2022-08/msg02772.html
> >
> > >      DEFINE_PROP_END_OF_LIST(),
> > >  };
> > >
> > > --
> > >
> >
>