Received-SPF: pass (google.com: domain of linux-kernel+bounces-135994-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <345cddbdfe8e91dc6395331c838329d02d519863.camel@gmail.com>
Subject: Re: [PATCH RFC bpf-next v6 6/6] selftests/bpf: add sleepable timer
 tests
From: Eduard Zingerman <eddyz87@gmail.com>
To: Benjamin Tissoires <bentiss@kernel.org>, Alexei Starovoitov
 <ast@kernel.org>,  Daniel Borkmann <daniel@iogearbox.net>, Andrii Nakryiko
 <andrii@kernel.org>, Martin KaFai Lau <martin.lau@linux.dev>,  Song Liu
 <song@kernel.org>, Yonghong Song <yonghong.song@linux.dev>, John Fastabend
 <john.fastabend@gmail.com>, KP Singh <kpsingh@kernel.org>, Stanislav
 Fomichev <sdf@google.com>, Hao Luo <haoluo@google.com>, Jiri Olsa
 <jolsa@kernel.org>,  Mykola Lysenko <mykolal@fb.com>, Shuah Khan
 <shuah@kernel.org>
Cc: bpf@vger.kernel.org, linux-kernel@vger.kernel.org, 
	linux-kselftest@vger.kernel.org
Date: Tue, 09 Apr 2024 02:00:57 +0300
In-Reply-To: <20240408-hid-bpf-sleepable-v6-6-0499ddd91b94@kernel.org>
References: <20240408-hid-bpf-sleepable-v6-0-0499ddd91b94@kernel.org>
	 <20240408-hid-bpf-sleepable-v6-6-0499ddd91b94@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.52.0 
Precedence: bulk
MIME-Version: 1.0

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.
>=20
> So keep sleepable tests in a separate bpf source file.
>=20
> 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
>=20
> Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
>=20
> ---

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

> diff --git a/tools/testing/selftests/bpf/prog_tests/timer.c b/tools/testi=
ng/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 =3D 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 =3D timer_sleepable__open_and_load();
> +	if (!ASSERT_OK_PTR(timer_sleepable_skel, "timer_sleepable_skel_load"))
> +		return;
> +
> +	err =3D timer_sleepable__attach(timer_sleepable_skel);
> +	if (!ASSERT_OK(err, "timer_sleepable_attach"))
> +		return;

Nit: this should call timer_sleepable__destroy();

> +
> +	prog_fd =3D bpf_program__fd(timer_sleepable_skel->progs.test_syscall_sl=
eepable);
> +	err =3D 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 =3D 0;
> +	struct bpf_timer *timer;
> +
> +	if (ok_sleepable & 1)
> +		return -1;
> +
> +	timer =3D bpf_map_lookup_elem(&timer_map, &key);
> +	if (timer) {
> +		if (bpf_timer_init(timer, &timer_map, CLOCK_MONOTONIC | BPF_F_TIMER_SL=
EEPABLE) !=3D 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 =3D 0;
> +	struct bpf_timer *timer;
> +
> +	if (ok_sleepable & 1)
> +		return -1;
> +
> +	timer =3D bpf_map_lookup_elem(&timer_map, &key);
> +	if (timer) {
> +		if (bpf_timer_init(timer, &timer_map, CLOCK_MONOTONIC | BPF_F_TIMER_SL=
EEPABLE) !=3D 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 =3D 2;
> +	struct bpf_timer *timer;
> +
> +	timer =3D bpf_map_lookup_elem(&timer_map, &key);
> +	if (!timer)
> +		return 1;
> +
> +	if (bpf_timer_init(timer, &timer_map, CLOCK_MONOTONIC | BPF_F_TIMER_SLE=
EPABLE))
> +		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 sleepabl=
e timers?
  (same question for call to bpf_timer_set_sleepable_cb() with non-sleepabl=
e map)

[...]