MIME-Version: 1.0
References: <20230203031742.1730761-1-imagedong@tencent.com>
 <20230203031742.1730761-3-imagedong@tencent.com> <CAEf4BzYh90NyyYvfTT=M=-KLspydMX4PZK8jCwNDydAP=kFgYw@mail.gmail.com>
 <CADxym3a6_wBHW_c_ZYtZ5QXbbunhKxau6k-fn4TNrn+6qzW6fw@mail.gmail.com>
 <CAEf4BzZAo6Bfio3pbY3j5yUDArCbdiWPC-r=XhFM9Bwq+4VVMg@mail.gmail.com> <75421c53-fa5c-d7c7-4b19-2d97e3e6d7f6@oracle.com>
In-Reply-To: <75421c53-fa5c-d7c7-4b19-2d97e3e6d7f6@oracle.com>
From:   Andrii Nakryiko <andrii.nakryiko@gmail.com>
Date:   Wed, 8 Feb 2023 15:30:51 -0800
Message-ID: <CAEf4BzZicf3B7BwPj=fWkcVJz0JayB9qUUbJFBPunxOJwQoMdw@mail.gmail.com>
Subject: Re: [PATCH bpf-next 2/2] selftests/bpf: add test for legacy/perf
 kprobe/uprobe attach mode
To:     Alan Maguire <alan.maguire@oracle.com>
Cc:     Menglong Dong <menglong8.dong@gmail.com>, ast@kernel.org,
        daniel@iogearbox.net, andrii@kernel.org, martin.lau@linux.dev,
        song@kernel.org, yhs@fb.com, john.fastabend@gmail.com,
        kpsingh@kernel.org, sdf@google.com, haoluo@google.com,
        jolsa@kernel.org, bpf@vger.kernel.org,
        linux-kernel@vger.kernel.org, Menglong Dong <imagedong@tencent.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Feb 8, 2023 at 3:49 AM Alan Maguire <alan.maguire@oracle.com> wrote:
>
> On 07/02/2023 22:50, Andrii Nakryiko wrote:
> > On Mon, Feb 6, 2023 at 6:39 PM Menglong Dong <menglong8.dong@gmail.com> wrote:
> >>
> >> On Tue, Feb 7, 2023 at 4:05 AM Andrii Nakryiko
> >> <andrii.nakryiko@gmail.com> wrote:
> >>>
> >>> On Thu, Feb 2, 2023 at 7:18 PM <menglong8.dong@gmail.com> wrote:
> >>>>
> >>>> From: Menglong Dong <imagedong@tencent.com>
> >>>>
> >>>> 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@tencent.com>
> >>>> ---
> >>>
> >>> 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.

>
> >>
> >> 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(-)
> >>>>
> >>>
> >>> [...]