On Thu, Mar 28, 2024 at 1:48 AM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> 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)?
Thanks for your suggestion. I will consider your suggestion carefully.
>
> > 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.
will do it.
>
> [...]
>
> > +
> > + RUN_TESTS(test_bits_iter_success);
> > + RUN_TESTS(test_bits_iter_failure);
>
> and I'd put these into prog_tests/verifier.c
will do it.
>
> > +}
> > 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
will do it.
--
Regards
Yafang
