[bpf-next,5/6] bpf: add bpf_test_tp() kfunc triggering tp and allowing error injection

Commit Message

Andrii Nakryiko March 22, 2024, midnight UTC

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 <[e-mail]>
---
 kernel/bpf/bpf_test.h | 34 ++++++++++++++++++++++++++++++++++
 kernel/bpf/helpers.c  | 13 +++++++++++++
 2 files changed, 47 insertions(+)
 create mode 100644 kernel/bpf/bpf_test.h

Comments

Jiri Olsa March 22, 2024, 1:12 p.m. UTC | #1

On Thu, Mar 21, 2024 at 05:00:40PM -0700, Andrii Nakryiko 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 <[e-mail]>

I'm getting compilation fail with this one:

  INSTALL libsubcmd_headers
  INSTALL libsubcmd_headers
  CALL    scripts/checksyscalls.sh
  CC      kernel/bpf/helpers.o
In file included from kernel/bpf/bpf_test.h:34,
                 from kernel/bpf/helpers.c:30:
./include/trace/define_trace.h:95:42: fatal error: ./bpf_test.h: No such file or directory
   95 | #include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
      |                                          ^

jirka

> ---
>  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);
> +
>  __bpf_kfunc_end_defs();
>  
>  BTF_KFUNCS_START(generic_btf_ids)
> @@ -2625,6 +2637,7 @@ BTF_ID_FLAGS(func, bpf_dynptr_is_null)
>  BTF_ID_FLAGS(func, bpf_dynptr_is_rdonly)
>  BTF_ID_FLAGS(func, bpf_dynptr_size)
>  BTF_ID_FLAGS(func, bpf_dynptr_clone)
> +BTF_ID_FLAGS(func, bpf_test_tp)
>  BTF_KFUNCS_END(common_btf_ids)
>  
>  static const struct btf_kfunc_id_set common_kfunc_set = {
> -- 
> 2.43.0
> 
>

Andrii Nakryiko March 22, 2024, 4:52 p.m. UTC | #2

On Fri, Mar 22, 2024 at 6:12 AM Jiri Olsa <[e-mail]> wrote:
>
> On Thu, Mar 21, 2024 at 05:00:40PM -0700, Andrii Nakryiko 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 <[e-mail]>
>
> I'm getting compilation fail with this one:
>
>   INSTALL libsubcmd_headers
>   INSTALL libsubcmd_headers
>   CALL    scripts/checksyscalls.sh
>   CC      kernel/bpf/helpers.o
> In file included from kernel/bpf/bpf_test.h:34,
>                  from kernel/bpf/helpers.c:30:
> ./include/trace/define_trace.h:95:42: fatal error: ./bpf_test.h: No such file or directory
>    95 | #include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
>       |                                          ^
>

I can't repro even with a clean build. CI also doesn't see any problem ([0]).

Maybe we need to have

#include "trace_probe.h"
#include "trace.h"

Not sure, can you please try?

I tried to do everything just like we do it in
kernel/trace/bpf_trace.c with trace_bpf_trace_printk().


  [0] https://github.com/kernel-patches/bpf/actions/runs/8383349803


> jirka
>
> > ---
> >  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);
> > +
> >  __bpf_kfunc_end_defs();
> >
> >  BTF_KFUNCS_START(generic_btf_ids)
> > @@ -2625,6 +2637,7 @@ BTF_ID_FLAGS(func, bpf_dynptr_is_null)
> >  BTF_ID_FLAGS(func, bpf_dynptr_is_rdonly)
> >  BTF_ID_FLAGS(func, bpf_dynptr_size)
> >  BTF_ID_FLAGS(func, bpf_dynptr_clone)
> > +BTF_ID_FLAGS(func, bpf_test_tp)
> >  BTF_KFUNCS_END(common_btf_ids)
> >
> >  static const struct btf_kfunc_id_set common_kfunc_set = {
> > --
> > 2.43.0
> >
> >

Alexei Starovoitov March 25, 2024, 5:36 p.m. UTC | #3

On Thu, Mar 21, 2024 at 5:01 PM Andrii Nakryiko <[e-mail]> 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 <[e-mail]>
> ---
>  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.

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

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.
All the test things in net/bpf/test_run.c will stay.

Andrii Nakryiko March 25, 2024, 10:19 p.m. UTC | #4

On Mon, Mar 25, 2024 at 10:37 AM Alexei Starovoitov
<[e-mail]> wrote:
>
> On Thu, Mar 21, 2024 at 5:01 PM Andrii Nakryiko <[e-mail]> 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 <[e-mail]>
> > ---
> >  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.

Andrii Nakryiko March 26, 2024, 1:43 a.m. UTC | #5

On Mon, Mar 25, 2024 at 3:19 PM Andrii Nakryiko
<[e-mail]> wrote:
>
> On Mon, Mar 25, 2024 at 10:37 AM Alexei Starovoitov
> <[e-mail]> wrote:
> >
> > On Thu, Mar 21, 2024 at 5:01 PM Andrii Nakryiko <[e-mail]> 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 <[e-mail]>
> > > ---
> > >  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.
>

So I currently have these changes. I moved tp into bpf_test_run.h
(didn't know we have that, this should eliminate the issue that Jiri
saw as well). Moved kfunc into net/bpf/test_run.c and renamed it to
bpf_modify_return_test_tp(), but I kept it referenced in "common
kfuncs", so that raw_tp and others could call it (doesn't seem like I
need extern declaration for it).\

I just didn't touch the test_run functionality. All this works in
bench tool, which is nice.

diff --git a/include/trace/events/bpf_test_run.h
b/include/trace/events/bpf_test_run.h
index 265447e3f71a..0c924d39b7cb 100644
--- a/include/trace/events/bpf_test_run.h
+++ b/include/trace/events/bpf_test_run.h
@@ -7,6 +7,23 @@

 #include <linux/tracepoint.h>

+TRACE_EVENT(bpf_trigger_tp,
+
+       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)
+);
+
 DECLARE_EVENT_CLASS(bpf_test_finish,

        TP_PROTO(int *err),
diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index 67bf9659c447..a85dc684450a 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -2553,14 +2553,6 @@ __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);
-
 __bpf_kfunc_end_defs();

 BTF_KFUNCS_START(generic_btf_ids)
@@ -2637,7 +2629,7 @@ BTF_ID_FLAGS(func, bpf_dynptr_is_null)
 BTF_ID_FLAGS(func, bpf_dynptr_is_rdonly)
 BTF_ID_FLAGS(func, bpf_dynptr_size)
 BTF_ID_FLAGS(func, bpf_dynptr_clone)
-BTF_ID_FLAGS(func, bpf_test_tp)
+BTF_ID_FLAGS(func, bpf_modify_return_test_tp)
 BTF_KFUNCS_END(common_btf_ids)

 static const struct btf_kfunc_id_set common_kfunc_set = {
diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
index 61efeadaff8d..f6aad4ed2ab2 100644
--- a/net/bpf/test_run.c
+++ b/net/bpf/test_run.c
@@ -575,6 +575,13 @@ __bpf_kfunc int bpf_modify_return_test2(int a,
int *b, short c, int d,
        return a + *b + c + d + (long)e + f + g;
 }

+__bpf_kfunc int bpf_modify_return_test_tp(int nonce)
+{
+       trace_bpf_trigger_tp(nonce);
+
+       return nonce;
+}
+
 int noinline bpf_fentry_shadow_test(int a)
 {
        return a + 1;
@@ -622,6 +629,7 @@ __bpf_kfunc_end_defs();
 BTF_KFUNCS_START(bpf_test_modify_return_ids)
 BTF_ID_FLAGS(func, bpf_modify_return_test)
 BTF_ID_FLAGS(func, bpf_modify_return_test2)
+BTF_ID_FLAGS(func, bpf_modify_return_test_tp)
 BTF_ID_FLAGS(func, bpf_fentry_test1, KF_SLEEPABLE)
 BTF_KFUNCS_END(bpf_test_modify_return_ids)

diff --git a/tools/testing/selftests/bpf/progs/trigger_bench.c
b/tools/testing/selftests/bpf/progs/trigger_bench.c
index 914ed625441a..2619ed193c65 100644
--- a/tools/testing/selftests/bpf/progs/trigger_bench.c
+++ b/tools/testing/selftests/bpf/progs/trigger_bench.c
@@ -56,7 +56,7 @@ int trigger_driver(void *ctx)
        return 0;
 }

-extern int bpf_test_tp(int nonce) __ksym __weak;
+extern int bpf_modify_return_test_tp(int nonce) __ksym __weak;

 SEC("?raw_tp")
 int trigger_driver_kfunc(void *ctx)
@@ -64,7 +64,7 @@ int trigger_driver_kfunc(void *ctx)
        int i;

        for (i = 0; i < batch_iters; i++)
-               (void)bpf_test_tp(0); /* attach point for benchmarking */
+               (void)bpf_modify_return_test_tp(0); /* attach point
for benchmarking */

        return 0;
 }
@@ -111,21 +111,21 @@ int bench_trigger_fexit(void *ctx)
        return 0;
 }

-SEC("?fmod_ret/bpf_test_tp")
+SEC("?fmod_ret/bpf_modify_return_test_tp")
 int bench_trigger_fmodret(void *ctx)
 {
        inc_counter();
        return -22;
 }

-SEC("?tp/bpf_test/bpf_test")
+SEC("?tp/bpf_test_run/bpf_trigger_tp")
 int bench_trigger_tp(void *ctx)
 {
        inc_counter();
        return 0;
 }

-SEC("?raw_tp/bpf_test")
+SEC("?raw_tp/bpf_trigger_tp")
 int bench_trigger_rawtp(void *ctx)
 {
        inc_counter();

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

Alexei Starovoitov March 26, 2024, 2:32 a.m. UTC | #6

On Mon, Mar 25, 2024 at 6:43 PM Andrii Nakryiko
<[e-mail]> wrote:
>
> So I currently have these changes. I moved tp into bpf_test_run.h

...

> +__bpf_kfunc int bpf_modify_return_test_tp(int nonce)
> +{
> +       trace_bpf_trigger_tp(nonce);
> +
> +       return nonce;
> +}

Looks good to me.

Jiri Olsa March 26, 2024, 10:57 a.m. UTC | #7

On Mon, Mar 25, 2024 at 06:43:29PM -0700, Andrii Nakryiko wrote:

SNIP

> > 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.
> >
> 
> So I currently have these changes. I moved tp into bpf_test_run.h
> (didn't know we have that, this should eliminate the issue that Jiri
> saw as well). Moved kfunc into net/bpf/test_run.c and renamed it to

sorry I did not get to it yet.. will test the new version

jirka

Patch

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);
+
 __bpf_kfunc_end_defs();
 
 BTF_KFUNCS_START(generic_btf_ids)
@@ -2625,6 +2637,7 @@  BTF_ID_FLAGS(func, bpf_dynptr_is_null)
 BTF_ID_FLAGS(func, bpf_dynptr_is_rdonly)
 BTF_ID_FLAGS(func, bpf_dynptr_size)
 BTF_ID_FLAGS(func, bpf_dynptr_clone)
+BTF_ID_FLAGS(func, bpf_test_tp)
 BTF_KFUNCS_END(common_btf_ids)
 
 static const struct btf_kfunc_id_set common_kfunc_set = {