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

[Jason Wang <jasowang@redhat.com>]

在 2023/10/27 15:54, Akihiko Odaki 写道:
> On 2023/10/27 16:07, Jason Wang wrote:
>> On Tue, Oct 17, 2023 at 12:10 PM Akihiko Odaki 
>> <akihiko.odaki@daynix.com> wrote:
>>>
>>> vhost requires eBPF for RSS. Even when eBPF is not available, 
>>> virtio-net
>>> reported RSS availability, and raised a warning only after the
>>> guest requested RSS, and the guest could not know that RSS is not
>>> available.
>>>
>>> Check RSS availability during device realization and return an error
>>> if RSS is requested but not available. Assert RSS availability when
>>> the guest actually requests the feature.
>>>
>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>>> ---
>>>   ebpf/ebpf_rss.h      |   2 +-
>>>   ebpf/ebpf_rss-stub.c |   4 +-
>>>   ebpf/ebpf_rss.c      |  68 +++++++++-----------------
>>>   hw/net/virtio-net.c  | 114 
>>> +++++++++++++++++++++----------------------
>>>   4 files changed, 82 insertions(+), 106 deletions(-)
>>>
>>> diff --git a/ebpf/ebpf_rss.h b/ebpf/ebpf_rss.h
>>> index bf3f2572c7..1128173572 100644
>>> --- a/ebpf/ebpf_rss.h
>>> +++ b/ebpf/ebpf_rss.h
>>> @@ -36,7 +36,7 @@ bool ebpf_rss_is_loaded(struct EBPFRSSContext *ctx);
>>>
>>>   bool ebpf_rss_load(struct EBPFRSSContext *ctx);
>>>
>>> -bool ebpf_rss_set_all(struct EBPFRSSContext *ctx, struct 
>>> EBPFRSSConfig *config,
>>> +void ebpf_rss_set_all(struct EBPFRSSContext *ctx, struct 
>>> EBPFRSSConfig *config,
>>>                         uint16_t *indirections_table, uint8_t 
>>> *toeplitz_key);
>>>
>>>   void ebpf_rss_unload(struct EBPFRSSContext *ctx);
>>> diff --git a/ebpf/ebpf_rss-stub.c b/ebpf/ebpf_rss-stub.c
>>> index e71e229190..525b358597 100644
>>> --- a/ebpf/ebpf_rss-stub.c
>>> +++ b/ebpf/ebpf_rss-stub.c
>>> @@ -28,10 +28,10 @@ bool ebpf_rss_load(struct EBPFRSSContext *ctx)
>>>       return false;
>>>   }
>>>
>>> -bool ebpf_rss_set_all(struct EBPFRSSContext *ctx, struct 
>>> EBPFRSSConfig *config,
>>> +void ebpf_rss_set_all(struct EBPFRSSContext *ctx, struct 
>>> EBPFRSSConfig *config,
>>>                         uint16_t *indirections_table, uint8_t 
>>> *toeplitz_key)
>>>   {
>>> -    return false;
>>> +    g_assert_not_reached();
>>>   }
>>>
>>>   void ebpf_rss_unload(struct EBPFRSSContext *ctx)
>>> diff --git a/ebpf/ebpf_rss.c b/ebpf/ebpf_rss.c
>>> index cee658c158..6cdf82d059 100644
>>> --- a/ebpf/ebpf_rss.c
>>> +++ b/ebpf/ebpf_rss.c
>>> @@ -74,42 +74,32 @@ error:
>>>       return false;
>>>   }
>>>
>>> -static bool ebpf_rss_set_config(struct EBPFRSSContext *ctx,
>>> +static void ebpf_rss_set_config(struct EBPFRSSContext *ctx,
>>>                                   struct EBPFRSSConfig *config)
>>>   {
>>>       uint32_t map_key = 0;
>>>
>>> -    if (!ebpf_rss_is_loaded(ctx)) {
>>> -        return false;
>>> -    }
>>> -    if (bpf_map_update_elem(ctx->map_configuration,
>>> -                            &map_key, config, 0) < 0) {
>>> -        return false;
>>> -    }
>>> -    return true;
>>> +    assert(ebpf_rss_is_loaded(ctx));
>>> +    assert(!bpf_map_update_elem(ctx->map_configuration, &map_key, 
>>> config, 0));
>>
>> Guest trigger-rable assertion should be avoided as much as possible.
>
> These assertions should never be triggered by the guest as the feature 
> availability is checked at the device realization. Are these 
> assertions still problematic?


I still think it's better to avoid assertions as code could be changed 
by various people so it may end up with such possibility in the future.

Thanks


>
> Regards,
> Akihiko Odaki
>