On Wed, Mar 27, 2024 at 5:00 AM Yafang Shao <laoar.shao@xxxxxxxxx> wrote:
>
> Add selftests for the newly added bits iter.
> - bits_iter_success
> - percpu data extracted from the percpu struct should be expected
> - RCU lock is not required
> - It is fine without calling bpf_iter_cpumask_next()
> - It can work as expected when invalid arguments are passed
>
> - bits_iter_failure
> - bpf_iter_bits_destroy() is required after calling
> bpf_iter_bits_new()
> - bpf_iter_bits_destroy() can only destroy an initialized iter
> - bpf_iter_bits_next() must use an initialized iter
>
> Signed-off-by: Yafang Shao <laoar.shao@xxxxxxxxx>
> ---
> .../selftests/bpf/prog_tests/bits_iter.c | 139 +++++++++++++++++
> .../bpf/progs/test_bits_iter_failure.c | 54 +++++++
> .../bpf/progs/test_bits_iter_success.c | 146 ++++++++++++++++++
> 3 files changed, 339 insertions(+)
> create mode 100644 tools/testing/selftests/bpf/prog_tests/bits_iter.c
> create mode 100644 tools/testing/selftests/bpf/progs/test_bits_iter_failure.c
> create mode 100644 tools/testing/selftests/bpf/progs/test_bits_iter_success.c
>
I think all the cgroup and bpf_iter related parts of tests seems like
an unnecessary complications and just more code to support than
necessary. We have tests for cgroup iterators, so we know it will
work. You are adding bits iterator, so let's just add tests that
validate bits iterator works. We don't need iterator programs and
cgroups for that. Can we just leave RUN_TESTS-based tests and not add
yet another test runner mini-setup (I mean those positive_testcases
and negative_testcases)?
> diff --git a/tools/testing/selftests/bpf/prog_tests/bits_iter.c b/tools/testing/selftests/bpf/prog_tests/bits_iter.c
> new file mode 100644
> index 000000000000..0e2520a9dc62
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/bits_iter.c
> @@ -0,0 +1,139 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2024 Yafang Shao <laoar.shao@xxxxxxxxx> */
> +
> +#define _GNU_SOURCE
> +#include <sched.h>
> +
> +#include <test_progs.h>
> +#include "test_bits_iter_success.skel.h"
> +#include "test_bits_iter_failure.skel.h"
I'd cut out all of this code I cut out from email.
[...]
> +
> + RUN_TESTS(test_bits_iter_success);
> + RUN_TESTS(test_bits_iter_failure);
and I'd put these into prog_tests/verifier.c
> +}
> diff --git a/tools/testing/selftests/bpf/progs/test_bits_iter_failure.c b/tools/testing/selftests/bpf/progs/test_bits_iter_failure.c
> new file mode 100644
> index 000000000000..974d0b7a540e
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/test_bits_iter_failure.c
> @@ -0,0 +1,54 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright (c) 2024 Yafang Shao <laoar.shao@xxxxxxxxx> */
> +
> +#include "vmlinux.h"
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_tracing.h>
> +
> +#include "bpf_misc.h"
> +#include "task_kfunc_common.h"
> +
> +char _license[] SEC("license") = "GPL";
> +
> +int bpf_iter_bits_new(struct bpf_iter_bits *it, const void *unsafe_ptr__ign,
> + u32 nr_bits) __ksym __weak;
> +int *bpf_iter_bits_next(struct bpf_iter_bits *it) __ksym __weak;
> +void bpf_iter_bits_destroy(struct bpf_iter_bits *it) __ksym __weak;
> +
> +SEC("iter.s/cgroup")
> +__failure __msg("Unreleased reference id=3 alloc_insn=10")
you don't control assembly instruction generation, so let's not rely
on specific instruction number in error message
> +int BPF_PROG(no_destroy, struct bpf_iter_meta *meta, struct cgroup *cgrp)
> +{
> + struct bpf_iter_bits it;
> + struct task_struct *p;
> +
> + p = bpf_task_from_pid(1);
> + if (!p)
> + return 1;
> +
> + bpf_iter_bits_new(&it, p->cpus_ptr, 8192);
> +
> + bpf_iter_bits_next(&it);
> + bpf_task_release(p);
> + return 0;
> +}
> +
> +SEC("iter/cgroup")
> +__failure __msg("expected an initialized iter_bits as arg #1")
> +int BPF_PROG(next_uninit, struct bpf_iter_meta *meta, struct cgroup *cgrp)
> +{
> + struct bpf_iter_bits *it = NULL;
> +
> + bpf_iter_bits_next(it);
> + return 0;
> +}
> +
> +SEC("iter/cgroup")
> +__failure __msg("expected an initialized iter_bits as arg #1")
> +int BPF_PROG(destroy_uninit, struct bpf_iter_meta *meta, struct cgroup *cgrp)
> +{
> + struct bpf_iter_bits it = {};
> +
> + bpf_iter_bits_destroy(&it);
> + return 0;
> +}
[...]
