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

[Jason Wang]

On 2021/1/19 下午10:53, Yuri Benditovich wrote:
> 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?


It means the configure will fail if libbpf is not installed. Consider 
libbpf is not very common at current stage. I think it's better to make 
it auto or disabled by default.


> > > +    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.


I mean the current can not be used without checking rss_is_loaded(). So 
rss_is_loaded() is somehow a guard like macro.

I personally prefer #ifdef but it's not a must.

Thanks