Re: [PATCH bpf-next 2/2] selftests/bpf: Check bpftool batch map dump contents

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 5292 bytes --]

> selftests/bpf: Check bpftool batch map dump contents
>
> Exercise hash map dumps around the initial batch size and across
> multiple batches, including empty and single-entry maps. Compare each
> complete unordered key/value set with the input data in plain, JSON
> and pretty JSON output.
>
> Cover one-byte keys, three-byte values and BTF-formatted maps to catch
> cursor sizing, buffer alignment and formatting regressions.

The changelog, the file name and the subtest names all describe a batched
map-dump implementation that does not exist in the code under test.

bpftool map dump is implemented by map_dump() in tools/bpf/bpftool/map.c,
which walks the map one element at a time:

    while (true) {
        err = bpf_map_get_next_key(fd, prev_key, key);
        if (err) { ... break; }
        if (!dump_map_elem(fd, key, value, info, btf, wtr))
            num_elems++;
        prev_key = key;
    }

and dump_map_elem() calls bpf_map_lookup_elem() per key.

grep -rn 'batch\|BPF_MAP_LOOKUP_BATCH\|bpf_map_lookup_batch'
tools/bpf/bpftool/ returns only the unrelated bpftool batch file FILE
subcommand in main.c. There is no bpf_map_lookup_batch() call, no batch
cursor (in_batch/out_batch), no per-batch buffer and no batch-size
constant anywhere in bpftool.

What does this mean for the test?

- "around the initial batch size", "across multiple batches" and "cursor
  sizing, buffer alignment" describe nothing in the tested code. The
  counts 255/256/257/1025 and the subtest names below_batch, exact_batch,
  above_batch, multiple_batches are arbitrary; 256 is not a boundary of
  any code path.

- The file name bpftool_map_batch.c and the test name bpftool_map_batch
  collide with bpftool's real, unrelated batch subcommand (bpftool batch
  file FILE, main.c:334 do_batch()), so a reader looking for coverage of
  that subcommand will find a map-dump test instead.

- The series range 407ec415f849..1dcddb9351e8 contains no commit
  converting map dump to the batch API (the only other change is
  map_perf_ring.c ring-buffer support in c23648f76644), so this is not
  infrastructure landing ahead of its user.

What the test actually does is valid and useful - it verifies the exact
plain/JSON/pretty-JSON rendering of map dump for various key/value sizes
and element counts. Should the changelog and naming describe that (e.g.
"check map dump output contents") instead of claiming batch-boundary
coverage, so a future maintainer doesn't believe the batch paths are
covered when they are not?

> diff --git a/tools/testing/selftests/bpf/prog_tests/bpftool_map_batch.c b/tools/testing/selftests/bpf/prog_tests/bpftool_map_batch.c
> new file mode 100644
> index 000000000000..139d13a49dce
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/bpftool_map_batch.c

[ ... ]

> @@ -0,0 +1,186 @@

[ ... ]

> +void test_bpftool_map_batch(void)
> +{
> +	static const struct dump_case cases[] = {
> +		{ "empty", 0, 4, 4 },
> +		{ "single", 1, 4, 4 },
> +		{ "below_batch", 255, 4, 4 },
> +		{ "exact_batch", 256, 4, 4 },
> +		{ "above_batch", 257, 4, 4 },
> +		{ "multiple_batches", 1025, 4, 4 },
> +		{ "one_byte_key", 256, 1, 4 },
> +		{ "odd_value_size", 257, 4, 3 },
> +		{ "btf_empty", 0, 4, 4, true },
> +		{ "btf_single", 1, 4, 4, true },
> +		{ "btf_multiple_batches", 1025, 4, 4, true },
> +	};

A subsystem pattern flags this as potentially concerning: the new test
file is structured around an internal batching strategy rather than the
interface it actually checks. Four of the eleven cases exist only to
straddle a supposed 256-element batch boundary (below_batch, exact_batch,
above_batch, multiple_batches), and the changelog names "cursor sizing"
and "buffer alignment" as the regressions being guarded.

map_dump() in tools/bpf/bpftool/map.c uses bpf_map_get_next_key() +
bpf_map_lookup_elem() per element, so no such boundary exists and these
cases are indistinguishable from any other element count. The
interface-level part of the test (exact plain/JSON/pretty-JSON record
text, element count footer, BTF formatted field) is sound and is what the
case table should be organised around.

The guide directive notes: a new selftest file added for a narrow variant
(a specific size, a specific internal code path) of behavior an existing
test in the same directory already covers more generally should be
reported as a concern - asking whether it should be a new case in the
existing test instead. Tests written against specific implementation
details (an internal batching strategy, a particular code path taken to
reach a result) break or need a rewrite whenever that implementation
changes.

That said, the assertions themselves are written against observable
bpftool output, not against internal state, so the test will not break if
bpftool's enumeration strategy changes - only the case names and the
changelog would become misleading. Adding extra element counts is also
harmless coverage.

Does the case table organization based on a non-existent batch boundary
make the test harder to maintain or extend?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/34048930508