Re: [PATCH RFC bpf-next v6 6/6] selftests/bpf: add sleepable timer tests

On Mon, 2024-04-08 at 10:09 +0200, Benjamin Tissoires wrote:

bpf_experimental.h and ../bpf_testmod/bpf_testmod_kfunc.h are both including vmlinux.h, which is not compatible with including time.h or bpf_tcp_helpers.h.

So keep sleepable tests in a separate bpf source file.

The first correct test is run twice for convenience:

• first through RUN_TESTS
• then we ensure that the timer was actually executed, in a manual load/attach/run

Signed-off-by: Benjamin Tissoires bentiss@kernel.org

---

Acked-by: Eduard Zingerman eddyz87@gmail.com (With a few nitpicks)

diff --git a/tools/testing/selftests/bpf/prog_tests/timer.c b/tools/testing/selftests/bpf/prog_tests/timer.c index d66687f1ee6a..c6c7c623b31c 100644 --- a/tools/testing/selftests/bpf/prog_tests/timer.c +++ b/tools/testing/selftests/bpf/prog_tests/timer.c

[...]

+void serial_test_sleepable_timer(void) +{

• struct timer_sleepable *timer_sleepable_skel = NULL;
• int err, prog_fd;
• LIBBPF_OPTS(bpf_test_run_opts, topts);
• RUN_TESTS(timer_sleepable);
• /* re-run the success test to check if the timer was actually executed */
• timer_sleepable_skel = timer_sleepable__open_and_load();
• if (!ASSERT_OK_PTR(timer_sleepable_skel, "timer_sleepable_skel_load"))

• return;
  

• err = timer_sleepable__attach(timer_sleepable_skel);
• if (!ASSERT_OK(err, "timer_sleepable_attach"))

• return;
  

Nit: this should call timer_sleepable__destroy();

• prog_fd = bpf_program__fd(timer_sleepable_skel->progs.test_syscall_sleepable);
• err = bpf_prog_test_run_opts(prog_fd, &topts);
• ASSERT_OK(err, "test_run");
• ASSERT_EQ(topts.retval, 0, "test_run");
• usleep(50); /* 10 usecs should be enough, but give it extra */
• ASSERT_EQ(timer_sleepable_skel->bss->ok_sleepable, 1, "ok_sleepable");

Nit: same as above.

+} diff --git a/tools/testing/selftests/bpf/progs/timer_sleepable.c b/tools/testing/selftests/bpf/progs/timer_sleepable.c new file mode 100644 index 000000000000..fc7829d8b6c4 --- /dev/null +++ b/tools/testing/selftests/bpf/progs/timer_sleepable.c

[...]

+SEC("tc") +/* check that calling bpf_timer_start() with BPF_F_TIMER_SLEEPABLE on a sleepable

• • callback works
• */

+__retval(0) +long test_call_sleepable(void *ctx) +{

• int key = 0;
• struct bpf_timer *timer;
• if (ok_sleepable & 1)

• return -1;
  

• timer = bpf_map_lookup_elem(&timer_map, &key);
• if (timer) {

• if (bpf_timer_init(timer, &timer_map, CLOCK_MONOTONIC | BPF_F_TIMER_SLEEPABLE) != 0)
  

• 	return -2;
  

• bpf_timer_set_sleepable_cb(timer, timer_cb_sleepable);
  

• if (bpf_timer_start(timer, 0, 0))
  

• 	return -3;
  

• } else {

• return -4;
  

• }
• return 0;

+}

+SEC("syscall") +/* check that calling bpf_timer_start() with BPF_F_TIMER_SLEEPABLE on a sleepable

• • callback works.
• */

+__retval(0) +long test_syscall_sleepable(void *ctx) +{

Nit: the body of this function is the same as in test_call_sleepable(), maybe factor it out as an auxiliary static function? (also, should these tests use different 'key' ?)

• int key = 0;
• struct bpf_timer *timer;
• if (ok_sleepable & 1)

• return -1;
  

• timer = bpf_map_lookup_elem(&timer_map, &key);
• if (timer) {

• if (bpf_timer_init(timer, &timer_map, CLOCK_MONOTONIC | BPF_F_TIMER_SLEEPABLE) != 0)
  

• 	return -2;
  

• bpf_timer_set_sleepable_cb(timer, timer_cb_sleepable);
  

• if (bpf_timer_start(timer, 0, 0))
  

• 	return -3;
  

• } else {

• return -4;
  

• }
• return 0;

+}

[...]

+SEC("tc") +/* check that calling bpf_timer_start() with a delay on a sleepable

• • callback is returning -EINVAL
• */

+__retval(-22) +long test_call_sleepable_delay(void *ctx) +{

• int key = 2;
• struct bpf_timer *timer;
• timer = bpf_map_lookup_elem(&timer_map, &key);
• if (!timer)

• return 1;
  

• if (bpf_timer_init(timer, &timer_map, CLOCK_MONOTONIC | BPF_F_TIMER_SLEEPABLE))

• return 2;
  

• if (bpf_timer_set_sleepable_cb(timer, timer_cb_sleepable))

• return 3;
  

• return bpf_timer_start(timer, 1, 0);

Q: should verifier statically check that 3rd parameter is zero for sleepable timers? (same question for call to bpf_timer_set_sleepable_cb() with non-sleepable map)

[...]