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

[Jason Wang]

在 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