Re: [PATCH bpf-next] selftests/bpf: Fix compile error when MPTCP not support

[Martin KaFai Lau <martin.lau@linux.dev>]

On 10/30/24 9:31 AM, Matthieu Baerts wrote:
> Hi Tao, BPF maintainers,
> 
> On 30/10/2024 12:12, Tao Chen wrote:
>> 在 2024/10/30 18:49, Matthieu Baerts 写道:
>>> Hi Tao Chen,
>>>
>>> Thank you for having shared this patch.
>>>
>>> On 30/10/2024 11:01, Tao Chen wrote:
>>>> Fix compile error when MPTCP feature not support, though eBPF core check
>>>> already done which seems invalid in this situation, the error info like:
>>>> progs/mptcp_sock.c:49:40: error: no member named 'is_mptcp' in 'struct
>>>> tcp_sock'
>>>>      49 |         is_mptcp = bpf_core_field_exists(tsk->is_mptcp) ?
>>>>
>>>> The filed created in new definitions with eBPF core feature to solve
>>>> this build problem, and test case result still ok in MPTCP kernel.
>>>>
>>>> 176/1   mptcp/base:OK
>>>> 176/2   mptcp/mptcpify:OK
>>>> 176     mptcp:OK
>>>> Summary: 1/2 PASSED, 0 SKIPPED, 0 FAILED
>>>>
>>>> Fixes: 8039d353217c ("selftests/bpf: Add MPTCP test base")
>>>
>>> The commit you mentioned here is more than 2 years old, and as far as I
>>> can see, nobody else reported this compilation issue. I guess that's
>>> because people used tools/testing/selftests/bpf/config file as expected
>>> to populate the kernel config, and I suppose you didn't, right?
>>>
>>
>> Hi Matt, thank you for your reply, as you said, i did not use tools/
>> testing/selftests/bpf/config to compile kernel, i will use this helpful
>> feature.
>>
>>> I don't think other BPF selftests check for missing kernel config if
>>> they are specified in the 'config' file, but even if it is the case, I
>>> think it would be better to skip all the MPTCP tests, and not try to
>>> have them checking something that doesn't exist: no need to validate
>>> these tests if the expected kernel config has not been enabled.
>>>
>>
>> If i use the kernel not support MPTCP, the compile error still exists,
>> and i can not build the bpf test successfully. Maybe skill the test case
>> seems better when kernel not support. Now that bpf_core_field_exists
>> check already used in the code, i think it is better to use new
>> definition mode.
> 
> I understand it would be better, but it means more code to maintain to
> handle that (and remembering that in future test cases). If that's not
> necessary, then no need to do the effort.
> 
> @BPF maintainers: do we need to support kernels not respecting the
> tools/testing/selftests/bpf/config file? Should we detect when a
> required kernel config is not set and skip some tests?

I guess it depends on the CONFIG_. Otherwise, it takes out the goodies of using 
<vmlinux.h> when writing bpf selftests.

If fixing the config is an option and sounds like it is for Tao, then it is 
always good to run everything in test_progs.

There are some "___local" definitions in the selftests. If mptcp test wants to 
go this path, then Matt's request to at least test__skip() makes sense to me.

pw-bot: cr

> 
>>> But again, please correct me if I'm wrong, but I don't think there is
>>> anything to change here to fix your compilation issue: simply make sure
>>> to use this tools/testing/selftests/bpf/config file to generate your
>>> kernel config, no?
> 
> Cheers,
> Matt