Re: [PATCH bpf-next 2/2] selftests/bpf: add test for legacy/perf kprobe/uprobe attach mode

[Andrii Nakryiko]

On Thu, Feb 9, 2023 at 6:08 AM Menglong Dong <menglong8.dong@xxxxxxxxx> wrote:
>
> On Thu, Feb 9, 2023 at 7:31 AM Andrii Nakryiko
> <andrii.nakryiko@xxxxxxxxx> wrote:
> >
> > On Wed, Feb 8, 2023 at 3:49 AM Alan Maguire <alan.maguire@xxxxxxxxxx> wrote:
> > >
> > > On 07/02/2023 22:50, Andrii Nakryiko wrote:
> > > > On Mon, Feb 6, 2023 at 6:39 PM Menglong Dong <menglong8.dong@xxxxxxxxx> wrote:
> > > >>
> > > >> On Tue, Feb 7, 2023 at 4:05 AM Andrii Nakryiko
> > > >> <andrii.nakryiko@xxxxxxxxx> wrote:
> > > >>>
> > > >>> On Thu, Feb 2, 2023 at 7:18 PM <menglong8.dong@xxxxxxxxx> wrote:
> > > >>>>
> > > >>>> From: Menglong Dong <imagedong@xxxxxxxxxxx>
> > > >>>>
> > > >>>> Add the testing for kprobe/uprobe attaching in legacy and perf mode.
> > > >>>> And the testing passed:
> > > >>>>
> > > >>>> ./test_progs -t attach_probe
> > > >>>> $5       attach_probe:OK
> > > >>>> Summary: 1/0 PASSED, 0 SKIPPED, 0 FAILED
> > > >>>>
> > > >>>> Signed-off-by: Menglong Dong <imagedong@xxxxxxxxxxx>
> > > >>>> ---
> > > >>>
> > > >>> Do you mind refactoring attach_probe test into multiple subtests,
> > > >>> where each subtest will only test one of the attach mode and type. The
> > > >>> reason is that libbpf CI runs tests with latest selftests and libbpf
> > > >>> against old kernels (4.9 and 5.5, currently). Due to attach_probe
> > > >>> testing all these uprobe/kprobe attach modes with extra features (like
> > > >>> cookie, ref count, etc), we had to disable attach_probe test in libbpf
> > > >>> CI on old kernels.
> > > >>>
> > > >>> If we can split each individual uprobe/kprobe mode, that will give us
> > > >>> flexibility to selectively allowlist those tests that don't force
> > > >>> libbpf to use newer features (like cookies, LINK or PERF mode, etc).
> > > >>>
> > > >>> It would be a great improvement and highly appreciated! If you don't
> > > >>> mind doing this, let's do the split of existing use cases into subtest
> > > >>> in a separate patch, and then add PERF/LEGACY/LINK mode tests on top
> > > >>> of that patch.
> > > >>>
> > > >>
> > > >> Of course, with pleasure. For the existing use cases, we split it into
> > > >> subtests, such as:
> > > >>
> > > >>   kprobe/kretprobe auto attach
> > > >>   kprobe/kretprobe manual attach
> > > >>   uprobe/uretprobe ref_ctr test
> > > >>   uprobe/uretprobe auto attach
> > > >>   sleepable kprobe/uprobe
> > > >>   ......
> > > >>
> > > >> Am I right?
> > > >
> > > > I haven't analysed all the different cases, but roughly it makes
> > > > sense. With more granular subtests we can also drop `legacy` flag and
> > > > rely on subtest allowlisting in CI.
> > > >
> > >
> > > I'm probably rusty on the details, but when you talk about subtest
> > > splitting for the [uk]probe manual attach, are we talking about running
> > > the same manual attach test for the different modes, with each as a
> > > separate subtest, such that each registers as a distinct pass/fail (and
> > > can thus be allowlisted as appropriate)? So in other words
> > >
> > > test__start_subtest("manual_attach_kprobe_link");
> > > attach_kprobe_manual(link_options);
> > > test__start_subtest("manual_attach_kprobe_legacy");
> > > attach_kprobe_manual(legay_options);
> > > test__start_subtest("manual_attach_kprobe_perf");
> > > attach_kprobe_manual(perf_options);
> > >
> > > ?
> >
> > Yep. One of the reasons is that on 4.9 kernel there won't be link or
> > perf method available, so it is expected for such modes to fail. I
> > want to be able to still test the legacy code path on 4.9, though.
> > Similarly tests that are using ref_ctr_offset or bpf_cookie won't work
> > on older kernels as well. Right now because of all of them being in a
> > single test, I have to block entire test, losing coverage on older
> > kernels.
> >
>
> I think I am beginning to understand it now. So we need 2 patches
> for the selftests part. In the 1th patch, we split the existing testings
> into multi subtests, such as:
>
> test__start_subtest("manual_attach_probe") // test kprobe/uprobe manual attach
> test__start_subtest("auto_attach_probe") // test kprobe/uprobe auto attach
> test__start_subtest("bpf_cookie") // test bpf_cookie
> test__start_subtest("ref_ctr_offset") test ref_ctr_offset
> test__start_subtest("sleepable_probe") // test sleepable
> uprobe/uretprobe, and sleepable kprobe
> ......
>
> And in the 2th patch, we change the subtest "manual_attach_probe" into
> the following tests:
>
> test__start_subtest("manual_attach_kprobe_link");
> test__start_subtest("manual_attach_kprobe_legacy");
> test__start_subtest("manual_attach_kprobe_perf");
>
> Therefore, every feature can be tested in a subtest alone.
>
> Am I right?

yep, exactly!

>
> Thanks!
> Menglong Dong
> > >
> > > >>
> > > >> Thanks!
> > > >> Dongmeng Long
> > > >>
> > > >>>
> > > >>>>  .../selftests/bpf/prog_tests/attach_probe.c   | 61 ++++++++++++++++++-
> > > >>>>  .../selftests/bpf/progs/test_attach_probe.c   | 32 ++++++++++
> > > >>>>  2 files changed, 92 insertions(+), 1 deletion(-)
> > > >>>>
> > > >>>
> > > >>> [...]