Re: [RFC PATCH v3 4/6] ebpf: Added eBPF RSS loader.

[Yuri Benditovich <yuri.benditovich@daynix.com>]

On Fri, Jan 15, 2021 at 9:02 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 2021/1/15 上午5:16, Andrew Melnychenko wrote:
> > From: Andrew <andrew@daynix.com>
> >
> > Added function that loads RSS eBPF program.
> > Added stub functions for RSS eBPF loader.
> > Added meson and configuration options.
> >
> > By default, eBPF feature enabled if libbpf is present in the build system.
> > libbpf checked in configuration shell script and meson script.
> >
> > Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>
> > Signed-off-by: Andrew Melnychenko <andrew@daynix.com>
> > ---
> >   configure               |  33 ++++
> >   ebpf/ebpf_rss-stub.c    |  40 ++++
> >   ebpf/ebpf_rss.c         | 165 +++++++++++++++++
> >   ebpf/ebpf_rss.h         |  44 +++++
> >   ebpf/meson.build        |   1 +
> >   ebpf/rss.bpf.skeleton.h | 397 ++++++++++++++++++++++++++++++++++++++++
> >   ebpf/trace-events       |   4 +
> >   ebpf/trace.h            |   2 +
> >   meson.build             |  13 ++
> >   9 files changed, 699 insertions(+)
> >   create mode 100644 ebpf/ebpf_rss-stub.c
> >   create mode 100644 ebpf/ebpf_rss.c
> >   create mode 100644 ebpf/ebpf_rss.h
> >   create mode 100644 ebpf/meson.build
> >   create mode 100644 ebpf/rss.bpf.skeleton.h
> >   create mode 100644 ebpf/trace-events
> >   create mode 100644 ebpf/trace.h
> >
> > diff --git a/configure b/configure
> > index 5860bdb77b..9d18e941f5 100755
> > --- a/configure
> > +++ b/configure
> > @@ -342,6 +342,7 @@ vhost_vsock="$default_feature"
> >   vhost_user="no"
> >   vhost_user_blk_server="auto"
> >   vhost_user_fs="$default_feature"
> > +bpf=""
> >   kvm="auto"
> >   hax="auto"
> >   hvf="auto"
> > @@ -1236,6 +1237,10 @@ for opt do
> >     ;;
> >     --enable-membarrier) membarrier="yes"
> >     ;;
> > +  --disable-bpf) bpf="no"
> > +  ;;
> > +  --enable-bpf) bpf="yes"
> > +  ;;
> >     --disable-blobs) blobs="false"
> >     ;;
> >     --with-pkgversion=*) pkgversion="$optarg"
> > @@ -1845,6 +1850,7 @@ disabled with --disable-FEATURE, default is enabled if available
> >     vhost-user      vhost-user backend support
> >     vhost-user-blk-server    vhost-user-blk server support
> >     vhost-vdpa      vhost-vdpa kernel backend support
> > +  bpf             BPF kernel support
> >     spice           spice
> >     rbd             rados block device (rbd)
> >     libiscsi        iscsi support
> > @@ -5057,6 +5063,30 @@ else
> >       membarrier=no
> >   fi
> >
> > +##########################################
> > +# check for usable bpf system call
> > +if test "$bpf" = ""; then
>
>
> This implies the bpf is enabled by default?

Yes, assuming libbpf-devel present and bpf system call defined.

Any problem with it?

>
>
> > +    have_bpf=no
> > +    if test "$linux" = "yes" -a "$bigendian" != "yes"; then
> > +        cat > $TMPC << EOF
> > +    #include <stdlib.h>
> > +    #include <bpf/libbpf.h>
> > +    int main(void) {
> > +        struct bpf_object *obj = NULL;
> > +        bpf_object__load(obj);
> > +        exit(0);
> > +    }
> > +EOF
> > +        if compile_prog "" "-lbpf" ; then
> > +            have_bpf=yes
> > +            bpf=yes
> > +        fi
> > +    fi
> > +    if test "$have_bpf" = "no"; then
> > +      feature_not_found "bpf" "the libbpf is not available"
> > +    fi
> > +fi
> > +
> >   ##########################################
> >   # check if rtnetlink.h exists and is useful
> >   have_rtnetlink=no
> > @@ -5905,6 +5935,9 @@ fi
> >   if test "$membarrier" = "yes" ; then
> >     echo "CONFIG_MEMBARRIER=y" >> $config_host_mak
> >   fi
> > +if test "$bpf" = "yes" -a "$bigendian" != "yes" -a "$linux" = "yes" ; then
> > +  echo "CONFIG_EBPF=y" >> $config_host_mak
> > +fi
> >   if test "$signalfd" = "yes" ; then
> >     echo "CONFIG_SIGNALFD=y" >> $config_host_mak
> >   fi
> > diff --git a/ebpf/ebpf_rss-stub.c b/ebpf/ebpf_rss-stub.c
> > new file mode 100644
> > index 0000000000..e71e229190
> > --- /dev/null
> > +++ b/ebpf/ebpf_rss-stub.c
> > @@ -0,0 +1,40 @@
> > +/*
> > + * eBPF RSS stub file
> > + *
> > + * Developed by Daynix Computing LTD (http://www.daynix.com)
> > + *
> > + * Authors:
> > + *  Yuri Benditovich <yuri.benditovich@daynix.com>
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2.  See
> > + * the COPYING file in the top-level directory.
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "ebpf/ebpf_rss.h"
>
>
> I wonder why not simply use #ifdef #else to exclude the rss functions.

Just to make the reading easier.

>
> If I read code correctly, this stub requires rss_is_loaded called before
> ebpf_rss_set_all(), so actually ebpf_rss_is_loaded serve the same as

Can you please get into details?
I think the stub does not require it, it just converts all the
ebpf_rss calls to nop at link time.
If I miss something please let us know, we'll fix it in v4.

>
>
> > +
> > +void ebpf_rss_init(struct EBPFRSSContext *ctx)
> > +{
> > +
> > +}
> > +
> > +bool ebpf_rss_is_loaded(struct EBPFRSSContext *ctx)
> > +{
> > +    return false;
> > +}
> > +
> > +bool ebpf_rss_load(struct EBPFRSSContext *ctx)
> > +{
> > +    return false;
> > +}
> > +
> > +bool ebpf_rss_set_all(struct EBPFRSSContext *ctx, struct EBPFRSSConfig *config,
> > +                      uint16_t *indirections_table, uint8_t *toeplitz_key)
> > +{
> > +    return false;
> > +}
> > +
> > +void ebpf_rss_unload(struct EBPFRSSContext *ctx)
> > +{
> > +
> > +}
> > diff --git a/ebpf/ebpf_rss.c b/ebpf/ebpf_rss.c
> > new file mode 100644
> > index 0000000000..f7f7102da5
> > --- /dev/null
> > +++ b/ebpf/ebpf_rss.c
> > @@ -0,0 +1,165 @@
> > +/*
> > + * eBPF RSS loader
> > + *
> > + * Developed by Daynix Computing LTD (http://www.daynix.com)
> > + *
> > + * Authors:
> > + *  Andrew Melnychenko <andrew@daynix.com>
> > + *  Yuri Benditovich <yuri.benditovich@daynix.com>
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2.  See
> > + * the COPYING file in the top-level directory.
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "qemu/error-report.h"
> > +
> > +#include <bpf/libbpf.h>
> > +#include <bpf/bpf.h>
> > +
> > +#include "hw/virtio/virtio-net.h" /* VIRTIO_NET_RSS_MAX_TABLE_LEN */
> > +
> > +#include "ebpf/ebpf_rss.h"
> > +#include "ebpf/rss.bpf.skeleton.h"
> > +#include "trace.h"
> > +
> > +void ebpf_rss_init(struct EBPFRSSContext *ctx)
> > +{
> > +    if (ctx != NULL) {
> > +        ctx->obj = NULL;
> > +    }
> > +}
> > +
> > +bool ebpf_rss_is_loaded(struct EBPFRSSContext *ctx)
> > +{
> > +    return ctx != NULL && ctx->obj != NULL;
> > +}
> > +
> > +bool ebpf_rss_load(struct EBPFRSSContext *ctx)
> > +{
> > +    struct rss_bpf *rss_bpf_ctx;
> > +
> > +    if (ctx == NULL) {
> > +        return false;
> > +    }
> > +
> > +    rss_bpf_ctx = rss_bpf__open();
> > +    if (rss_bpf_ctx == NULL) {
> > +        trace_ebpf_error("eBPF RSS", "can not open eBPF RSS object");
> > +        goto l_issue;
>
>
> Nit: it's not easy to infer what did "l" mean, it's probably better to
> use "error".

ok, will be renamed, 'l' means 'load' probably

>
> Thanks
>