Message-ID: <CAEf4Bzazst1rBi4=LuP6_FnPXCRYBNFEtDnK3UVBj6Eo6xFNtQ@mail.gmail.com>
Date:   Tue, 13 Apr 2021 18:04:05 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Jiri Olsa <jolsa@...nel.org>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andriin@...com>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...omium.org>, Daniel Xu <dxu@...uu.xyz>,
        Steven Rostedt <rostedt@...dmis.org>,
        Jesper Brouer <jbrouer@...hat.com>,
        Toke Høiland-Jørgensen <toke@...hat.com>,
        Viktor Malik <vmalik@...hat.com>
Subject: Re: [PATCHv2 RFC bpf-next 0/7] bpf: Add support for ftrace probe

On Tue, Apr 13, 2021 at 7:57 AM Jiri Olsa <jolsa@...nel.org> wrote:
>
> hi,
> sending another attempt on speeding up load of multiple probes
> for bpftrace and possibly other tools (first post in [1]).
>
> This patchset adds support to attach bpf program directly to
> ftrace probe as suggested by Steven and it speeds up loading
> for bpftrace commands like:
>
>    # bpftrace -e 'kfunc:_raw_spin* { @[probe] = count(); }'
>    # bpftrace -e 'kfunc:ksys_* { @[probe] = count(); }'
>
> Using ftrace with single bpf program for attachment to multiple
> functions is much faster than current approach, where we need to
> load and attach program for each probe function.
>

Ok, so first of all, I think it's super important to allow fast
attachment of a single BPF program to multiple kernel functions (I
call it mass-attachment). I've been recently prototyping a tool
(retsnoop, [0]) that allows attaching fentry/fexit to multiple
functions, and not having this feature turned into lots of extra code
and slow startup/teardown speeds. So we should definitely fix that.

But I think the approach you've taken is not the best one, even though
it's a good starting point for discussion.

First, you are saying function return attachment support is missing,
but is not needed so far. I actually think that without func return
the whole feature is extremely limiting. Not being able to measure
function latency  by tracking enter/exit events is crippling for tons
of useful applications. So I think this should go with both at the
same time.

But guess what, we already have a good BPF infra (BPF trampoline and
fexit programs) that supports func exit tracing. Additionally, it
supports the ability to read input arguments *on function exit*, which
is something that kretprobe doesn't support and which is often a very
limiting restriction, necessitating complicated logic to trace
function entry just to store input arguments. It's a killer feature
and one that makes fexit so much more useful than kretprobe.

The only problem is that currently we have a 1:1:1 relationship
between BPF trampoline, BPF program, and kernel function. I think we
should allow to have a single BPF program, using a single BPF
trampoline, but being able to attach to multiple kernel functions
(1:1:N). This will allow to validate BPF program once, allocate only
one dedicated BPF trampoline, and then (with appropriate attach API)
attach them in a batch mode.

We'll probably have to abandon direct memory read for input arguments,
but for these mass-attachment scenarios that's rarely needed at all.
Just allowing to read input args as u64 and providing traced function
IP would be enough to do a lot. BPF trampoline can just
unconditionally save the first 6 arguments, similarly how we do it
today for a specific BTF function, just always 6.

As for attachment, dedicating an entire new FD for storing functions
seems like an overkill. I think BPF_LINK_CREATE is the right place to
do this, providing an array of BTF IDs to identify all functions to be
attached to. It's both simple and efficient.

We'll get to libbpf APIs and those pseudo-regexp usage a bit later, I
don't think we need to discuss that at this stage yet :)

So, WDYT about BPF trampoline-based generic fentry/fexit with mass-attach API?

  [0] https://github.com/anakryiko/retsnoop

> Also available in
>   git://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
>   bpf/ftrace
>
> thanks,
> jirka
>
>
> [1] https://lore.kernel.org/bpf/20201022082138.2322434-1-jolsa@kernel.org/
> ---
> Jiri Olsa (7):
>       bpf: Move bpf_prog_start/end functions to generic place
>       bpf: Add bpf_functions object
>       bpf: Add support to attach program to ftrace probe
>       libbpf: Add btf__find_by_pattern_kind function
>       libbpf: Add support to load and attach ftrace probe
>       selftests/bpf: Add ftrace probe to fentry test
>       selftests/bpf: Add ftrace probe test
>
>  include/uapi/linux/bpf.h                             |   8 ++++
>  kernel/bpf/syscall.c                                 | 381 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  kernel/bpf/trampoline.c                              |  97 ---------------------------------------
>  kernel/bpf/verifier.c                                |  27 +++++++++++
>  net/bpf/test_run.c                                   |   1 +
>  tools/include/uapi/linux/bpf.h                       |   8 ++++
>  tools/lib/bpf/bpf.c                                  |  12 +++++
>  tools/lib/bpf/bpf.h                                  |   5 +-
>  tools/lib/bpf/btf.c                                  |  67 +++++++++++++++++++++++++++
>  tools/lib/bpf/btf.h                                  |   3 ++
>  tools/lib/bpf/libbpf.c                               |  74 ++++++++++++++++++++++++++++++
>  tools/lib/bpf/libbpf.map                             |   1 +
>  tools/testing/selftests/bpf/prog_tests/fentry_test.c |   5 +-
>  tools/testing/selftests/bpf/prog_tests/ftrace_test.c |  48 +++++++++++++++++++
>  tools/testing/selftests/bpf/progs/fentry_test.c      |  16 +++++++
>  tools/testing/selftests/bpf/progs/ftrace_test.c      |  17 +++++++
>  16 files changed, 671 insertions(+), 99 deletions(-)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/ftrace_test.c
>  create mode 100644 tools/testing/selftests/bpf/progs/ftrace_test.c
>

Powered by blists - more mailing lists