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

[Akihiko Odaki <akihiko.odaki@daynix.com>]

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