On 2024/04/08 6:46, Yuri Benditovich wrote:
On Wed, Apr 3, 2024 at 2:11 PM Akihiko Odaki <akihiko.od...@daynix.com> wrote:vhost requires eBPF for RSS. When eBPF is not available, virtio-net implicitly disables RSS even if the user explicitly requests it. Return an error instead of implicitly disabling RSS if RSS is requested but not available. Signed-off-by: Akihiko Odaki <akihiko.od...@daynix.com> --- hw/net/virtio-net.c | 97 ++++++++++++++++++++++++++--------------------------- 1 file changed, 48 insertions(+), 49 deletions(-) diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c index 61b49e335dea..3d53eba88cfc 100644 --- a/hw/net/virtio-net.c +++ b/hw/net/virtio-net.c @@ -793,9 +793,6 @@ static uint64_t virtio_net_get_features(VirtIODevice *vdev, uint64_t features, return features; } - if (!ebpf_rss_is_loaded(&n->ebpf_rss)) { - virtio_clear_feature(&features, VIRTIO_NET_F_RSS); - } features = vhost_net_get_features(get_vhost_net(nc->peer), features); vdev->backend_features = features; @@ -3591,6 +3588,50 @@ static bool failover_hide_primary_device(DeviceListener *listener, return qatomic_read(&n->failover_primary_hidden); } +static void virtio_net_device_unrealize(DeviceState *dev) +{ + VirtIODevice *vdev = VIRTIO_DEVICE(dev); + VirtIONet *n = VIRTIO_NET(dev); + int i, max_queue_pairs; + + if (virtio_has_feature(n->host_features, VIRTIO_NET_F_RSS)) { + virtio_net_unload_ebpf(n); + } + + /* This will stop vhost backend if appropriate. */ + virtio_net_set_status(vdev, 0); + + g_free(n->netclient_name); + n->netclient_name = NULL; + g_free(n->netclient_type); + n->netclient_type = NULL; + + g_free(n->mac_table.macs); + g_free(n->vlans); + + if (n->failover) { + qobject_unref(n->primary_opts); + device_listener_unregister(&n->primary_listener); + migration_remove_notifier(&n->migration_state); + } else { + assert(n->primary_opts == NULL); + } + + max_queue_pairs = n->multiqueue ? n->max_queue_pairs : 1; + for (i = 0; i < max_queue_pairs; i++) { + virtio_net_del_queue(n, i); + } + /* delete also control vq */ + virtio_del_queue(vdev, max_queue_pairs * 2); + qemu_announce_timer_del(&n->announce_timer, false); + g_free(n->vqs); + qemu_del_nic(n->nic); + virtio_net_rsc_cleanup(n); + g_free(n->rss_data.indirections_table); + net_rx_pkt_uninit(n->rx_pkt); + virtio_cleanup(vdev); +} + static void virtio_net_device_realize(DeviceState *dev, Error **errp) { VirtIODevice *vdev = VIRTIO_DEVICE(dev); @@ -3760,53 +3801,11 @@ static void virtio_net_device_realize(DeviceState *dev, Error **errp) net_rx_pkt_init(&n->rx_pkt); - if (virtio_has_feature(n->host_features, VIRTIO_NET_F_RSS)) { - virtio_net_load_ebpf(n); - } -} - -static void virtio_net_device_unrealize(DeviceState *dev) -{ - VirtIODevice *vdev = VIRTIO_DEVICE(dev); - VirtIONet *n = VIRTIO_NET(dev); - int i, max_queue_pairs; - - if (virtio_has_feature(n->host_features, VIRTIO_NET_F_RSS)) { - virtio_net_unload_ebpf(n); + if (virtio_has_feature(n->host_features, VIRTIO_NET_F_RSS) &&I disagree with this change of qemu behavior. From my point of view: - this is not a major problem and it should not be a reason to stop VM execution - it is enough to disable the RSS feature and continue working. Depending on other qemu parameters (number of queues, number of cpus) this might be just suboptimal. might be a minor problem and might be not a problem at all
The reasoning is that we shouldn't disable what the user explicitly requested. c.f., https://lore.kernel.org/all/20231102091717-mutt-send-email-...@kernel.org/
- this change defines rss as _only_ feature whose absence breaks the VM start, _all_ other features are dropped silently and only rss is not. Why??
I'm following what QEMU does in the other places rather than what it does just in virtio-net. I have pointed out virtio-gpu raises errors in such a situation. c.f., https://lore.kernel.org/all/8880b6f9-f556-46f7-a191-eeec0fe20...@daynix.com
- the series has a title 'Fixes and improvements' . This is not a fix and not an improvement, this is significant behavioral change that should be discussed in light of future plans regarding rss - I suggest to remove this change from the series, submit it separately and discuss from all the sides
We should have already discussed about these matters; I responded all past replies in the previous versions months ago and had no update after that. Let's focus on matters that were not previously pointed out.
Regards, Akihiko Odaki
