On Mon, Apr 8, 2024 at 4:31 AM Akihiko Odaki <akihiko.od...@daynix.com> wrote:
>
> 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
>
I think the basic example is when the kernel doesn't support ebpf
loading (either old kernel or kernel that, for some security reason,
disabled the capability). In this case, we will get a behavior change.
Best regards,
Yan.
>
> 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
>
