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

[Yuri Benditovich]

On Mon, Jan 25, 2021 at 11:03 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
> 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

Yes, this is a mistake. These 3 lines are to be removed.
If libbpf is not installed, this should not fail the build.

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