Re: [PATCH v6 11/21] virtio-net: Return an error when vhost cannot enable RSS

[Jason Wang]

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