On Wed, Nov 15, 2023 at 9:18 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> The patch a few patches from this one changes logic for callbacks
> handling. While previously callbacks were verified as a single
> function call, new scheme takes into account that callbacks could be
> executed unknown number of times.
>
> This has dire implications for bpf_loop_bench:
>
> SEC("fentry/" SYS_PREFIX "sys_getpgid")
> int benchmark(void *ctx)
> {
> for (int i = 0; i < 1000; i++) {
> bpf_loop(nr_loops, empty_callback, NULL, 0);
> __sync_add_and_fetch(&hits, nr_loops);
> }
> return 0;
> }
>
> W/o callbacks change for verifier it merely represents 1000 calls to
> empty_callback(). However, with callbacks change things become
> exponential:
> - i=0: state exploring empty_callback is scheduled with i=0 (a);
> - i=1: state exploring empty_callback is scheduled with i=1;
> ...
> - i=999: state exploring empty_callback is scheduled with i=999;
> - state (a) is popped from stack;
> - i=1: state exploring empty_callback is scheduled with i=1;
> ...
would this still happen if you use an obfuscated zero initializer for i?
int zero = 0; /* global var */
...
for (i = zero; i < 1000; i++) {
...
}
>
> Avoid this issue by rewriting outer loop as bpf_loop().
> Unfortunately, this adds a function call to a loop at runtime, which
> negatively affects performance:
>
> throughput latency
> before: 149.919 ± 0.168 M ops/s, 6.670 ns/op
> after : 137.040 ± 0.187 M ops/s, 7.297 ns/op
>
> Signed-off-by: Eduard Zingerman <eddyz87@xxxxxxxxx>
> ---
> tools/testing/selftests/bpf/progs/bpf_loop_bench.c | 13 ++++++++-----
> 1 file changed, 8 insertions(+), 5 deletions(-)
>
Either way, it doesn't seem like a big deal to me.
Acked-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> diff --git a/tools/testing/selftests/bpf/progs/bpf_loop_bench.c b/tools/testing/selftests/bpf/progs/bpf_loop_bench.c
> index 4ce76eb064c4..d461746fd3c1 100644
> --- a/tools/testing/selftests/bpf/progs/bpf_loop_bench.c
> +++ b/tools/testing/selftests/bpf/progs/bpf_loop_bench.c
> @@ -15,13 +15,16 @@ static int empty_callback(__u32 index, void *data)
> return 0;
> }
>
> +static int outer_loop(__u32 index, void *data)
> +{
> + bpf_loop(nr_loops, empty_callback, NULL, 0);
> + __sync_add_and_fetch(&hits, nr_loops);
> + return 0;
> +}
> +
> SEC("fentry/" SYS_PREFIX "sys_getpgid")
> int benchmark(void *ctx)
> {
> - for (int i = 0; i < 1000; i++) {
> - bpf_loop(nr_loops, empty_callback, NULL, 0);
> -
> - __sync_add_and_fetch(&hits, nr_loops);
> - }
> + bpf_loop(1000, outer_loop, NULL, 0);
> return 0;
> }
> --
> 2.42.0
>
