On Fri, Jan 19, 2024 at 7:42 AM Wentao Jia <wentao....@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...@corigine.com>
> Signed-off-by: Wentao Jia <wentao....@corigine.com>
> Reviewed-by: Xinying Yu <xinying...@corigine.com>
> Reviewed-by: Shujing Dong <shujing.d...@corigine.com>
> Reviewed-by: Rick Zhong <zhaoyong.zh...@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#how-backwards-compatibility-works
[2] https://lists.gnu.org/archive/html/qemu-devel/2022-08/msg02772.html
> DEFINE_PROP_END_OF_LIST(),
> };
>
> --
>
