| Message ID | 20231030051356.33123-12-akihiko.odaki@daynix.com |
|---|---|
| State | New |
| Headers | show |
| Series | virtio-net RSS/hash report fixes and improvements | expand |
On Mon, Oct 30, 2023 at 7:14 AM Akihiko Odaki <akihiko.odaki@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. > I think that suggesting RSS feature when in fact it is not available is not a good idea, this rather desinforms the guest. Existing behavior (IMHO) makes more sense. We can extend this discussion if needed, of course. > Signed-off-by: Akihiko Odaki <akihiko.odaki@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 5d4afd12b2..7bb91617d0 100644 > --- a/hw/net/virtio-net.c > +++ b/hw/net/virtio-net.c > @@ -792,9 +792,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; > > @@ -3533,6 +3530,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); > @@ -3704,53 +3745,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) && > + !virtio_net_load_ebpf(n)) { > + error_setg(errp, "Can't load eBPF RSS"); > + virtio_net_device_unrealize(dev); > } > - > - /* 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_reset(VirtIODevice *vdev) > -- > 2.42.0 > >
On 2023/10/30 21:14, Yuri Benditovich wrote: > > > On Mon, Oct 30, 2023 at 7:14 AM Akihiko Odaki <akihiko.odaki@daynix.com > <mailto:akihiko.odaki@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. > > > I think that suggesting RSS feature when in fact it is not available is > not a good idea, this rather desinforms the guest. > Existing behavior (IMHO) makes more sense. > We can extend this discussion if needed, of course. This change is not to advertise RSS when it's not available; it instead reports an error to the user. For example, think of the following command line: qemu-system-x86_64 -device virtio-net,rss=on,netdev=n -netdev user,id=n Before this change, it gives no error and the user will not know RSS is not available. With this change it now gives an error as follows: qemu-system-x86_64: -device virtio-net,rss=on,netdev=n: Can't load eBPF RSS
On Mon, Oct 30, 2023 at 2:21 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote: > On 2023/10/30 21:14, Yuri Benditovich wrote: > > > > > > On Mon, Oct 30, 2023 at 7:14 AM Akihiko Odaki <akihiko.odaki@daynix.com > > <mailto:akihiko.odaki@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. > > > > > > I think that suggesting RSS feature when in fact it is not available is > > not a good idea, this rather desinforms the guest. > > Existing behavior (IMHO) makes more sense. > > We can extend this discussion if needed, of course. > > This change is not to advertise RSS when it's not available; it instead > reports an error to the user. > > For example, think of the following command line: > qemu-system-x86_64 -device virtio-net,rss=on,netdev=n -netdev user,id=n > > Before this change, it gives no error and the user will not know RSS is > not available. With this change it now gives an error as follows: > qemu-system-x86_64: -device virtio-net,rss=on,netdev=n: Can't load eBPF RSS > Does this mean failure to run QEMU if the RSS required in the command line and EBPF can't be loaded? (for example if we run the system with kernel < 5.8)? I'm not sure this is user-friendly behavior...
On 2023/10/30 21:51, Yuri Benditovich wrote: > > > On Mon, Oct 30, 2023 at 2:21 PM Akihiko Odaki <akihiko.odaki@daynix.com > <mailto:akihiko.odaki@daynix.com>> wrote: > > On 2023/10/30 21:14, Yuri Benditovich wrote: > > > > > > On Mon, Oct 30, 2023 at 7:14 AM Akihiko Odaki > <akihiko.odaki@daynix.com <mailto:akihiko.odaki@daynix.com> > > <mailto:akihiko.odaki@daynix.com > <mailto:akihiko.odaki@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. > > > > > > I think that suggesting RSS feature when in fact it is not > available is > > not a good idea, this rather desinforms the guest. > > Existing behavior (IMHO) makes more sense. > > We can extend this discussion if needed, of course. > > This change is not to advertise RSS when it's not available; it instead > reports an error to the user. > > For example, think of the following command line: > qemu-system-x86_64 -device virtio-net,rss=on,netdev=n -netdev user,id=n > > Before this change, it gives no error and the user will not know RSS is > not available. With this change it now gives an error as follows: > qemu-system-x86_64: -device virtio-net,rss=on,netdev=n: Can't load > eBPF RSS > > > Does this mean failure to run QEMU if the RSS required in the command > line and EBPF can't be loaded? > (for example if we run the system with kernel < 5.8)? > I'm not sure this is user-friendly behavior... This patch is wrong that it assumes software RSS is not enabled at all; I missed the vhost check before clearing VIRTIO_NET_F_RSS in virtio_net_get_features(). That said, I indeed intend to make it return a hard error for the case that RSS is requested for vhost but eBPF can't be loaded. I believe the current behavior of implicitly disabling a feature explicitly requested by the user is not good, but we can still emit a warning instead of an error. It's better to follow a convention common in QEMU but I see no documentation regarding this kind of situation. I know virtio-gpu gives an error in such a case but it's just one example. I'd also like to ask Jason if we should have a warning or error. Perhaps we may better consult QOM maintainers too.
On Mon, Oct 30, 2023 at 9:15 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote: > > On 2023/10/30 21:51, Yuri Benditovich wrote: > > > > > > On Mon, Oct 30, 2023 at 2:21 PM Akihiko Odaki <akihiko.odaki@daynix.com > > <mailto:akihiko.odaki@daynix.com>> wrote: > > > > On 2023/10/30 21:14, Yuri Benditovich wrote: > > > > > > > > > On Mon, Oct 30, 2023 at 7:14 AM Akihiko Odaki > > <akihiko.odaki@daynix.com <mailto:akihiko.odaki@daynix.com> > > > <mailto:akihiko.odaki@daynix.com > > <mailto:akihiko.odaki@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. > > > > > > > > > I think that suggesting RSS feature when in fact it is not > > available is > > > not a good idea, this rather desinforms the guest. > > > Existing behavior (IMHO) makes more sense. > > > We can extend this discussion if needed, of course. > > > > This change is not to advertise RSS when it's not available; it instead > > reports an error to the user. > > > > For example, think of the following command line: > > qemu-system-x86_64 -device virtio-net,rss=on,netdev=n -netdev user,id=n > > > > Before this change, it gives no error and the user will not know RSS is > > not available. With this change it now gives an error as follows: > > qemu-system-x86_64: -device virtio-net,rss=on,netdev=n: Can't load > > eBPF RSS > > > > > > Does this mean failure to run QEMU if the RSS required in the command > > line and EBPF can't be loaded? > > (for example if we run the system with kernel < 5.8)? > > I'm not sure this is user-friendly behavior... > > This patch is wrong that it assumes software RSS is not enabled at all; > I missed the vhost check before clearing VIRTIO_NET_F_RSS in > virtio_net_get_features(). > > That said, I indeed intend to make it return a hard error for the case > that RSS is requested for vhost but eBPF can't be loaded. > > I believe the current behavior of implicitly disabling a feature > explicitly requested by the user is not good, Yes, but it has been there for years. It complicates a lot to stick to the migration compatibility. > but we can still emit a warning instead of an error. > > It's better to follow a convention common in QEMU but I see no > documentation regarding this kind of situation. I know virtio-gpu gives > an error in such a case but it's just one example. The problem is that it's too late to fix old Qemu, so we probably can't do more than using compatibility flags... > > I'd also like to ask Jason if we should have a warning or error. Perhaps > we may better consult QOM maintainers too. > Thanks
On 2023/11/01 13:19, Jason Wang wrote: > On Mon, Oct 30, 2023 at 9:15 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote: >> >> On 2023/10/30 21:51, Yuri Benditovich wrote: >>> >>> >>> On Mon, Oct 30, 2023 at 2:21 PM Akihiko Odaki <akihiko.odaki@daynix.com >>> <mailto:akihiko.odaki@daynix.com>> wrote: >>> >>> On 2023/10/30 21:14, Yuri Benditovich wrote: >>> > >>> > >>> > On Mon, Oct 30, 2023 at 7:14 AM Akihiko Odaki >>> <akihiko.odaki@daynix.com <mailto:akihiko.odaki@daynix.com> >>> > <mailto:akihiko.odaki@daynix.com >>> <mailto:akihiko.odaki@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. >>> > >>> > >>> > I think that suggesting RSS feature when in fact it is not >>> available is >>> > not a good idea, this rather desinforms the guest. >>> > Existing behavior (IMHO) makes more sense. >>> > We can extend this discussion if needed, of course. >>> >>> This change is not to advertise RSS when it's not available; it instead >>> reports an error to the user. >>> >>> For example, think of the following command line: >>> qemu-system-x86_64 -device virtio-net,rss=on,netdev=n -netdev user,id=n >>> >>> Before this change, it gives no error and the user will not know RSS is >>> not available. With this change it now gives an error as follows: >>> qemu-system-x86_64: -device virtio-net,rss=on,netdev=n: Can't load >>> eBPF RSS >>> >>> >>> Does this mean failure to run QEMU if the RSS required in the command >>> line and EBPF can't be loaded? >>> (for example if we run the system with kernel < 5.8)? >>> I'm not sure this is user-friendly behavior... >> >> This patch is wrong that it assumes software RSS is not enabled at all; >> I missed the vhost check before clearing VIRTIO_NET_F_RSS in >> virtio_net_get_features(). >> >> That said, I indeed intend to make it return a hard error for the case >> that RSS is requested for vhost but eBPF can't be loaded. >> >> I believe the current behavior of implicitly disabling a feature >> explicitly requested by the user is not good, > > Yes, but it has been there for years. It complicates a lot to stick to > the migration compatibility. > >> but we can still emit a warning instead of an error. >> >> It's better to follow a convention common in QEMU but I see no >> documentation regarding this kind of situation. I know virtio-gpu gives >> an error in such a case but it's just one example. > > The problem is that it's too late to fix old Qemu, so we probably > can't do more than using compatibility flags... This patch does not affect migration in my understanding; it does not touch any VM state or runtime behavior. We had another discussion regarding migration for patch "virtio-net: Do not clear VIRTIO_NET_F_HASH_REPORT". It does change the runtime behavior so we need to take migration into account. I still think the patch does not require a compatibility flag since it only exposes a new feature and does not prevent migrating from old QEMU that exposes less features. It instead fixes the case where migrating between hosts with different tap feature sets. Regards, Akihiko Odaki
On Wed, Nov 01, 2023 at 01:50:00PM +0900, Akihiko Odaki wrote: > We had another discussion regarding migration for patch "virtio-net: Do not > clear VIRTIO_NET_F_HASH_REPORT". It does change the runtime behavior so we > need to take migration into account. I still think the patch does not > require a compatibility flag since it only exposes a new feature and does > not prevent migrating from old QEMU that exposes less features. It instead > fixes the case where migrating between hosts with different tap feature > sets. When in doubt, add a compat flag.
On 2023/11/01 15:38, Michael S. Tsirkin wrote: > On Wed, Nov 01, 2023 at 01:50:00PM +0900, Akihiko Odaki wrote: >> We had another discussion regarding migration for patch "virtio-net: Do not >> clear VIRTIO_NET_F_HASH_REPORT". It does change the runtime behavior so we >> need to take migration into account. I still think the patch does not >> require a compatibility flag since it only exposes a new feature and does >> not prevent migrating from old QEMU that exposes less features. It instead >> fixes the case where migrating between hosts with different tap feature >> sets. > > When in doubt, add a compat flag. Personally I'm confident about the migration compatibility with patch "virtio-net: Do not clear VIRTIO_NET_F_HASH_REPORT". virtio-net already does the same thing when the tap implementation on the destination implements virtio-net header support while the counterpart of the source does not.
On Wed, Nov 01, 2023 at 05:35:50PM +0900, Akihiko Odaki wrote: > On 2023/11/01 15:38, Michael S. Tsirkin wrote: > > On Wed, Nov 01, 2023 at 01:50:00PM +0900, Akihiko Odaki wrote: > > > We had another discussion regarding migration for patch "virtio-net: Do not > > > clear VIRTIO_NET_F_HASH_REPORT". It does change the runtime behavior so we > > > need to take migration into account. I still think the patch does not > > > require a compatibility flag since it only exposes a new feature and does > > > not prevent migrating from old QEMU that exposes less features. It instead > > > fixes the case where migrating between hosts with different tap feature > > > sets. > > > > When in doubt, add a compat flag. > > Personally I'm confident about the migration compatibility with patch > "virtio-net: Do not clear VIRTIO_NET_F_HASH_REPORT". virtio-net already does > the same thing when the tap implementation on the destination implements > virtio-net header support while the counterpart of the source does not. Trust me there's been so many times where we were very sure and problems come up later. Just don't enable new functionality for old machine types, problem solved. Why is this hard?
On 2023/11/01 18:09, Michael S. Tsirkin wrote: > On Wed, Nov 01, 2023 at 05:35:50PM +0900, Akihiko Odaki wrote: >> On 2023/11/01 15:38, Michael S. Tsirkin wrote: >>> On Wed, Nov 01, 2023 at 01:50:00PM +0900, Akihiko Odaki wrote: >>>> We had another discussion regarding migration for patch "virtio-net: Do not >>>> clear VIRTIO_NET_F_HASH_REPORT". It does change the runtime behavior so we >>>> need to take migration into account. I still think the patch does not >>>> require a compatibility flag since it only exposes a new feature and does >>>> not prevent migrating from old QEMU that exposes less features. It instead >>>> fixes the case where migrating between hosts with different tap feature >>>> sets. >>> >>> When in doubt, add a compat flag. >> >> Personally I'm confident about the migration compatibility with patch >> "virtio-net: Do not clear VIRTIO_NET_F_HASH_REPORT". virtio-net already does >> the same thing when the tap implementation on the destination implements >> virtio-net header support while the counterpart of the source does not. > > Trust me there's been so many times where we were very sure and > problems come up later. Just don't enable new functionality for > old machine types, problem solved. Why is this hard? I see. I'll add a compatibility flag for VIRTIO_NET_F_HASH_REPORT exposure; it should be quite easy.
Probably we mix two different patches in this discussion. Focusing on the patch in the e-mail header: IMO it is not acceptable to fail QEMU run for one feature that we can't make active when we silently drop all other features in such a case. On Wed, Nov 1, 2023 at 11:15 AM Akihiko Odaki <akihiko.odaki@daynix.com> wrote: > On 2023/11/01 18:09, Michael S. Tsirkin wrote: > > On Wed, Nov 01, 2023 at 05:35:50PM +0900, Akihiko Odaki wrote: > >> On 2023/11/01 15:38, Michael S. Tsirkin wrote: > >>> On Wed, Nov 01, 2023 at 01:50:00PM +0900, Akihiko Odaki wrote: > >>>> We had another discussion regarding migration for patch "virtio-net: > Do not > >>>> clear VIRTIO_NET_F_HASH_REPORT". It does change the runtime behavior > so we > >>>> need to take migration into account. I still think the patch does not > >>>> require a compatibility flag since it only exposes a new feature and > does > >>>> not prevent migrating from old QEMU that exposes less features. It > instead > >>>> fixes the case where migrating between hosts with different tap > feature > >>>> sets. > >>> > >>> When in doubt, add a compat flag. > >> > >> Personally I'm confident about the migration compatibility with patch > >> "virtio-net: Do not clear VIRTIO_NET_F_HASH_REPORT". virtio-net already > does > >> the same thing when the tap implementation on the destination implements > >> virtio-net header support while the counterpart of the source does not. > > > > Trust me there's been so many times where we were very sure and > > problems come up later. Just don't enable new functionality for > > old machine types, problem solved. Why is this hard? > > I see. I'll add a compatibility flag for VIRTIO_NET_F_HASH_REPORT > exposure; it should be quite easy. >
On Thu, Nov 02, 2023 at 11:09:27AM +0200, Yuri Benditovich wrote: > Probably we mix two different patches in this discussion. > Focusing on the patch in the e-mail header: > > IMO it is not acceptable to fail QEMU run for one feature that we can't make > active when we silently drop all other features in such a case. If the feature is off by default then it seems more reasonable and silent masking can be seen as a bug. Most virtio features are on by default this is why it's reasonable to mask them.
On Thu, Nov 2, 2023 at 11:33 AM Michael S. Tsirkin <mst@redhat.com> wrote: > On Thu, Nov 02, 2023 at 11:09:27AM +0200, Yuri Benditovich wrote: > > Probably we mix two different patches in this discussion. > > Focusing on the patch in the e-mail header: > > > > IMO it is not acceptable to fail QEMU run for one feature that we can't > make > > active when we silently drop all other features in such a case. > > If the feature is off by default then it seems more reasonable > and silent masking can be seen as a bug. > Most virtio features are on by default this is why it's > reasonable to mask them. > > If we are talking about RSS: setting it initially off is the development time decision. When it will be completely stable there is no reason to keep it off by default, so this is more a question of time and of a readiness of libvirt. > -- > MST > >
On Thu, Nov 02, 2023 at 12:20:39PM +0200, Yuri Benditovich wrote: > > > On Thu, Nov 2, 2023 at 11:33 AM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Thu, Nov 02, 2023 at 11:09:27AM +0200, Yuri Benditovich wrote: > > Probably we mix two different patches in this discussion. > > Focusing on the patch in the e-mail header: > > > > IMO it is not acceptable to fail QEMU run for one feature that we can't > make > > active when we silently drop all other features in such a case. > > If the feature is off by default then it seems more reasonable > and silent masking can be seen as a bug. > Most virtio features are on by default this is why it's > reasonable to mask them. > > > > If we are talking about RSS: setting it initially off is the development time > decision. > When it will be completely stable there is no reason to keep it off by default, > so this is more a question of time and of a readiness of libvirt. Well when we flip the default we'll need compat machinery for sure ;)
On Thu, Nov 2, 2023 at 1:26 PM Michael S. Tsirkin <mst@redhat.com> wrote: > On Thu, Nov 02, 2023 at 12:20:39PM +0200, Yuri Benditovich wrote: > > > > > > On Thu, Nov 2, 2023 at 11:33 AM Michael S. Tsirkin <mst@redhat.com> > wrote: > > > > On Thu, Nov 02, 2023 at 11:09:27AM +0200, Yuri Benditovich wrote: > > > Probably we mix two different patches in this discussion. > > > Focusing on the patch in the e-mail header: > > > > > > IMO it is not acceptable to fail QEMU run for one feature that we > can't > > make > > > active when we silently drop all other features in such a case. > > > > If the feature is off by default then it seems more reasonable > > and silent masking can be seen as a bug. > > Most virtio features are on by default this is why it's > > reasonable to mask them. > > > > > > > > If we are talking about RSS: setting it initially off is the development > time > > decision. > > When it will be completely stable there is no reason to keep it off by > default, > > so this is more a question of time and of a readiness of libvirt. > > Well when we flip the default we'll need compat machinery for sure ;) > Of course, on the flip or default we'll need to keep compatibility to earlier machine types. But, because in the perspective it makes sense to make the RSS is on by default, I do not think we need _now_ to make qemu fail to start if the ebpf can't be loaded. > > -- > MST > >
On Thu, Nov 02, 2023 at 02:00:46PM +0200, Yuri Benditovich wrote: > > > On Thu, Nov 2, 2023 at 1:26 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Thu, Nov 02, 2023 at 12:20:39PM +0200, Yuri Benditovich wrote: > > > > > > On Thu, Nov 2, 2023 at 11:33 AM Michael S. Tsirkin <mst@redhat.com> > wrote: > > > > On Thu, Nov 02, 2023 at 11:09:27AM +0200, Yuri Benditovich wrote: > > > Probably we mix two different patches in this discussion. > > > Focusing on the patch in the e-mail header: > > > > > > IMO it is not acceptable to fail QEMU run for one feature that we > can't > > make > > > active when we silently drop all other features in such a case. > > > > If the feature is off by default then it seems more reasonable > > and silent masking can be seen as a bug. > > Most virtio features are on by default this is why it's > > reasonable to mask them. > > > > > > > > If we are talking about RSS: setting it initially off is the development > time > > decision. > > When it will be completely stable there is no reason to keep it off by > default, > > so this is more a question of time and of a readiness of libvirt. > > Well when we flip the default we'll need compat machinery for sure ;) > > > Of course, on the flip or default we'll need to keep compatibility to earlier > machine types. > But, because in the perspective it makes sense to make the RSS is on by > default, I do not think we need _now_ to make qemu fail to start if the ebpf > can't be loaded. > Generally if user asks for something and we can't do it, we must fail. If we can't apply a default we need a better default. qemu has a bug in that it can't generally distinguish between default set automatically and same default given on command line.
On 2023/11/02 19:20, Yuri Benditovich wrote: > > > On Thu, Nov 2, 2023 at 11:33 AM Michael S. Tsirkin <mst@redhat.com > <mailto:mst@redhat.com>> wrote: > > On Thu, Nov 02, 2023 at 11:09:27AM +0200, Yuri Benditovich wrote: > > Probably we mix two different patches in this discussion. > > Focusing on the patch in the e-mail header: > > > > IMO it is not acceptable to fail QEMU run for one feature that we > can't make > > active when we silently drop all other features in such a case. > > If the feature is off by default then it seems more reasonable > and silent masking can be seen as a bug. > Most virtio features are on by default this is why it's > reasonable to mask them. > > > If we are talking about RSS: setting it initially off is the development > time decision. > When it will be completely stable there is no reason to keep it off by > default, so this is more a question of time and of a readiness of libvirt. It is not ok to make "on" the default; that will enable RSS even when eBPF steering support is not present and can result in performance degradation. We will need OnOffAuto instead of a simple boolean value if we are going to enable RSS when eBPF steering support is available; "auto" will be the default and will enable RSS if and only if eBPF steering support is available. "on" will not be default so it's better to validate if RSS is available when the user explicitly specified "on" for the "rss" property.
On Thu, Nov 2, 2023 at 4:56 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote: > On 2023/11/02 19:20, Yuri Benditovich wrote: > > > > > > On Thu, Nov 2, 2023 at 11:33 AM Michael S. Tsirkin <mst@redhat.com > > <mailto:mst@redhat.com>> wrote: > > > > On Thu, Nov 02, 2023 at 11:09:27AM +0200, Yuri Benditovich wrote: > > > Probably we mix two different patches in this discussion. > > > Focusing on the patch in the e-mail header: > > > > > > IMO it is not acceptable to fail QEMU run for one feature that we > > can't make > > > active when we silently drop all other features in such a case. > > > > If the feature is off by default then it seems more reasonable > > and silent masking can be seen as a bug. > > Most virtio features are on by default this is why it's > > reasonable to mask them. > > > > > > If we are talking about RSS: setting it initially off is the development > > time decision. > > When it will be completely stable there is no reason to keep it off by > > default, so this is more a question of time and of a readiness of > libvirt. > > It is not ok to make "on" the default; that will enable RSS even when > eBPF steering support is not present and can result in performance > degradation. > Exactly as it is today - with vhost=on the host does not suggest RSS without eBPF. I do not understand what you call "performance degradation", can you describe the scenario? > > We will need OnOffAuto instead of a simple boolean value if we are going > to enable RSS when eBPF steering support is available; "auto" will be > the default and will enable RSS if and only if eBPF steering support is > available. "on" will not be default so it's better to validate if RSS is > available when the user explicitly specified "on" for the "rss" property. >
On 2023/11/03 18:35, Yuri Benditovich wrote: > > > On Thu, Nov 2, 2023 at 4:56 PM Akihiko Odaki <akihiko.odaki@daynix.com > <mailto:akihiko.odaki@daynix.com>> wrote: > > On 2023/11/02 19:20, Yuri Benditovich wrote: > > > > > > On Thu, Nov 2, 2023 at 11:33 AM Michael S. Tsirkin > <mst@redhat.com <mailto:mst@redhat.com> > > <mailto:mst@redhat.com <mailto:mst@redhat.com>>> wrote: > > > > On Thu, Nov 02, 2023 at 11:09:27AM +0200, Yuri Benditovich wrote: > > > Probably we mix two different patches in this discussion. > > > Focusing on the patch in the e-mail header: > > > > > > IMO it is not acceptable to fail QEMU run for one feature > that we > > can't make > > > active when we silently drop all other features in such a > case. > > > > If the feature is off by default then it seems more reasonable > > and silent masking can be seen as a bug. > > Most virtio features are on by default this is why it's > > reasonable to mask them. > > > > > > If we are talking about RSS: setting it initially off is the > development > > time decision. > > When it will be completely stable there is no reason to keep it > off by > > default, so this is more a question of time and of a readiness of > libvirt. > > It is not ok to make "on" the default; that will enable RSS even when > eBPF steering support is not present and can result in performance > degradation. > > > Exactly as it is today - with vhost=on the host does not suggest RSS > without eBPF. > I do not understand what you call "performance degradation", can you > describe the scenario? I was not clear, but I was talking about the case of vhost=off or peers other than tap (e.g., user). rss=on employs in-qemu RSS, which incurs overheads for such configurations.
On Fri, Nov 3, 2023 at 11:55 AM Akihiko Odaki <akihiko.odaki@daynix.com> wrote: > On 2023/11/03 18:35, Yuri Benditovich wrote: > > > > > > On Thu, Nov 2, 2023 at 4:56 PM Akihiko Odaki <akihiko.odaki@daynix.com > > <mailto:akihiko.odaki@daynix.com>> wrote: > > > > On 2023/11/02 19:20, Yuri Benditovich wrote: > > > > > > > > > On Thu, Nov 2, 2023 at 11:33 AM Michael S. Tsirkin > > <mst@redhat.com <mailto:mst@redhat.com> > > > <mailto:mst@redhat.com <mailto:mst@redhat.com>>> wrote: > > > > > > On Thu, Nov 02, 2023 at 11:09:27AM +0200, Yuri Benditovich > wrote: > > > > Probably we mix two different patches in this discussion. > > > > Focusing on the patch in the e-mail header: > > > > > > > > IMO it is not acceptable to fail QEMU run for one feature > > that we > > > can't make > > > > active when we silently drop all other features in such a > > case. > > > > > > If the feature is off by default then it seems more reasonable > > > and silent masking can be seen as a bug. > > > Most virtio features are on by default this is why it's > > > reasonable to mask them. > > > > > > > > > If we are talking about RSS: setting it initially off is the > > development > > > time decision. > > > When it will be completely stable there is no reason to keep it > > off by > > > default, so this is more a question of time and of a readiness of > > libvirt. > > > > It is not ok to make "on" the default; that will enable RSS even when > > eBPF steering support is not present and can result in performance > > degradation. > > > > > > Exactly as it is today - with vhost=on the host does not suggest RSS > > without eBPF. > > I do not understand what you call "performance degradation", can you > > describe the scenario? > > I was not clear, but I was talking about the case of vhost=off or peers > other than tap (e.g., user). rss=on employs in-qemu RSS, which incurs > overheads for such configurations. > So, vhost=off OR peers other than tap: In the case of peers other than tap (IMO) we're not talking about performance at all. Backends like "user" (without vnet_hdr) do not support _many_ performance-oriented features. If RSS is somehow "supported" for such backends this is rather a misunderstanding (IMO again). In the case of tap with vhost=off the RSS support does not create any performance degradation without eBPF.
On 2023/11/03 22:14, Yuri Benditovich wrote: > > > On Fri, Nov 3, 2023 at 11:55 AM Akihiko Odaki <akihiko.odaki@daynix.com > <mailto:akihiko.odaki@daynix.com>> wrote: > > On 2023/11/03 18:35, Yuri Benditovich wrote: > > > > > > On Thu, Nov 2, 2023 at 4:56 PM Akihiko Odaki > <akihiko.odaki@daynix.com <mailto:akihiko.odaki@daynix.com> > > <mailto:akihiko.odaki@daynix.com > <mailto:akihiko.odaki@daynix.com>>> wrote: > > > > On 2023/11/02 19:20, Yuri Benditovich wrote: > > > > > > > > > On Thu, Nov 2, 2023 at 11:33 AM Michael S. Tsirkin > > <mst@redhat.com <mailto:mst@redhat.com> > <mailto:mst@redhat.com <mailto:mst@redhat.com>> > > > <mailto:mst@redhat.com <mailto:mst@redhat.com> > <mailto:mst@redhat.com <mailto:mst@redhat.com>>>> wrote: > > > > > > On Thu, Nov 02, 2023 at 11:09:27AM +0200, Yuri > Benditovich wrote: > > > > Probably we mix two different patches in this > discussion. > > > > Focusing on the patch in the e-mail header: > > > > > > > > IMO it is not acceptable to fail QEMU run for one > feature > > that we > > > can't make > > > > active when we silently drop all other features in > such a > > case. > > > > > > If the feature is off by default then it seems more > reasonable > > > and silent masking can be seen as a bug. > > > Most virtio features are on by default this is why it's > > > reasonable to mask them. > > > > > > > > > If we are talking about RSS: setting it initially off is the > > development > > > time decision. > > > When it will be completely stable there is no reason to > keep it > > off by > > > default, so this is more a question of time and of a > readiness of > > libvirt. > > > > It is not ok to make "on" the default; that will enable RSS > even when > > eBPF steering support is not present and can result in > performance > > degradation. > > > > > > Exactly as it is today - with vhost=on the host does not suggest RSS > > without eBPF. > > I do not understand what you call "performance degradation", can you > > describe the scenario? > > I was not clear, but I was talking about the case of vhost=off or peers > other than tap (e.g., user). rss=on employs in-qemu RSS, which incurs > overheads for such configurations. > > > So, vhost=off OR peers other than tap: > > In the case of peers other than tap (IMO) we're not talking about > performance at all. > Backends like "user" (without vnet_hdr) do not support _many_ > performance-oriented features. > If RSS is somehow "supported" for such backends this is rather a > misunderstanding (IMO again). We do not need to ensure good performance when RSS is enabled by the guest for backends without eBPF steering program as you say. In-QEMU RSS is only useful for testing and not meant to improve the performance. However, if you set rss=on, QEMU will advertise the availability of RSS feature. The guest will have no mean to know if it's implemented in a way not performance-wise so it may decide to use the feature to improve the performance, which can result in performance degradation. Therefore, it's better not to set rss=on for such backends.
On Sat, Nov 11, 2023 at 5:28 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote: > On 2023/11/03 22:14, Yuri Benditovich wrote: > > > > > > On Fri, Nov 3, 2023 at 11:55 AM Akihiko Odaki <akihiko.odaki@daynix.com > > <mailto:akihiko.odaki@daynix.com>> wrote: > > > > On 2023/11/03 18:35, Yuri Benditovich wrote: > > > > > > > > > On Thu, Nov 2, 2023 at 4:56 PM Akihiko Odaki > > <akihiko.odaki@daynix.com <mailto:akihiko.odaki@daynix.com> > > > <mailto:akihiko.odaki@daynix.com > > <mailto:akihiko.odaki@daynix.com>>> wrote: > > > > > > On 2023/11/02 19:20, Yuri Benditovich wrote: > > > > > > > > > > > > On Thu, Nov 2, 2023 at 11:33 AM Michael S. Tsirkin > > > <mst@redhat.com <mailto:mst@redhat.com> > > <mailto:mst@redhat.com <mailto:mst@redhat.com>> > > > > <mailto:mst@redhat.com <mailto:mst@redhat.com> > > <mailto:mst@redhat.com <mailto:mst@redhat.com>>>> wrote: > > > > > > > > On Thu, Nov 02, 2023 at 11:09:27AM +0200, Yuri > > Benditovich wrote: > > > > > Probably we mix two different patches in this > > discussion. > > > > > Focusing on the patch in the e-mail header: > > > > > > > > > > IMO it is not acceptable to fail QEMU run for one > > feature > > > that we > > > > can't make > > > > > active when we silently drop all other features in > > such a > > > case. > > > > > > > > If the feature is off by default then it seems more > > reasonable > > > > and silent masking can be seen as a bug. > > > > Most virtio features are on by default this is why it's > > > > reasonable to mask them. > > > > > > > > > > > > If we are talking about RSS: setting it initially off is > the > > > development > > > > time decision. > > > > When it will be completely stable there is no reason to > > keep it > > > off by > > > > default, so this is more a question of time and of a > > readiness of > > > libvirt. > > > > > > It is not ok to make "on" the default; that will enable RSS > > even when > > > eBPF steering support is not present and can result in > > performance > > > degradation. > > > > > > > > > Exactly as it is today - with vhost=on the host does not suggest > RSS > > > without eBPF. > > > I do not understand what you call "performance degradation", can > you > > > describe the scenario? > > > > I was not clear, but I was talking about the case of vhost=off or > peers > > other than tap (e.g., user). rss=on employs in-qemu RSS, which incurs > > overheads for such configurations. > > > > > > So, vhost=off OR peers other than tap: > > > > In the case of peers other than tap (IMO) we're not talking about > > performance at all. > > Backends like "user" (without vnet_hdr) do not support _many_ > > performance-oriented features. > > If RSS is somehow "supported" for such backends this is rather a > > misunderstanding (IMO again). > > We do not need to ensure good performance when RSS is enabled by the > guest for backends without eBPF steering program as you say. In-QEMU RSS > is only useful for testing and not meant to improve the performance. > > However, if you set rss=on, QEMU will advertise the availability of RSS > feature. The guest will have no mean to know if it's implemented in a > way not performance-wise so it may decide to use the feature to improve > the performance, which can result in performance degradation. Therefore, > it's better not to set rss=on for such backends. > I still do not understand what is the scenario where you see or suspect the mentioned "performance degradation". We can discuss whether such a problem exists as soon as you explain it.
On 2023/11/13 20:44, Yuri Benditovich wrote: > > > On Sat, Nov 11, 2023 at 5:28 PM Akihiko Odaki <akihiko.odaki@daynix.com > <mailto:akihiko.odaki@daynix.com>> wrote: > > On 2023/11/03 22:14, Yuri Benditovich wrote: > > > > > > On Fri, Nov 3, 2023 at 11:55 AM Akihiko Odaki > <akihiko.odaki@daynix.com <mailto:akihiko.odaki@daynix.com> > > <mailto:akihiko.odaki@daynix.com > <mailto:akihiko.odaki@daynix.com>>> wrote: > > > > On 2023/11/03 18:35, Yuri Benditovich wrote: > > > > > > > > > On Thu, Nov 2, 2023 at 4:56 PM Akihiko Odaki > > <akihiko.odaki@daynix.com <mailto:akihiko.odaki@daynix.com> > <mailto:akihiko.odaki@daynix.com <mailto:akihiko.odaki@daynix.com>> > > > <mailto:akihiko.odaki@daynix.com > <mailto:akihiko.odaki@daynix.com> > > <mailto:akihiko.odaki@daynix.com > <mailto:akihiko.odaki@daynix.com>>>> wrote: > > > > > > On 2023/11/02 19:20, Yuri Benditovich wrote: > > > > > > > > > > > > On Thu, Nov 2, 2023 at 11:33 AM Michael S. Tsirkin > > > <mst@redhat.com <mailto:mst@redhat.com> > <mailto:mst@redhat.com <mailto:mst@redhat.com>> > > <mailto:mst@redhat.com <mailto:mst@redhat.com> > <mailto:mst@redhat.com <mailto:mst@redhat.com>>> > > > > <mailto:mst@redhat.com <mailto:mst@redhat.com> > <mailto:mst@redhat.com <mailto:mst@redhat.com>> > > <mailto:mst@redhat.com <mailto:mst@redhat.com> > <mailto:mst@redhat.com <mailto:mst@redhat.com>>>>> wrote: > > > > > > > > On Thu, Nov 02, 2023 at 11:09:27AM +0200, Yuri > > Benditovich wrote: > > > > > Probably we mix two different patches in this > > discussion. > > > > > Focusing on the patch in the e-mail header: > > > > > > > > > > IMO it is not acceptable to fail QEMU run > for one > > feature > > > that we > > > > can't make > > > > > active when we silently drop all other > features in > > such a > > > case. > > > > > > > > If the feature is off by default then it seems more > > reasonable > > > > and silent masking can be seen as a bug. > > > > Most virtio features are on by default this is > why it's > > > > reasonable to mask them. > > > > > > > > > > > > If we are talking about RSS: setting it initially > off is the > > > development > > > > time decision. > > > > When it will be completely stable there is no reason to > > keep it > > > off by > > > > default, so this is more a question of time and of a > > readiness of > > > libvirt. > > > > > > It is not ok to make "on" the default; that will > enable RSS > > even when > > > eBPF steering support is not present and can result in > > performance > > > degradation. > > > > > > > > > Exactly as it is today - with vhost=on the host does not > suggest RSS > > > without eBPF. > > > I do not understand what you call "performance > degradation", can you > > > describe the scenario? > > > > I was not clear, but I was talking about the case of > vhost=off or peers > > other than tap (e.g., user). rss=on employs in-qemu RSS, > which incurs > > overheads for such configurations. > > > > > > So, vhost=off OR peers other than tap: > > > > In the case of peers other than tap (IMO) we're not talking about > > performance at all. > > Backends like "user" (without vnet_hdr) do not support _many_ > > performance-oriented features. > > If RSS is somehow "supported" for such backends this is rather a > > misunderstanding (IMO again). > > We do not need to ensure good performance when RSS is enabled by the > guest for backends without eBPF steering program as you say. In-QEMU > RSS > is only useful for testing and not meant to improve the performance. > > However, if you set rss=on, QEMU will advertise the availability of RSS > feature. The guest will have no mean to know if it's implemented in a > way not performance-wise so it may decide to use the feature to improve > the performance, which can result in performance degradation. > Therefore, > it's better not to set rss=on for such backends. > > > I still do not understand what is the scenario where you see or suspect > the mentioned "performance degradation". > We can discuss whether such a problem exists as soon as you explain it. The scenario is that: - rss=on, - A backend without eBPF steering support is in use, and - The guest expects VIRTIO_NET_F_RSS has little overheads as hardware RSS implementations do. I consider the risk of the performance degradation in such a situation is the reason why virtio-net emits a warning ("Can't load eBPF RSS - fallback to software RSS") when in-QEMU RSS is in use.
On Mon, Nov 13, 2023 at 2:44 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote: > On 2023/11/13 20:44, Yuri Benditovich wrote: > > > > > > On Sat, Nov 11, 2023 at 5:28 PM Akihiko Odaki <akihiko.odaki@daynix.com > > <mailto:akihiko.odaki@daynix.com>> wrote: > > > > On 2023/11/03 22:14, Yuri Benditovich wrote: > > > > > > > > > On Fri, Nov 3, 2023 at 11:55 AM Akihiko Odaki > > <akihiko.odaki@daynix.com <mailto:akihiko.odaki@daynix.com> > > > <mailto:akihiko.odaki@daynix.com > > <mailto:akihiko.odaki@daynix.com>>> wrote: > > > > > > On 2023/11/03 18:35, Yuri Benditovich wrote: > > > > > > > > > > > > On Thu, Nov 2, 2023 at 4:56 PM Akihiko Odaki > > > <akihiko.odaki@daynix.com <mailto:akihiko.odaki@daynix.com> > > <mailto:akihiko.odaki@daynix.com <mailto:akihiko.odaki@daynix.com>> > > > > <mailto:akihiko.odaki@daynix.com > > <mailto:akihiko.odaki@daynix.com> > > > <mailto:akihiko.odaki@daynix.com > > <mailto:akihiko.odaki@daynix.com>>>> wrote: > > > > > > > > On 2023/11/02 19:20, Yuri Benditovich wrote: > > > > > > > > > > > > > > > On Thu, Nov 2, 2023 at 11:33 AM Michael S. Tsirkin > > > > <mst@redhat.com <mailto:mst@redhat.com> > > <mailto:mst@redhat.com <mailto:mst@redhat.com>> > > > <mailto:mst@redhat.com <mailto:mst@redhat.com> > > <mailto:mst@redhat.com <mailto:mst@redhat.com>>> > > > > > <mailto:mst@redhat.com <mailto:mst@redhat.com> > > <mailto:mst@redhat.com <mailto:mst@redhat.com>> > > > <mailto:mst@redhat.com <mailto:mst@redhat.com> > > <mailto:mst@redhat.com <mailto:mst@redhat.com>>>>> wrote: > > > > > > > > > > On Thu, Nov 02, 2023 at 11:09:27AM +0200, Yuri > > > Benditovich wrote: > > > > > > Probably we mix two different patches in this > > > discussion. > > > > > > Focusing on the patch in the e-mail header: > > > > > > > > > > > > IMO it is not acceptable to fail QEMU run > > for one > > > feature > > > > that we > > > > > can't make > > > > > > active when we silently drop all other > > features in > > > such a > > > > case. > > > > > > > > > > If the feature is off by default then it seems > more > > > reasonable > > > > > and silent masking can be seen as a bug. > > > > > Most virtio features are on by default this is > > why it's > > > > > reasonable to mask them. > > > > > > > > > > > > > > > If we are talking about RSS: setting it initially > > off is the > > > > development > > > > > time decision. > > > > > When it will be completely stable there is no > reason to > > > keep it > > > > off by > > > > > default, so this is more a question of time and of a > > > readiness of > > > > libvirt. > > > > > > > > It is not ok to make "on" the default; that will > > enable RSS > > > even when > > > > eBPF steering support is not present and can result in > > > performance > > > > degradation. > > > > > > > > > > > > Exactly as it is today - with vhost=on the host does not > > suggest RSS > > > > without eBPF. > > > > I do not understand what you call "performance > > degradation", can you > > > > describe the scenario? > > > > > > I was not clear, but I was talking about the case of > > vhost=off or peers > > > other than tap (e.g., user). rss=on employs in-qemu RSS, > > which incurs > > > overheads for such configurations. > > > > > > > > > So, vhost=off OR peers other than tap: > > > > > > In the case of peers other than tap (IMO) we're not talking about > > > performance at all. > > > Backends like "user" (without vnet_hdr) do not support _many_ > > > performance-oriented features. > > > If RSS is somehow "supported" for such backends this is rather a > > > misunderstanding (IMO again). > > > > We do not need to ensure good performance when RSS is enabled by the > > guest for backends without eBPF steering program as you say. In-QEMU > > RSS > > is only useful for testing and not meant to improve the performance. > > > > However, if you set rss=on, QEMU will advertise the availability of > RSS > > feature. The guest will have no mean to know if it's implemented in a > > way not performance-wise so it may decide to use the feature to > improve > > the performance, which can result in performance degradation. > > Therefore, > > it's better not to set rss=on for such backends. > > > > > > I still do not understand what is the scenario where you see or suspect > > the mentioned "performance degradation". > > We can discuss whether such a problem exists as soon as you explain it. > > The scenario is that: > - rss=on, > - A backend without eBPF steering support is in use, and > - The guest expects VIRTIO_NET_F_RSS has little overheads as hardware > RSS implementations do. > > I consider the risk of the performance degradation in such a situation > is the reason why virtio-net emits a warning ("Can't load eBPF RSS - > fallback to software RSS") when in-QEMU RSS is in use. > In a described scenario (vhost=off) I do not see why the performance degradation should happen: the SW RSS (if activated) will place each packet into proper queue (even if the auto_mq in kernel is not able to do that) and such a way the guest will not need to reschedule the packet to proper CPU
On 2023/11/14 2:26, Yuri Benditovich wrote: > > > On Mon, Nov 13, 2023 at 2:44 PM Akihiko Odaki <akihiko.odaki@daynix.com > <mailto:akihiko.odaki@daynix.com>> wrote: > > On 2023/11/13 20:44, Yuri Benditovich wrote: > > > > > > On Sat, Nov 11, 2023 at 5:28 PM Akihiko Odaki > <akihiko.odaki@daynix.com <mailto:akihiko.odaki@daynix.com> > > <mailto:akihiko.odaki@daynix.com > <mailto:akihiko.odaki@daynix.com>>> wrote: > > > > On 2023/11/03 22:14, Yuri Benditovich wrote: > > > > > > > > > On Fri, Nov 3, 2023 at 11:55 AM Akihiko Odaki > > <akihiko.odaki@daynix.com <mailto:akihiko.odaki@daynix.com> > <mailto:akihiko.odaki@daynix.com <mailto:akihiko.odaki@daynix.com>> > > > <mailto:akihiko.odaki@daynix.com > <mailto:akihiko.odaki@daynix.com> > > <mailto:akihiko.odaki@daynix.com > <mailto:akihiko.odaki@daynix.com>>>> wrote: > > > > > > On 2023/11/03 18:35, Yuri Benditovich wrote: > > > > > > > > > > > > On Thu, Nov 2, 2023 at 4:56 PM Akihiko Odaki > > > <akihiko.odaki@daynix.com > <mailto:akihiko.odaki@daynix.com> <mailto:akihiko.odaki@daynix.com > <mailto:akihiko.odaki@daynix.com>> > > <mailto:akihiko.odaki@daynix.com > <mailto:akihiko.odaki@daynix.com> <mailto:akihiko.odaki@daynix.com > <mailto:akihiko.odaki@daynix.com>>> > > > > <mailto:akihiko.odaki@daynix.com > <mailto:akihiko.odaki@daynix.com> > > <mailto:akihiko.odaki@daynix.com > <mailto:akihiko.odaki@daynix.com>> > > > <mailto:akihiko.odaki@daynix.com > <mailto:akihiko.odaki@daynix.com> > > <mailto:akihiko.odaki@daynix.com > <mailto:akihiko.odaki@daynix.com>>>>> wrote: > > > > > > > > On 2023/11/02 19:20, Yuri Benditovich wrote: > > > > > > > > > > > > > > > On Thu, Nov 2, 2023 at 11:33 AM Michael S. > Tsirkin > > > > <mst@redhat.com <mailto:mst@redhat.com> > <mailto:mst@redhat.com <mailto:mst@redhat.com>> > > <mailto:mst@redhat.com <mailto:mst@redhat.com> > <mailto:mst@redhat.com <mailto:mst@redhat.com>>> > > > <mailto:mst@redhat.com <mailto:mst@redhat.com> > <mailto:mst@redhat.com <mailto:mst@redhat.com>> > > <mailto:mst@redhat.com <mailto:mst@redhat.com> > <mailto:mst@redhat.com <mailto:mst@redhat.com>>>> > > > > > <mailto:mst@redhat.com > <mailto:mst@redhat.com> <mailto:mst@redhat.com <mailto:mst@redhat.com>> > > <mailto:mst@redhat.com <mailto:mst@redhat.com> > <mailto:mst@redhat.com <mailto:mst@redhat.com>>> > > > <mailto:mst@redhat.com <mailto:mst@redhat.com> > <mailto:mst@redhat.com <mailto:mst@redhat.com>> > > <mailto:mst@redhat.com <mailto:mst@redhat.com> > <mailto:mst@redhat.com <mailto:mst@redhat.com>>>>>> wrote: > > > > > > > > > > On Thu, Nov 02, 2023 at 11:09:27AM > +0200, Yuri > > > Benditovich wrote: > > > > > > Probably we mix two different patches > in this > > > discussion. > > > > > > Focusing on the patch in the e-mail > header: > > > > > > > > > > > > IMO it is not acceptable to fail QEMU run > > for one > > > feature > > > > that we > > > > > can't make > > > > > > active when we silently drop all other > > features in > > > such a > > > > case. > > > > > > > > > > If the feature is off by default then it > seems more > > > reasonable > > > > > and silent masking can be seen as a bug. > > > > > Most virtio features are on by default > this is > > why it's > > > > > reasonable to mask them. > > > > > > > > > > > > > > > If we are talking about RSS: setting it > initially > > off is the > > > > development > > > > > time decision. > > > > > When it will be completely stable there is > no reason to > > > keep it > > > > off by > > > > > default, so this is more a question of time > and of a > > > readiness of > > > > libvirt. > > > > > > > > It is not ok to make "on" the default; that will > > enable RSS > > > even when > > > > eBPF steering support is not present and can > result in > > > performance > > > > degradation. > > > > > > > > > > > > Exactly as it is today - with vhost=on the host > does not > > suggest RSS > > > > without eBPF. > > > > I do not understand what you call "performance > > degradation", can you > > > > describe the scenario? > > > > > > I was not clear, but I was talking about the case of > > vhost=off or peers > > > other than tap (e.g., user). rss=on employs in-qemu RSS, > > which incurs > > > overheads for such configurations. > > > > > > > > > So, vhost=off OR peers other than tap: > > > > > > In the case of peers other than tap (IMO) we're not > talking about > > > performance at all. > > > Backends like "user" (without vnet_hdr) do not support _many_ > > > performance-oriented features. > > > If RSS is somehow "supported" for such backends this is > rather a > > > misunderstanding (IMO again). > > > > We do not need to ensure good performance when RSS is enabled > by the > > guest for backends without eBPF steering program as you say. > In-QEMU > > RSS > > is only useful for testing and not meant to improve the > performance. > > > > However, if you set rss=on, QEMU will advertise the > availability of RSS > > feature. The guest will have no mean to know if it's > implemented in a > > way not performance-wise so it may decide to use the feature > to improve > > the performance, which can result in performance degradation. > > Therefore, > > it's better not to set rss=on for such backends. > > > > > > I still do not understand what is the scenario where you see or > suspect > > the mentioned "performance degradation". > > We can discuss whether such a problem exists as soon as you > explain it. > > The scenario is that: > - rss=on, > - A backend without eBPF steering support is in use, and > - The guest expects VIRTIO_NET_F_RSS has little overheads as hardware > RSS implementations do. > > I consider the risk of the performance degradation in such a situation > is the reason why virtio-net emits a warning ("Can't load eBPF RSS - > fallback to software RSS") when in-QEMU RSS is in use. > > > In a described scenario (vhost=off) I do not see why the performance > degradation should happen: > the SW RSS (if activated) will place each packet into proper queue (even > if the auto_mq in kernel is not able to do that) and such a way the > guest will not need to reschedule the packet to proper CPU > The scenario I'm concerned is that the guest has its own packet steering mechanism which is feature-wise superior to RSS. For example, Linux has such a mechanism called RPS, which has some advantages due to its extensible nature according to: https://www.kernel.org/doc/html/v6.6/networking/scaling.html#rps-receive-packet-steering Such a guest may still prefer hardware RSS if available since hardware RSS is expected to have less overheads. However, it is not true for in-qemu RSS, and using in-QEMU RSS instead of the guest-side steering mechanism may just hide useful features the guest-side steering mechanism has and result in performance degradation. Andrew, I appreciate if you also tell the rationale behind the warning you put for software RSS ("Can't load eBPF RSS - fallback to software RSS").
On Tue, Nov 14, 2023 at 9:03 AM Akihiko Odaki <akihiko.odaki@daynix.com> wrote: > On 2023/11/14 2:26, Yuri Benditovich wrote: > > > > > > On Mon, Nov 13, 2023 at 2:44 PM Akihiko Odaki <akihiko.odaki@daynix.com > > <mailto:akihiko.odaki@daynix.com>> wrote: > > > > On 2023/11/13 20:44, Yuri Benditovich wrote: > > > > > > > > > On Sat, Nov 11, 2023 at 5:28 PM Akihiko Odaki > > <akihiko.odaki@daynix.com <mailto:akihiko.odaki@daynix.com> > > > <mailto:akihiko.odaki@daynix.com > > <mailto:akihiko.odaki@daynix.com>>> wrote: > > > > > > On 2023/11/03 22:14, Yuri Benditovich wrote: > > > > > > > > > > > > On Fri, Nov 3, 2023 at 11:55 AM Akihiko Odaki > > > <akihiko.odaki@daynix.com <mailto:akihiko.odaki@daynix.com> > > <mailto:akihiko.odaki@daynix.com <mailto:akihiko.odaki@daynix.com>> > > > > <mailto:akihiko.odaki@daynix.com > > <mailto:akihiko.odaki@daynix.com> > > > <mailto:akihiko.odaki@daynix.com > > <mailto:akihiko.odaki@daynix.com>>>> wrote: > > > > > > > > On 2023/11/03 18:35, Yuri Benditovich wrote: > > > > > > > > > > > > > > > On Thu, Nov 2, 2023 at 4:56 PM Akihiko Odaki > > > > <akihiko.odaki@daynix.com > > <mailto:akihiko.odaki@daynix.com> <mailto:akihiko.odaki@daynix.com > > <mailto:akihiko.odaki@daynix.com>> > > > <mailto:akihiko.odaki@daynix.com > > <mailto:akihiko.odaki@daynix.com> <mailto:akihiko.odaki@daynix.com > > <mailto:akihiko.odaki@daynix.com>>> > > > > > <mailto:akihiko.odaki@daynix.com > > <mailto:akihiko.odaki@daynix.com> > > > <mailto:akihiko.odaki@daynix.com > > <mailto:akihiko.odaki@daynix.com>> > > > > <mailto:akihiko.odaki@daynix.com > > <mailto:akihiko.odaki@daynix.com> > > > <mailto:akihiko.odaki@daynix.com > > <mailto:akihiko.odaki@daynix.com>>>>> wrote: > > > > > > > > > > On 2023/11/02 19:20, Yuri Benditovich wrote: > > > > > > > > > > > > > > > > > > On Thu, Nov 2, 2023 at 11:33 AM Michael S. > > Tsirkin > > > > > <mst@redhat.com <mailto:mst@redhat.com> > > <mailto:mst@redhat.com <mailto:mst@redhat.com>> > > > <mailto:mst@redhat.com <mailto:mst@redhat.com> > > <mailto:mst@redhat.com <mailto:mst@redhat.com>>> > > > > <mailto:mst@redhat.com <mailto:mst@redhat.com> > > <mailto:mst@redhat.com <mailto:mst@redhat.com>> > > > <mailto:mst@redhat.com <mailto:mst@redhat.com> > > <mailto:mst@redhat.com <mailto:mst@redhat.com>>>> > > > > > > <mailto:mst@redhat.com > > <mailto:mst@redhat.com> <mailto:mst@redhat.com <mailto: > mst@redhat.com>> > > > <mailto:mst@redhat.com <mailto:mst@redhat.com> > > <mailto:mst@redhat.com <mailto:mst@redhat.com>>> > > > > <mailto:mst@redhat.com <mailto:mst@redhat.com> > > <mailto:mst@redhat.com <mailto:mst@redhat.com>> > > > <mailto:mst@redhat.com <mailto:mst@redhat.com> > > <mailto:mst@redhat.com <mailto:mst@redhat.com>>>>>> wrote: > > > > > > > > > > > > On Thu, Nov 02, 2023 at 11:09:27AM > > +0200, Yuri > > > > Benditovich wrote: > > > > > > > Probably we mix two different patches > > in this > > > > discussion. > > > > > > > Focusing on the patch in the e-mail > > header: > > > > > > > > > > > > > > IMO it is not acceptable to fail QEMU > run > > > for one > > > > feature > > > > > that we > > > > > > can't make > > > > > > > active when we silently drop all other > > > features in > > > > such a > > > > > case. > > > > > > > > > > > > If the feature is off by default then it > > seems more > > > > reasonable > > > > > > and silent masking can be seen as a bug. > > > > > > Most virtio features are on by default > > this is > > > why it's > > > > > > reasonable to mask them. > > > > > > > > > > > > > > > > > > If we are talking about RSS: setting it > > initially > > > off is the > > > > > development > > > > > > time decision. > > > > > > When it will be completely stable there is > > no reason to > > > > keep it > > > > > off by > > > > > > default, so this is more a question of time > > and of a > > > > readiness of > > > > > libvirt. > > > > > > > > > > It is not ok to make "on" the default; that will > > > enable RSS > > > > even when > > > > > eBPF steering support is not present and can > > result in > > > > performance > > > > > degradation. > > > > > > > > > > > > > > > Exactly as it is today - with vhost=on the host > > does not > > > suggest RSS > > > > > without eBPF. > > > > > I do not understand what you call "performance > > > degradation", can you > > > > > describe the scenario? > > > > > > > > I was not clear, but I was talking about the case of > > > vhost=off or peers > > > > other than tap (e.g., user). rss=on employs in-qemu > RSS, > > > which incurs > > > > overheads for such configurations. > > > > > > > > > > > > So, vhost=off OR peers other than tap: > > > > > > > > In the case of peers other than tap (IMO) we're not > > talking about > > > > performance at all. > > > > Backends like "user" (without vnet_hdr) do not support > _many_ > > > > performance-oriented features. > > > > If RSS is somehow "supported" for such backends this is > > rather a > > > > misunderstanding (IMO again). > > > > > > We do not need to ensure good performance when RSS is enabled > > by the > > > guest for backends without eBPF steering program as you say. > > In-QEMU > > > RSS > > > is only useful for testing and not meant to improve the > > performance. > > > > > > However, if you set rss=on, QEMU will advertise the > > availability of RSS > > > feature. The guest will have no mean to know if it's > > implemented in a > > > way not performance-wise so it may decide to use the feature > > to improve > > > the performance, which can result in performance degradation. > > > Therefore, > > > it's better not to set rss=on for such backends. > > > > > > > > > I still do not understand what is the scenario where you see or > > suspect > > > the mentioned "performance degradation". > > > We can discuss whether such a problem exists as soon as you > > explain it. > > > > The scenario is that: > > - rss=on, > > - A backend without eBPF steering support is in use, and > > - The guest expects VIRTIO_NET_F_RSS has little overheads as hardware > > RSS implementations do. > > > > I consider the risk of the performance degradation in such a > situation > > is the reason why virtio-net emits a warning ("Can't load eBPF RSS - > > fallback to software RSS") when in-QEMU RSS is in use. > > > > > > In a described scenario (vhost=off) I do not see why the performance > > degradation should happen: > > the SW RSS (if activated) will place each packet into proper queue (even > > if the auto_mq in kernel is not able to do that) and such a way the > > guest will not need to reschedule the packet to proper CPU > > > > The scenario I'm concerned is that the guest has its own packet steering > mechanism which is feature-wise superior to RSS. For example, Linux has > such a mechanism called RPS, which has some advantages due to its > extensible nature according to: > > https://www.kernel.org/doc/html/v6.6/networking/scaling.html#rps-receive-packet-steering > > Such a guest may still prefer hardware RSS if available since hardware > RSS is expected to have less overheads. However, it is not true for > in-qemu RSS, and using in-QEMU RSS instead of the guest-side steering > mechanism may just hide useful features the guest-side steering > mechanism has and result in performance degradation. > Note that in terms of per-packet computation for RSS the in-QEMU RSS does exactly the same operations in native code that the eBPF does in the emulation. So, I wouldn't say that SW RSS brings some "performance degradation". We prefer eBPF as it can serve both vhost and non-vhost setups. > Andrew, I appreciate if you also tell the rationale behind the warning > you put for software RSS ("Can't load eBPF RSS - fallback to software > RSS"). >
On 2023/11/15 7:09, Yuri Benditovich wrote: > > > On Tue, Nov 14, 2023 at 9:03 AM Akihiko Odaki <akihiko.odaki@daynix.com > <mailto:akihiko.odaki@daynix.com>> wrote: > > On 2023/11/14 2:26, Yuri Benditovich wrote: > > > > > > On Mon, Nov 13, 2023 at 2:44 PM Akihiko Odaki > <akihiko.odaki@daynix.com <mailto:akihiko.odaki@daynix.com> > > <mailto:akihiko.odaki@daynix.com > <mailto:akihiko.odaki@daynix.com>>> wrote: > > > > On 2023/11/13 20:44, Yuri Benditovich wrote: > > > > > > > > > On Sat, Nov 11, 2023 at 5:28 PM Akihiko Odaki > > <akihiko.odaki@daynix.com <mailto:akihiko.odaki@daynix.com> > <mailto:akihiko.odaki@daynix.com <mailto:akihiko.odaki@daynix.com>> > > > <mailto:akihiko.odaki@daynix.com > <mailto:akihiko.odaki@daynix.com> > > <mailto:akihiko.odaki@daynix.com > <mailto:akihiko.odaki@daynix.com>>>> wrote: > > > > > > On 2023/11/03 22:14, Yuri Benditovich wrote: > > > > > > > > > > > > On Fri, Nov 3, 2023 at 11:55 AM Akihiko Odaki > > > <akihiko.odaki@daynix.com > <mailto:akihiko.odaki@daynix.com> <mailto:akihiko.odaki@daynix.com > <mailto:akihiko.odaki@daynix.com>> > > <mailto:akihiko.odaki@daynix.com > <mailto:akihiko.odaki@daynix.com> <mailto:akihiko.odaki@daynix.com > <mailto:akihiko.odaki@daynix.com>>> > > > > <mailto:akihiko.odaki@daynix.com > <mailto:akihiko.odaki@daynix.com> > > <mailto:akihiko.odaki@daynix.com > <mailto:akihiko.odaki@daynix.com>> > > > <mailto:akihiko.odaki@daynix.com > <mailto:akihiko.odaki@daynix.com> > > <mailto:akihiko.odaki@daynix.com > <mailto:akihiko.odaki@daynix.com>>>>> wrote: > > > > > > > > On 2023/11/03 18:35, Yuri Benditovich wrote: > > > > > > > > > > > > > > > On Thu, Nov 2, 2023 at 4:56 PM Akihiko Odaki > > > > <akihiko.odaki@daynix.com > <mailto:akihiko.odaki@daynix.com> > > <mailto:akihiko.odaki@daynix.com > <mailto:akihiko.odaki@daynix.com>> <mailto:akihiko.odaki@daynix.com > <mailto:akihiko.odaki@daynix.com> > > <mailto:akihiko.odaki@daynix.com > <mailto:akihiko.odaki@daynix.com>>> > > > <mailto:akihiko.odaki@daynix.com > <mailto:akihiko.odaki@daynix.com> > > <mailto:akihiko.odaki@daynix.com > <mailto:akihiko.odaki@daynix.com>> <mailto:akihiko.odaki@daynix.com > <mailto:akihiko.odaki@daynix.com> > > <mailto:akihiko.odaki@daynix.com > <mailto:akihiko.odaki@daynix.com>>>> > > > > > <mailto:akihiko.odaki@daynix.com > <mailto:akihiko.odaki@daynix.com> > > <mailto:akihiko.odaki@daynix.com > <mailto:akihiko.odaki@daynix.com>> > > > <mailto:akihiko.odaki@daynix.com > <mailto:akihiko.odaki@daynix.com> > > <mailto:akihiko.odaki@daynix.com > <mailto:akihiko.odaki@daynix.com>>> > > > > <mailto:akihiko.odaki@daynix.com > <mailto:akihiko.odaki@daynix.com> > > <mailto:akihiko.odaki@daynix.com > <mailto:akihiko.odaki@daynix.com>> > > > <mailto:akihiko.odaki@daynix.com > <mailto:akihiko.odaki@daynix.com> > > <mailto:akihiko.odaki@daynix.com > <mailto:akihiko.odaki@daynix.com>>>>>> wrote: > > > > > > > > > > On 2023/11/02 19:20, Yuri Benditovich wrote: > > > > > > > > > > > > > > > > > > On Thu, Nov 2, 2023 at 11:33 AM > Michael S. > > Tsirkin > > > > > <mst@redhat.com <mailto:mst@redhat.com> > <mailto:mst@redhat.com <mailto:mst@redhat.com>> > > <mailto:mst@redhat.com <mailto:mst@redhat.com> > <mailto:mst@redhat.com <mailto:mst@redhat.com>>> > > > <mailto:mst@redhat.com <mailto:mst@redhat.com> > <mailto:mst@redhat.com <mailto:mst@redhat.com>> > > <mailto:mst@redhat.com <mailto:mst@redhat.com> > <mailto:mst@redhat.com <mailto:mst@redhat.com>>>> > > > > <mailto:mst@redhat.com <mailto:mst@redhat.com> > <mailto:mst@redhat.com <mailto:mst@redhat.com>> > > <mailto:mst@redhat.com <mailto:mst@redhat.com> > <mailto:mst@redhat.com <mailto:mst@redhat.com>>> > > > <mailto:mst@redhat.com <mailto:mst@redhat.com> > <mailto:mst@redhat.com <mailto:mst@redhat.com>> > > <mailto:mst@redhat.com <mailto:mst@redhat.com> > <mailto:mst@redhat.com <mailto:mst@redhat.com>>>>> > > > > > > <mailto:mst@redhat.com > <mailto:mst@redhat.com> > > <mailto:mst@redhat.com <mailto:mst@redhat.com>> > <mailto:mst@redhat.com <mailto:mst@redhat.com> > <mailto:mst@redhat.com <mailto:mst@redhat.com>>> > > > <mailto:mst@redhat.com <mailto:mst@redhat.com> > <mailto:mst@redhat.com <mailto:mst@redhat.com>> > > <mailto:mst@redhat.com <mailto:mst@redhat.com> > <mailto:mst@redhat.com <mailto:mst@redhat.com>>>> > > > > <mailto:mst@redhat.com <mailto:mst@redhat.com> > <mailto:mst@redhat.com <mailto:mst@redhat.com>> > > <mailto:mst@redhat.com <mailto:mst@redhat.com> > <mailto:mst@redhat.com <mailto:mst@redhat.com>>> > > > <mailto:mst@redhat.com <mailto:mst@redhat.com> > <mailto:mst@redhat.com <mailto:mst@redhat.com>> > > <mailto:mst@redhat.com <mailto:mst@redhat.com> > <mailto:mst@redhat.com <mailto:mst@redhat.com>>>>>>> wrote: > > > > > > > > > > > > On Thu, Nov 02, 2023 at 11:09:27AM > > +0200, Yuri > > > > Benditovich wrote: > > > > > > > Probably we mix two different > patches > > in this > > > > discussion. > > > > > > > Focusing on the patch in the > e-mail > > header: > > > > > > > > > > > > > > IMO it is not acceptable to > fail QEMU run > > > for one > > > > feature > > > > > that we > > > > > > can't make > > > > > > > active when we silently drop > all other > > > features in > > > > such a > > > > > case. > > > > > > > > > > > > If the feature is off by default > then it > > seems more > > > > reasonable > > > > > > and silent masking can be seen as > a bug. > > > > > > Most virtio features are on by > default > > this is > > > why it's > > > > > > reasonable to mask them. > > > > > > > > > > > > > > > > > > If we are talking about RSS: setting it > > initially > > > off is the > > > > > development > > > > > > time decision. > > > > > > When it will be completely stable > there is > > no reason to > > > > keep it > > > > > off by > > > > > > default, so this is more a question > of time > > and of a > > > > readiness of > > > > > libvirt. > > > > > > > > > > It is not ok to make "on" the default; > that will > > > enable RSS > > > > even when > > > > > eBPF steering support is not present and can > > result in > > > > performance > > > > > degradation. > > > > > > > > > > > > > > > Exactly as it is today - with vhost=on the host > > does not > > > suggest RSS > > > > > without eBPF. > > > > > I do not understand what you call "performance > > > degradation", can you > > > > > describe the scenario? > > > > > > > > I was not clear, but I was talking about the > case of > > > vhost=off or peers > > > > other than tap (e.g., user). rss=on employs > in-qemu RSS, > > > which incurs > > > > overheads for such configurations. > > > > > > > > > > > > So, vhost=off OR peers other than tap: > > > > > > > > In the case of peers other than tap (IMO) we're not > > talking about > > > > performance at all. > > > > Backends like "user" (without vnet_hdr) do not > support _many_ > > > > performance-oriented features. > > > > If RSS is somehow "supported" for such backends this is > > rather a > > > > misunderstanding (IMO again). > > > > > > We do not need to ensure good performance when RSS is > enabled > > by the > > > guest for backends without eBPF steering program as > you say. > > In-QEMU > > > RSS > > > is only useful for testing and not meant to improve the > > performance. > > > > > > However, if you set rss=on, QEMU will advertise the > > availability of RSS > > > feature. The guest will have no mean to know if it's > > implemented in a > > > way not performance-wise so it may decide to use the > feature > > to improve > > > the performance, which can result in performance > degradation. > > > Therefore, > > > it's better not to set rss=on for such backends. > > > > > > > > > I still do not understand what is the scenario where you > see or > > suspect > > > the mentioned "performance degradation". > > > We can discuss whether such a problem exists as soon as you > > explain it. > > > > The scenario is that: > > - rss=on, > > - A backend without eBPF steering support is in use, and > > - The guest expects VIRTIO_NET_F_RSS has little overheads as > hardware > > RSS implementations do. > > > > I consider the risk of the performance degradation in such a > situation > > is the reason why virtio-net emits a warning ("Can't load > eBPF RSS - > > fallback to software RSS") when in-QEMU RSS is in use. > > > > > > In a described scenario (vhost=off) I do not see why the performance > > degradation should happen: > > the SW RSS (if activated) will place each packet into proper > queue (even > > if the auto_mq in kernel is not able to do that) and such a way the > > guest will not need to reschedule the packet to proper CPU > > > > The scenario I'm concerned is that the guest has its own packet > steering > mechanism which is feature-wise superior to RSS. For example, Linux has > such a mechanism called RPS, which has some advantages due to its > extensible nature according to: > https://www.kernel.org/doc/html/v6.6/networking/scaling.html#rps-receive-packet-steering <https://www.kernel.org/doc/html/v6.6/networking/scaling.html#rps-receive-packet-steering> > > Such a guest may still prefer hardware RSS if available since hardware > RSS is expected to have less overheads. However, it is not true for > in-qemu RSS, and using in-QEMU RSS instead of the guest-side steering > mechanism may just hide useful features the guest-side steering > mechanism has and result in performance degradation. > > > Note that in terms of per-packet computation for RSS the in-QEMU RSS > does exactly the same operations in native code that the eBPF does in > the emulation. > So, I wouldn't say that SW RSS brings some "performance degradation". > We prefer eBPF as it can serve both vhost and non-vhost setups. I see the eBPF steering program in a different way. tuntap can have several queues, and the assignment of packets is automatically done by tuntap by default. However, the default assignment policy may not be good for all applications, and some may need a different policy. Such an application can still re-assign packets to application-internal queues that reside on different CPUs and that's what 'in-qemu' RSS does. However, that incurs communication overheads across CPUs. The eBPF steering program can eliminate the overheads by allowing the userspace to override the packet assignment policy with eBPF. The eBPF steering program feature of tuntap and its benefit are independent of the vhost usage. In fact, the feature predates the usage in QEMU that combines vhost and eBPF steering program. The initial proposal of the eBPF steering program I found is submitted by Jason and available at: https://lore.kernel.org/lkml/1506500637-13881-1-git-send-email-jasowang@redhat.com/ Jason, please point out if you find something wrong in my understanding with the eBPF steering program feature or something to add.
On Wed, Nov 15, 2023 at 2:09 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote: > > > > On 2023/11/15 7:09, Yuri Benditovich wrote: > > > > > > On Tue, Nov 14, 2023 at 9:03 AM Akihiko Odaki <akihiko.odaki@daynix.com > > <mailto:akihiko.odaki@daynix.com>> wrote: > > > > On 2023/11/14 2:26, Yuri Benditovich wrote: > > > > > > > > > On Mon, Nov 13, 2023 at 2:44 PM Akihiko Odaki > > <akihiko.odaki@daynix.com <mailto:akihiko.odaki@daynix.com> > > > <mailto:akihiko.odaki@daynix.com > > <mailto:akihiko.odaki@daynix.com>>> wrote: > > > > > > On 2023/11/13 20:44, Yuri Benditovich wrote: > > > > > > > > > > > > On Sat, Nov 11, 2023 at 5:28 PM Akihiko Odaki > > > <akihiko.odaki@daynix.com <mailto:akihiko.odaki@daynix.com> > > <mailto:akihiko.odaki@daynix.com <mailto:akihiko.odaki@daynix.com>> > > > > <mailto:akihiko.odaki@daynix.com > > <mailto:akihiko.odaki@daynix.com> > > > <mailto:akihiko.odaki@daynix.com > > <mailto:akihiko.odaki@daynix.com>>>> wrote: > > > > > > > > On 2023/11/03 22:14, Yuri Benditovich wrote: > > > > > > > > > > > > > > > On Fri, Nov 3, 2023 at 11:55 AM Akihiko Odaki > > > > <akihiko.odaki@daynix.com > > <mailto:akihiko.odaki@daynix.com> <mailto:akihiko.odaki@daynix.com > > <mailto:akihiko.odaki@daynix.com>> > > > <mailto:akihiko.odaki@daynix.com > > <mailto:akihiko.odaki@daynix.com> <mailto:akihiko.odaki@daynix.com > > <mailto:akihiko.odaki@daynix.com>>> > > > > > <mailto:akihiko.odaki@daynix.com > > <mailto:akihiko.odaki@daynix.com> > > > <mailto:akihiko.odaki@daynix.com > > <mailto:akihiko.odaki@daynix.com>> > > > > <mailto:akihiko.odaki@daynix.com > > <mailto:akihiko.odaki@daynix.com> > > > <mailto:akihiko.odaki@daynix.com > > <mailto:akihiko.odaki@daynix.com>>>>> wrote: > > > > > > > > > > On 2023/11/03 18:35, Yuri Benditovich wrote: > > > > > > > > > > > > > > > > > > On Thu, Nov 2, 2023 at 4:56 PM Akihiko Odaki > > > > > <akihiko.odaki@daynix.com > > <mailto:akihiko.odaki@daynix.com> > > > <mailto:akihiko.odaki@daynix.com > > <mailto:akihiko.odaki@daynix.com>> <mailto:akihiko.odaki@daynix.com > > <mailto:akihiko.odaki@daynix.com> > > > <mailto:akihiko.odaki@daynix.com > > <mailto:akihiko.odaki@daynix.com>>> > > > > <mailto:akihiko.odaki@daynix.com > > <mailto:akihiko.odaki@daynix.com> > > > <mailto:akihiko.odaki@daynix.com > > <mailto:akihiko.odaki@daynix.com>> <mailto:akihiko.odaki@daynix.com > > <mailto:akihiko.odaki@daynix.com> > > > <mailto:akihiko.odaki@daynix.com > > <mailto:akihiko.odaki@daynix.com>>>> > > > > > > <mailto:akihiko.odaki@daynix.com > > <mailto:akihiko.odaki@daynix.com> > > > <mailto:akihiko.odaki@daynix.com > > <mailto:akihiko.odaki@daynix.com>> > > > > <mailto:akihiko.odaki@daynix.com > > <mailto:akihiko.odaki@daynix.com> > > > <mailto:akihiko.odaki@daynix.com > > <mailto:akihiko.odaki@daynix.com>>> > > > > > <mailto:akihiko.odaki@daynix.com > > <mailto:akihiko.odaki@daynix.com> > > > <mailto:akihiko.odaki@daynix.com > > <mailto:akihiko.odaki@daynix.com>> > > > > <mailto:akihiko.odaki@daynix.com > > <mailto:akihiko.odaki@daynix.com> > > > <mailto:akihiko.odaki@daynix.com > > <mailto:akihiko.odaki@daynix.com>>>>>> wrote: > > > > > > > > > > > > On 2023/11/02 19:20, Yuri Benditovich wrote: > > > > > > > > > > > > > > > > > > > > > On Thu, Nov 2, 2023 at 11:33 AM > > Michael S. > > > Tsirkin > > > > > > <mst@redhat.com <mailto:mst@redhat.com> > > <mailto:mst@redhat.com <mailto:mst@redhat.com>> > > > <mailto:mst@redhat.com <mailto:mst@redhat.com> > > <mailto:mst@redhat.com <mailto:mst@redhat.com>>> > > > > <mailto:mst@redhat.com <mailto:mst@redhat.com> > > <mailto:mst@redhat.com <mailto:mst@redhat.com>> > > > <mailto:mst@redhat.com <mailto:mst@redhat.com> > > <mailto:mst@redhat.com <mailto:mst@redhat.com>>>> > > > > > <mailto:mst@redhat.com <mailto:mst@redhat.com> > > <mailto:mst@redhat.com <mailto:mst@redhat.com>> > > > <mailto:mst@redhat.com <mailto:mst@redhat.com> > > <mailto:mst@redhat.com <mailto:mst@redhat.com>>> > > > > <mailto:mst@redhat.com <mailto:mst@redhat.com> > > <mailto:mst@redhat.com <mailto:mst@redhat.com>> > > > <mailto:mst@redhat.com <mailto:mst@redhat.com> > > <mailto:mst@redhat.com <mailto:mst@redhat.com>>>>> > > > > > > > <mailto:mst@redhat.com > > <mailto:mst@redhat.com> > > > <mailto:mst@redhat.com <mailto:mst@redhat.com>> > > <mailto:mst@redhat.com <mailto:mst@redhat.com> > > <mailto:mst@redhat.com <mailto:mst@redhat.com>>> > > > > <mailto:mst@redhat.com <mailto:mst@redhat.com> > > <mailto:mst@redhat.com <mailto:mst@redhat.com>> > > > <mailto:mst@redhat.com <mailto:mst@redhat.com> > > <mailto:mst@redhat.com <mailto:mst@redhat.com>>>> > > > > > <mailto:mst@redhat.com <mailto:mst@redhat.com> > > <mailto:mst@redhat.com <mailto:mst@redhat.com>> > > > <mailto:mst@redhat.com <mailto:mst@redhat.com> > > <mailto:mst@redhat.com <mailto:mst@redhat.com>>> > > > > <mailto:mst@redhat.com <mailto:mst@redhat.com> > > <mailto:mst@redhat.com <mailto:mst@redhat.com>> > > > <mailto:mst@redhat.com <mailto:mst@redhat.com> > > <mailto:mst@redhat.com <mailto:mst@redhat.com>>>>>>> wrote: > > > > > > > > > > > > > > On Thu, Nov 02, 2023 at 11:09:27AM > > > +0200, Yuri > > > > > Benditovich wrote: > > > > > > > > Probably we mix two different > > patches > > > in this > > > > > discussion. > > > > > > > > Focusing on the patch in the > > e-mail > > > header: > > > > > > > > > > > > > > > > IMO it is not acceptable to > > fail QEMU run > > > > for one > > > > > feature > > > > > > that we > > > > > > > can't make > > > > > > > > active when we silently drop > > all other > > > > features in > > > > > such a > > > > > > case. > > > > > > > > > > > > > > If the feature is off by default > > then it > > > seems more > > > > > reasonable > > > > > > > and silent masking can be seen as > > a bug. > > > > > > > Most virtio features are on by > > default > > > this is > > > > why it's > > > > > > > reasonable to mask them. > > > > > > > > > > > > > > > > > > > > > If we are talking about RSS: setting it > > > initially > > > > off is the > > > > > > development > > > > > > > time decision. > > > > > > > When it will be completely stable > > there is > > > no reason to > > > > > keep it > > > > > > off by > > > > > > > default, so this is more a question > > of time > > > and of a > > > > > readiness of > > > > > > libvirt. > > > > > > > > > > > > It is not ok to make "on" the default; > > that will > > > > enable RSS > > > > > even when > > > > > > eBPF steering support is not present and can > > > result in > > > > > performance > > > > > > degradation. > > > > > > > > > > > > > > > > > > Exactly as it is today - with vhost=on the host > > > does not > > > > suggest RSS > > > > > > without eBPF. > > > > > > I do not understand what you call "performance > > > > degradation", can you > > > > > > describe the scenario? > > > > > > > > > > I was not clear, but I was talking about the > > case of > > > > vhost=off or peers > > > > > other than tap (e.g., user). rss=on employs > > in-qemu RSS, > > > > which incurs > > > > > overheads for such configurations. > > > > > > > > > > > > > > > So, vhost=off OR peers other than tap: > > > > > > > > > > In the case of peers other than tap (IMO) we're not > > > talking about > > > > > performance at all. > > > > > Backends like "user" (without vnet_hdr) do not > > support _many_ > > > > > performance-oriented features. > > > > > If RSS is somehow "supported" for such backends this is > > > rather a > > > > > misunderstanding (IMO again). > > > > > > > > We do not need to ensure good performance when RSS is > > enabled > > > by the > > > > guest for backends without eBPF steering program as > > you say. > > > In-QEMU > > > > RSS > > > > is only useful for testing and not meant to improve the > > > performance. > > > > > > > > However, if you set rss=on, QEMU will advertise the > > > availability of RSS > > > > feature. The guest will have no mean to know if it's > > > implemented in a > > > > way not performance-wise so it may decide to use the > > feature > > > to improve > > > > the performance, which can result in performance > > degradation. > > > > Therefore, > > > > it's better not to set rss=on for such backends. > > > > > > > > > > > > I still do not understand what is the scenario where you > > see or > > > suspect > > > > the mentioned "performance degradation". > > > > We can discuss whether such a problem exists as soon as you > > > explain it. > > > > > > The scenario is that: > > > - rss=on, > > > - A backend without eBPF steering support is in use, and > > > - The guest expects VIRTIO_NET_F_RSS has little overheads as > > hardware > > > RSS implementations do. > > > > > > I consider the risk of the performance degradation in such a > > situation > > > is the reason why virtio-net emits a warning ("Can't load > > eBPF RSS - > > > fallback to software RSS") when in-QEMU RSS is in use. > > > > > > > > > In a described scenario (vhost=off) I do not see why the performance > > > degradation should happen: > > > the SW RSS (if activated) will place each packet into proper > > queue (even > > > if the auto_mq in kernel is not able to do that) and such a way the > > > guest will not need to reschedule the packet to proper CPU > > > > > > > The scenario I'm concerned is that the guest has its own packet > > steering > > mechanism which is feature-wise superior to RSS. For example, Linux has > > such a mechanism called RPS, which has some advantages due to its > > extensible nature according to: > > https://www.kernel.org/doc/html/v6.6/networking/scaling.html#rps-receive-packet-steering <https://www.kernel.org/doc/html/v6.6/networking/scaling.html#rps-receive-packet-steering> > > > > Such a guest may still prefer hardware RSS if available since hardware > > RSS is expected to have less overheads. However, it is not true for > > in-qemu RSS, and using in-QEMU RSS instead of the guest-side steering > > mechanism may just hide useful features the guest-side steering > > mechanism has and result in performance degradation. > > > > > > Note that in terms of per-packet computation for RSS the in-QEMU RSS > > does exactly the same operations in native code that the eBPF does in > > the emulation. > > So, I wouldn't say that SW RSS brings some "performance degradation". > > We prefer eBPF as it can serve both vhost and non-vhost setups. > > I see the eBPF steering program in a different way. > > tuntap can have several queues, Note that RSS can benefit for single queue as well. > and the assignment of packets is > automatically done by tuntap by default. However, the default assignment > policy may not be good for all applications, and some may need a > different policy. Such an application can still re-assign packets to > application-internal queues that reside on different CPUs and that's > what 'in-qemu' RSS does. However, that incurs communication overheads > across CPUs. The eBPF steering program can eliminate the overheads by > allowing the userspace to override the packet assignment policy with eBPF. > Yes, eBPF steering can do more than just RSS. For VM use cases, it just needs to wait for the spec support. For example, spec will support receiving flow director soon, we can leverage the eBPF support for tuntap to do that. > The eBPF steering program feature of tuntap and its benefit are > independent of the vhost usage. In fact, the feature predates the usage > in QEMU that combines vhost and eBPF steering program. I think they should coexist. For example, for many reasons eBPF might not be available on the host. > The initial > proposal of the eBPF steering program I found is submitted by Jason and > available at: > https://lore.kernel.org/lkml/1506500637-13881-1-git-send-email-jasowang@redhat.com/ > > Jason, please point out if you find something wrong in my understanding > with the eBPF steering program feature or something to add. Thanks >
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c index 5d4afd12b2..7bb91617d0 100644 --- a/hw/net/virtio-net.c +++ b/hw/net/virtio-net.c @@ -792,9 +792,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; @@ -3533,6 +3530,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); @@ -3704,53 +3745,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) && + !virtio_net_load_ebpf(n)) { + error_setg(errp, "Can't load eBPF RSS"); + virtio_net_device_unrealize(dev); } - - /* 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_reset(VirtIODevice *vdev)
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.odaki@daynix.com> --- hw/net/virtio-net.c | 97 ++++++++++++++++++++++----------------------- 1 file changed, 48 insertions(+), 49 deletions(-)