Re: [PATCH bpf-next 4/4] selftests/bpf: Add test for checking correct nop of optimized usdt

[Jiri Olsa <olsajiri@gmail.com>]

On Mon, Nov 24, 2025 at 09:34:45AM -0800, Andrii Nakryiko wrote:
> On Mon, Nov 17, 2025 at 12:36 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Adding test that attaches bpf program on usdt probe in 2 scenarios;
> >
> > - attach program on top of usdt_1 which is standard nop probe
> >   incidentally followed by nop5. The usdt probe does not have
> >   extra data in elf note record, so we expect the probe to land
> >   on the first nop without being optimized.
> >
> > - attach program on top of usdt_2 which is probe defined on top
> >   of nop,nop5 combo. The extra data in the elf note record and
> >   presence of upeobe syscall ensures that the probe is placed
> >   on top of nop5 and optimized.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  tools/testing/selftests/bpf/.gitignore        |  2 +
> >  tools/testing/selftests/bpf/Makefile          |  7 +-
> >  tools/testing/selftests/bpf/prog_tests/usdt.c | 82 +++++++++++++++++++
> >  tools/testing/selftests/bpf/progs/test_usdt.c |  9 ++
> >  tools/testing/selftests/bpf/usdt_1.c          | 14 ++++
> >  tools/testing/selftests/bpf/usdt_2.c          | 13 +++
> >  6 files changed, 126 insertions(+), 1 deletion(-)
> >  create mode 100644 tools/testing/selftests/bpf/usdt_1.c
> >  create mode 100644 tools/testing/selftests/bpf/usdt_2.c
> >
> 
> can you please add a simple uprobe benchmark so that we can compare
> nop1 vs nop5 performance easily? See below, I'd actually force nop1
> for existing test with custom USDT_NOP override, and assume nop1+nop5
> as a default case for nop5 bench.

yes, will add it

> 
> > diff --git a/tools/testing/selftests/bpf/.gitignore b/tools/testing/selftests/bpf/.gitignore
> > index be1ee7ba7ce0..89f480729a6b 100644
> > --- a/tools/testing/selftests/bpf/.gitignore
> > +++ b/tools/testing/selftests/bpf/.gitignore
> > @@ -45,3 +45,5 @@ xdp_synproxy
> >  xdp_hw_metadata
> >  xdp_features
> >  verification_cert.h
> > +usdt_1
> > +usdt_2
> > diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> > index 34ea23c63bd5..4a21657e45f7 100644
> > --- a/tools/testing/selftests/bpf/Makefile
> > +++ b/tools/testing/selftests/bpf/Makefile
> > @@ -733,6 +733,10 @@ $(VERIFICATION_CERT) $(PRIVATE_KEY): $(VERIFY_SIG_SETUP)
> >  $(VERIFY_SIG_HDR): $(VERIFICATION_CERT)
> >         $(Q)xxd -i -n test_progs_verification_cert $< > $@
> >
> > +ifeq ($(SRCARCH),$(filter $(SRCARCH),x86))
> > +USDTX := usdt_1.c usdt_2.c
> > +endif
> > +
> 
> why not compile it unconditionally, why complicating makefile if we
> can do x86-64-specific logic in corresponding files?

ok

> 
> >  # Define test_progs test runner.
> >  TRUNNER_TESTS_DIR := prog_tests
> >  TRUNNER_BPF_PROGS_DIR := progs
> > @@ -754,7 +758,8 @@ TRUNNER_EXTRA_SOURCES := test_progs.c               \
> >                          json_writer.c          \
> >                          $(VERIFY_SIG_HDR)              \
> >                          flow_dissector_load.h  \
> > -                        ip_check_defrag_frags.h
> > +                        ip_check_defrag_frags.h \
> > +                        $(USDTX)
> >  TRUNNER_LIB_SOURCES := find_bit.c
> >  TRUNNER_EXTRA_FILES := $(OUTPUT)/urandom_read                          \
> >                        $(OUTPUT)/liburandom_read.so                     \
> > diff --git a/tools/testing/selftests/bpf/prog_tests/usdt.c b/tools/testing/selftests/bpf/prog_tests/usdt.c
> > index f4be5269fa90..a8ca2920c009 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/usdt.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/usdt.c
> > @@ -247,6 +247,86 @@ static void subtest_basic_usdt(bool optimized)
> >  #undef TRIGGER
> >  }
> >
> > +#ifdef __x86_64
> > +extern void usdt_1(void);
> > +extern void usdt_2(void);
> > +
> > +/* nop, nop5 */
> > +static unsigned char nop15[6] = { 0x90, 0x0f, 0x1f, 0x44, 0x00, 0x00 };
> 
> nop15 is a very confusing name for this :) nop1_nop5_combo ?

ok :)

> 
> > +
> > +static void *find_nop15(void *fn)
> > +{
> > +       int i;
> > +
> > +       for (i = 0; i < 10; i++) {
> > +               if (!memcmp(nop15, fn + i, 5))
> > +                       return fn + i;
> > +       }
> > +       return NULL;
> > +}
> > +
> 
> [...]
> 
> >  char _license[] SEC("license") = "GPL";
> > diff --git a/tools/testing/selftests/bpf/usdt_1.c b/tools/testing/selftests/bpf/usdt_1.c
> > new file mode 100644
> > index 000000000000..0e00702b1701
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/usdt_1.c
> > @@ -0,0 +1,14 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +/*
> > + * Include usdt.h with defined USDT_NOP macro will switch
> > + * off the extra info in usdt probe.
> > + */
> > +#define USDT_NOP .byte 0x90, 0x0f, 0x1f, 0x44, 0x00, 0x00
> > +#include "usdt.h"
> 
> upstream usdt.h will use nop1+nop5 on x86-64 unconditionally, so I'd
> invert this, and *force* one of the cases to a single nop1 with custom
> USDT_NOP, wdyt?

ok, it's basically what it's doing now, just with the extra nop5,
but I agree that having just nop1 makes more sense

thanks,
jirka