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

[Jason Wang]

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?


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

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


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

Thanks