From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Yafang Shao <laoar.shao@gmail.com>
Cc: ast@kernel.org, daniel@iogearbox.net, john.fastabend@gmail.com,
andrii@kernel.org, martin.lau@linux.dev, eddyz87@gmail.com,
song@kernel.org, yonghong.song@linux.dev, kpsingh@kernel.org,
sdf@google.com, haoluo@google.com, jolsa@kernel.org,
bpf@vger.kernel.org
Subject: Re: [PATCH v7 bpf-next 2/2] selftests/bpf: Add selftest for bits iter
Date: Thu, 9 May 2024 15:03:16 -0700 [thread overview]
Message-ID: <CAEf4BzawfHfeOhm+BP3AzrcLE8dv+gEXQsE7nRY+yqebkbcvbA@mail.gmail.com> (raw)
In-Reply-To: <CALOAHbBiHa8+LXio6ZnJ-kUhezQnDhaj9koT-hVOaPiS3SHrDA@mail.gmail.com>
On Wed, May 8, 2024 at 7:11 PM Yafang Shao <laoar.shao@gmail.com> wrote:
>
> On Wed, May 8, 2024 at 1:12 AM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Tue, May 7, 2024 at 6:39 AM Yafang Shao <laoar.shao@gmail.com> wrote:
> > >
> > > On Tue, May 7, 2024 at 11:42 AM Andrii Nakryiko
> > > <andrii.nakryiko@gmail.com> wrote:
> > > >
> > > > On Sun, May 5, 2024 at 8:35 PM Yafang Shao <laoar.shao@gmail.com> wrote:
> > > > >
> > > > > Add test cases for the bits iter:
> > > > > - positive case
> > > > > - bit mask smaller than 8 bytes
> > > > > - a typical case of having 8-byte bit mask
> > > > > - another typical case where bit mask is > 8 bytes
> > > > > - the index of set bit
> > > > >
> > > > > - nagative cases
> > > > > - 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@gmail.com>
> > > > > ---
> > > > > .../selftests/bpf/prog_tests/verifier.c | 2 +
> > > > > .../selftests/bpf/progs/verifier_bits_iter.c | 160 ++++++++++++++++++
> > > > > 2 files changed, 162 insertions(+)
> > > > > create mode 100644 tools/testing/selftests/bpf/progs/verifier_bits_iter.c
> > > > >
> > > > > diff --git a/tools/testing/selftests/bpf/prog_tests/verifier.c b/tools/testing/selftests/bpf/prog_tests/verifier.c
> > > > > index c4f9f306646e..7e04ecaaa20a 100644
> > > > > --- a/tools/testing/selftests/bpf/prog_tests/verifier.c
> > > > > +++ b/tools/testing/selftests/bpf/prog_tests/verifier.c
> > > > > @@ -84,6 +84,7 @@
> > > > > #include "verifier_xadd.skel.h"
> > > > > #include "verifier_xdp.skel.h"
> > > > > #include "verifier_xdp_direct_packet_access.skel.h"
> > > > > +#include "verifier_bits_iter.skel.h"
> > > > >
> > > > > #define MAX_ENTRIES 11
> > > > >
> > > > > @@ -198,6 +199,7 @@ void test_verifier_var_off(void) { RUN(verifier_var_off); }
> > > > > void test_verifier_xadd(void) { RUN(verifier_xadd); }
> > > > > void test_verifier_xdp(void) { RUN(verifier_xdp); }
> > > > > void test_verifier_xdp_direct_packet_access(void) { RUN(verifier_xdp_direct_packet_access); }
> > > > > +void test_verifier_bits_iter(void) { RUN(verifier_bits_iter); }
> > > > >
> > > > > static int init_test_val_map(struct bpf_object *obj, char *map_name)
> > > > > {
> > > > > diff --git a/tools/testing/selftests/bpf/progs/verifier_bits_iter.c b/tools/testing/selftests/bpf/progs/verifier_bits_iter.c
> > > > > new file mode 100644
> > > > > index 000000000000..2f7b62b25638
> > > > > --- /dev/null
> > > > > +++ b/tools/testing/selftests/bpf/progs/verifier_bits_iter.c
> > > > > @@ -0,0 +1,160 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0-only
> > > > > +/* Copyright (c) 2024 Yafang Shao <laoar.shao@gmail.com> */
> > > > > +
> > > > > +#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")
> > > > > +__description("bits iter without destroy")
> > > > > +__failure __msg("Unreleased reference")
> > > > > +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")
> > > > > +__description("bits iter with uninitialized iter in ->next()")
> > > > > +__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")
> > > > > +__description("bits iter with uninitialized iter in ->destroy()")
> > > > > +__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;
> > > > > +}
> > > > > +
> > > > > +SEC("syscall")
> > > > > +__description("bits copy 32")
> > > > > +__success __retval(10)
> > > > > +int bits_copy32(void)
> > > > > +{
> > > > > + /* 21 bits: --------------------- */
> > > > > + u32 data = 0b11111101111101111100001000100101U;
> > > >
> > > > if you define this bit mask as an array of bytes, then you won't have
> > > > to handle big-endian in the tests at all
> > >
> > > This test case provides a clear example of iterating over data of type
> > > u32, offering valuable guidance for users who need to perform such
> > > iterations.
> > >
> > > >
> > > >
> > > > > + int nr = 0, offset = 0;
> > > > > + int *bit;
> > > > > +
> > > > > +#if defined(__TARGET_ARCH_s390)
> > > > > + offset = sizeof(u32) - (21 + 7) / 8;
> > > > > +#endif
> > > > > + bpf_for_each(bits, bit, ((char *)&data) + offset, 21)
> > > > > + nr++;
> > > > > + return nr;
> > > > > +}
> > > > > +
> > > > > +SEC("syscall")
> > > > > +__description("bits copy 64")
> > > > > +__success __retval(18)
> > > > > +int bits_copy64(void)
> > > > > +{
> > > > > + /* 34 bits: ~-------- */
> > > > > + u64 data = 0xffffefdf0f0f0f0fUL;
> > > > > + int nr = 0, offset = 0;
> > > > > + int *bit;
> > > > > +
> > > > > +#if defined(__TARGET_ARCH_s390)
> > > > > + offset = sizeof(u64) - (34 + 7) / 8;
> > > > > +#endif
> > > > > +
> > > > > + bpf_for_each(bits, bit, ((char *)&data) + offset, 34)
> > > >
> > > > see above about byte array, but if we define different (not as byte
> > > > array but long[]), it would be cleaner to have
> > >
> > > This test case demonstrates how to iterate over data of type u64.
> > >
> > > >
> > > > #if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
> > > > u64 data = 0x......UL;
> > > > #else
> > > > u64 data = 0x......UL;
> > > > #endif
> > >
> > > looks good.
> > >
> >
> > Please hold off on sending a new revision until we figure out what the
> > contract should be. Because I feel like it's a (relatively) big
> > decision whether a bit mask is treated as an array of bytes or as an
> > array of longs. For little-endian it makes no difference, but for
> > big-endian it's a big difference and has usability and performance
> > implications.
>
> Perhaps it would be advantageous to define the interface as follows:
>
> bpf_iter_bits_new(struct bpf_iter_bits *it, const u64
> *unsafe_ptr__ign, u32 words)
>
> This approach eliminates the need to account for endianness.
I don't mind that, if others don't have any opinion. Let's just
document that by "words" we mean 8-byte integers.
>
> --
> Regards
> Yafang
prev parent reply other threads:[~2024-05-09 22:03 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-06 3:33 [PATCH v7 bpf-next 0/2] bpf: Add a generic bits iterator Yafang Shao
2024-05-06 3:33 ` [PATCH v7 bpf-next 1/2] bpf: Add " Yafang Shao
2024-05-07 3:38 ` Andrii Nakryiko
2024-05-07 13:32 ` Yafang Shao
2024-05-07 17:09 ` Andrii Nakryiko
2024-05-06 3:33 ` [PATCH v7 bpf-next 2/2] selftests/bpf: Add selftest for bits iter Yafang Shao
2024-05-07 3:42 ` Andrii Nakryiko
2024-05-07 13:38 ` Yafang Shao
2024-05-07 17:11 ` Andrii Nakryiko
2024-05-09 2:11 ` Yafang Shao
2024-05-09 22:03 ` Andrii Nakryiko [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAEf4BzawfHfeOhm+BP3AzrcLE8dv+gEXQsE7nRY+yqebkbcvbA@mail.gmail.com \
--to=andrii.nakryiko@gmail.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=eddyz87@gmail.com \
--cc=haoluo@google.com \
--cc=john.fastabend@gmail.com \
--cc=jolsa@kernel.org \
--cc=kpsingh@kernel.org \
--cc=laoar.shao@gmail.com \
--cc=martin.lau@linux.dev \
--cc=sdf@google.com \
--cc=song@kernel.org \
--cc=yonghong.song@linux.dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).