Hi Ian,
Thanks for your comments!
On Thu, May 29, 2025 at 11:12 AM Ian Rogers <irogers@xxxxxxxxxx> wrote:
> On Wed, May 21, 2025 at 3:27 PM Blake Jones <blakejones@xxxxxxxxxx> wrote:
> > diff --git a/tools/perf/util/bpf_skel/sample_filter.bpf.c b/tools/perf/util/bpf_skel/sample_filter.bpf.c
> > [...]
> > +// This is used by tests/shell/record_bpf_metadata.sh
> > +// to verify that BPF metadata generation works.
> > +const int bpf_metadata_test_value SEC(".rodata") = 42;
>
> This is a bit random.
Yeah, that's fair. I added it because it was a straightforward way to reliably
get a known value that I could observe from tests/shell/test_bpf_metadata.sh.
> For the non-BPF C code we have a build generated
> PERF-VERSION-FILE that contains something like `#define PERF_VERSION
> "6.15.rc7.ge450e74276d2"`. I wonder having something like
]> [...]
> would be more useful/meaningful. Perhaps the build could inject the
> variable to avoid duplicating it all the BPF skeletons.
I could do this if you'd like. It would make it harder for my test to check
that it was reporting the right value, because the PERF-VERSION-FILE defines
PERF_VERSION in a way that's useful for C programs but not shell scripts.
I'd just have the test check that it was reporting some string (maybe one
with at least one dot in it, if I can stably make that assumption). WDYT?
> nit: I wonder for testing it would be interesting to have 0 and >1
> metadata values tested too. We may want to have test programs
> explicitly for that, in tools/perf/tests.
My testing right now depends pretty heavily on the fact that perf will load
sample_filter.bpf.o for me; this seems much better than trying to load a
BPF program manually from a test.
If I could find other perf invocations that would load other BPF programs
automatically, I could potentially test the "0 metadata" and ">1 metadata"
cases. That would involve more BPF-program-specific modifications, though,
which would be closer to the thing you objected to above. So to me this
doesn't seem worth the effort.
Blake
