| Message ID | 20240331034154.16284-1-laoar.shao@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | bpf: Add a generic bits iterator | expand |
On Sat, Mar 30, 2024 at 8:44 PM Yafang Shao <laoar.shao@gmail.com> wrote: > > Three new kfuncs, namely bpf_iter_bits_{new,next,destroy}, have been > added for the new bpf_iter_bits functionality. These kfuncs enable the > iteration of the bits from a given address and a given number of bits. > > - bpf_iter_bits_new > Initialize a new bits iterator for a given memory area. Due to the > limitation of bpf memalloc, the max number of bits to be iterated > over is (4096 * 8). > - bpf_iter_bits_next > Get the next bit in a bpf_iter_bits > - bpf_iter_bits_destroy > Destroy a bpf_iter_bits > > The bits iterator can be used in any context and on any address. > > Changes: > - v4->v5: > - Simplify test cases (Andrii) hm... I think you oversimplified them :) Your negative tests are good, but there is now no "positive" test that shows that iterator functions properly. I'd suggest to add few positive tests (still within RUN_TESTS framework) using __retval (grep for other use cases using it) to check actual values. I think you need to validate that iterator works for: a) bit mask smaller than 8 bytes (and also maybe number of bits that are not multiple of 8, e.g., have 20 bit bitmask, but set entire 32-bit value to 1s, check that you get only 20 iterator next() returns) b) a typical case of having 8-byte bit mask (you can have some irregular pattern of bits, like 0b1111000111001101, and calculate a sum of bit positions, returned for __retval check. This will catch endianness issues, if there are still any. c) another typical case where bit mask is > 8 bytes (same idea with position sum would work, but probably just counting number of bits would be enough) pw-bot: cr > - v3->v4: > - Fix endianness error on s390x (Andrii) > - zero-initialize kit->bits_copy and zero out nr_bits (Andrii) > - v2->v3: > - Optimization for u64/u32 mask (Andrii) > - v1->v2: > - Simplify the CPU number verification code to avoid the failure on s390x > (Eduard) > - bpf: Add bpf_iter_cpumask > https://lwn.net/Articles/961104/ > - bpf: Add new bpf helper bpf_for_each_cpu > https://lwn.net/Articles/939939/ > > Yafang Shao (2): > bpf: Add bits iterator > selftests/bpf: Add selftest for bits iter > > kernel/bpf/helpers.c | 120 ++++++++++++++++++ > .../selftests/bpf/prog_tests/verifier.c | 2 + > .../selftests/bpf/progs/verifier_bits_iter.c | 57 +++++++++ > 3 files changed, 179 insertions(+) > create mode 100644 tools/testing/selftests/bpf/progs/verifier_bits_iter.c > > -- > 2.39.1 >
On Wed, Apr 3, 2024 at 1:22 AM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Sat, Mar 30, 2024 at 8:44 PM Yafang Shao <laoar.shao@gmail.com> wrote: > > > > Three new kfuncs, namely bpf_iter_bits_{new,next,destroy}, have been > > added for the new bpf_iter_bits functionality. These kfuncs enable the > > iteration of the bits from a given address and a given number of bits. > > > > - bpf_iter_bits_new > > Initialize a new bits iterator for a given memory area. Due to the > > limitation of bpf memalloc, the max number of bits to be iterated > > over is (4096 * 8). > > - bpf_iter_bits_next > > Get the next bit in a bpf_iter_bits > > - bpf_iter_bits_destroy > > Destroy a bpf_iter_bits > > > > The bits iterator can be used in any context and on any address. > > > > Changes: > > - v4->v5: > > - Simplify test cases (Andrii) > > hm... I think you oversimplified them :) Your negative tests are good, I misinterpreted your earlier comment :( > but there is now no "positive" test that shows that iterator functions > properly. I'd suggest to add few positive tests (still within > RUN_TESTS framework) using __retval (grep for other use cases using > it) to check actual values. > > I think you need to validate that iterator works for: > > a) bit mask smaller than 8 bytes (and also maybe number of bits that > are not multiple of 8, e.g., have 20 bit bitmask, but set entire > 32-bit value to 1s, check that you get only 20 iterator next() > returns) > > b) a typical case of having 8-byte bit mask (you can have some > irregular pattern of bits, like 0b1111000111001101, and calculate a > sum of bit positions, returned for __retval check. This will catch > endianness issues, if there are still any. > > c) another typical case where bit mask is > 8 bytes (same idea with > position sum would work, but probably just counting number of bits > would be enough) Thank you for providing such a detailed explanation. I will take some time to carefully consider it.
Three new kfuncs, namely bpf_iter_bits_{new,next,destroy}, have been added for the new bpf_iter_bits functionality. These kfuncs enable the iteration of the bits from a given address and a given number of bits. - bpf_iter_bits_new Initialize a new bits iterator for a given memory area. Due to the limitation of bpf memalloc, the max number of bits to be iterated over is (4096 * 8). - bpf_iter_bits_next Get the next bit in a bpf_iter_bits - bpf_iter_bits_destroy Destroy a bpf_iter_bits The bits iterator can be used in any context and on any address. Changes: - v4->v5: - Simplify test cases (Andrii) - v3->v4: - Fix endianness error on s390x (Andrii) - zero-initialize kit->bits_copy and zero out nr_bits (Andrii) - v2->v3: - Optimization for u64/u32 mask (Andrii) - v1->v2: - Simplify the CPU number verification code to avoid the failure on s390x (Eduard) - bpf: Add bpf_iter_cpumask https://lwn.net/Articles/961104/ - bpf: Add new bpf helper bpf_for_each_cpu https://lwn.net/Articles/939939/ Yafang Shao (2): bpf: Add bits iterator selftests/bpf: Add selftest for bits iter kernel/bpf/helpers.c | 120 ++++++++++++++++++ .../selftests/bpf/prog_tests/verifier.c | 2 + .../selftests/bpf/progs/verifier_bits_iter.c | 57 +++++++++ 3 files changed, 179 insertions(+) create mode 100644 tools/testing/selftests/bpf/progs/verifier_bits_iter.c