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

[Alan Maguire]

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);

?

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