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

[Jason Wang <jasowang@redhat.com>]

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