On Mon, Mar 25, 2024 at 10:37 AM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> On Thu, Mar 21, 2024 at 5:01 PM Andrii Nakryiko <andrii@xxxxxxxxxx> wrote:
> >
> > Add a simple bpf_test_tp() kfunc, available to all program types, that
> > is useful for various testing and benchmarking scenarios, as it allows
> > to trigger most tracing BPF program types from BPF side, allowing to do
> > complex testing and benchmarking scenarios.
> >
> > Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> > ---
> > kernel/bpf/bpf_test.h | 34 ++++++++++++++++++++++++++++++++++
> > kernel/bpf/helpers.c | 13 +++++++++++++
> > 2 files changed, 47 insertions(+)
> > create mode 100644 kernel/bpf/bpf_test.h
> >
> > diff --git a/kernel/bpf/bpf_test.h b/kernel/bpf/bpf_test.h
> > new file mode 100644
> > index 000000000000..c779927d3574
> > --- /dev/null
> > +++ b/kernel/bpf/bpf_test.h
> > @@ -0,0 +1,34 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#undef TRACE_SYSTEM
> > +#define TRACE_SYSTEM bpf_test
> > +
> > +#if !defined(_TRACE_BPF_TEST_H) || defined(TRACE_HEADER_MULTI_READ)
> > +
> > +#define _TRACE_BPF_TEST_H
> > +
> > +#include <linux/tracepoint.h>
> > +
> > +TRACE_EVENT(bpf_test,
> > +
> > + TP_PROTO(int nonce),
> > +
> > + TP_ARGS(nonce),
> > +
> > + TP_STRUCT__entry(
> > + __field(int, nonce)
> > + ),
> > +
> > + TP_fast_assign(
> > + __entry->nonce = nonce;
> > + ),
> > +
> > + TP_printk("nonce %d", __entry->nonce)
> > +);
> > +
> > +#endif /* _TRACE_BPF_TEST_H */
> > +
> > +#undef TRACE_INCLUDE_PATH
> > +#define TRACE_INCLUDE_PATH .
> > +#define TRACE_INCLUDE_FILE bpf_test
> > +
> > +#include <trace/define_trace.h>
> > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> > index 9234174ccb21..67bf9659c447 100644
> > --- a/kernel/bpf/helpers.c
> > +++ b/kernel/bpf/helpers.c
> > @@ -26,6 +26,10 @@
> >
> > #include "../../lib/kstrtox.h"
> >
> > +#define CREATE_TRACE_POINTS
> > +#include "bpf_test.h"
> > +
> > +
> > /* If kernel subsystem is allowing eBPF programs to call this function,
> > * inside its own verifier_ops->get_func_proto() callback it should return
> > * bpf_map_lookup_elem_proto, so that verifier can properly check the arguments
> > @@ -2549,6 +2553,14 @@ __bpf_kfunc void bpf_throw(u64 cookie)
> > WARN(1, "A call to BPF exception callback should never return\n");
> > }
> >
> > +__bpf_kfunc int bpf_test_tp(int nonce)
> > +{
> > + trace_bpf_test(nonce);
> > +
> > + return nonce;
> > +}
> > +ALLOW_ERROR_INJECTION(bpf_test_tp, ERRNO);
>
> There is a better way with register_btf_fmodret_id_set().
> Error injection can be disabled in config.
ok, didn't know about it
>
> Also we have two such test funcs already:
> bpf_modify_return_test*
> and they can be exercised by bpf_prog_test_run_tracing.
> Let's make it recognize 'repeat' argument and call
> bpf_modify_return_test or 3rd such func multiple times ?
I'll add bpf_modify_return_test_tp() to not touch all the tests using
bpf_modify_return_test() and bpf_modify_return_test2(), if that's ok.
Existing ones expect some memory pointer, dereference it, etc, it
seems cleaner to have a dedicated tp-triggering one for this.
> Exercise of test tracepoint can be there as well.
> Asking bpf prog to call a kfunc to call a tracepoint
> looks like extra hop.
> Existing test_run facility should be able to accommodate.
You mean if I add bpf_modify_return_test_tp() above, I should pass an
argument of how many times that tracepoint should be triggered? Or you
mean to use test_run's repeat argument to trigger "driver program" N
times, and the driver program would just call
bpf_modify_return_test_tp() once? If the latter, I'm not sure it's the
same as calling the driver program once and doing a loop inside, as
we'll measure more of calling driver program overhead (N times vs 1
time right now, per each N tp/fmod_ret calls).
(But tbh, not having to use test_run's repeat functionality is a
benefit, IMO, we can have more flexible counting/timing code and
whatever else we might need, I'm not sure why using test_run's repeat
is advantageous here)
Not sure what you are trying to optimize for here, please clarify.
>
> I still don't get it how modifying the kernel is better than
> adding all that to a kernel module.
> When you want to run bench tool on a different server
> you either need to build/copy bpf_testmod or
> build/copy the whole kernel.
> But fine.
I optimize for having a single pre-built bench binary that one can scp
to multiple different (potentially custom built) kernels and run
benchmarks. Of course on old kernels where we don't yet have this new
bpf_modify_return_test_tp() kfunc some of benchmarks won't work
(raw_tp/fmod_ret/tp only), but that's fine, as newer kernels get
deployed we'll eventually get them even in production. kprobe/fentry
benchmark will work as is (which is why I'm not switching them to new
kfunc, so we can run them on a wider variety of kernels).
> All the test things in net/bpf/test_run.c will stay.
