Re: [PATCH mptcp-next v9 0/7] add mptcp_subflow bpf_iter

[Matthieu Baerts <matttbe@kernel.org>]

On 15/10/2024 11:20, Geliang Tang wrote:
> Hi Matt,
> 
> On Tue, 2024-10-15 at 11:01 +0200, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> Thank you for your reply!
>>
>> On 15/10/2024 09:27, Geliang Tang wrote:
>>> Hi Matt,
>>>
>>> Thanks for this review.
>>>
>>> On Mon, 2024-10-14 at 18:08 +0200, Matthieu Baerts wrote:
>>>> Hi Geliang,
>>>>
>>>> On 09/10/2024 12:05, MPTCP CI wrote:
>>>>> Hi Geliang,
>>>>>
>>>>> Thank you for your modifications, that's great!
>>>>>
>>>>> But sadly, our CI spotted some issues with it when trying to
>>>>> build
>>>>> it.
>>>>>
>>>>> You can find more details there:
>>>>>
>>>>>  
>>>>> https://github.com/multipath-tcp/mptcp_net-next/actions/runs/11252652867
>>>>
>>>> I was looking at applying this series, but there are some issues
>>>> reported by the CI:
>>>>
>>>>   warning: symbol 'bpf_*mptcp_*' was not declared. Should it be
>>>> static?
>>>>
>>>> Could it be possible to have a fix for that please before
>>>> applying
>>>> the
>>>> series?
>>>
>>> No fix is ​​needed, just ignore these warnings.
>>
>> If possible, I would prefer not to ignore these warnings, because
>> other
>> CI might report the same issue. I didn't check, but can you not
>> simply
>> declare these new helpers as "static"? It looks like we can have
>> kfunc
>> declared as static, no?
> 
> No, "static" doesn't work.

Could we declare them in protocol.h? Or is it not enough?

>>> This error is also
>>> reported in other places:
>>>
>>> $ make C=1 -o net/socket.o
>>>   CALL    scripts/checksyscalls.sh
>>>   DESCEND objtool
>>>   INSTALL libsubcmd_headers
>>>   DESCEND bpf/resolve_btfids
>>>   INSTALL libsubcmd_headers
>>>   CC      net/socket.o
>>>   CHECK   net/socket.c
>>> net/socket.c:1704:21: warning: symbol 'update_socket_protocol' was
>>> not
>>> declared. Should it be static?
>>
>> In this example, you are showing one symbol that has been added for
>> MPTCP, maybe we forgot something :)
> 
> Do you mean we should name it as "mptcp_update_socket_protocol"? No
> need. It's a public hook for any protocol.

No sorry, I just wanted to say that it is probably not a good example,
because this helper has been introduced by you, and we have maybe missed
something to avoid the warning.

>>> It seems that it is because "-Wmissing-declarations" is not
>>> recognized
>>> by sparse
>>
>> I don't see complains about that when introducing new kfunc, maybe we
>> are supposed to do something else to avoid that?
> 
> I have no idea yet. You can listen to the opinions of BPF maintainers
> when you are in upstream.

I will try to get an answer before, just not to have to modify the CI to
ignore all these cases if there is no need to.

>>>> I guess you are missing __bpf_kfunc_start_defs() and
>>>> __bpf_kfunc_end_defs() around the declaration of the BPF
>>>> dedicated
>>>> kfunc, no?
>>>
>>> No. __bpf_kfunc_start_defs() and __bpf_kfunc_end_defs() are indeed
>>> used
>>> in patch 2.
>>
>> Thanks, I missed that.
>>
>>>> Also, where should I apply these patches? Before "mptcp: add
>>>> sched_data
>>>> helpers"?
>>>
>>> Yes, before "mptcp: add sched_data helpers", after "selftests/bpf:
>>> Add
>>> mptcp subflow subtest".
>>
>> OK!
>>
>>>> But then there should not be any dependences on the BPF
>>>> scheduler work (and I think that would be better without this
>>>> dependence, see my comment on patch 4/7)
>>>
>>> This set doesn't have any dependence on the BPF packet scheduler
>>> since
>>> the selftest is added as a ftrace. It somehow depends on packet
>>> scheduler since it invoke some packet scheduler functions such as
>>> mptcp_subflow_active() and bpf_mptcp_subflow_tcp_sock().
>>
>> OK, but if I insert the series just after "selftests/bpf: Add mptcp
>> subflow subtest", 
> 
> "selftests/bpf: Add mptcp subflow subtest" has been upstreamed. It
> should be before "mptcp: add sched_data helpers".
> 
>> it will not have access to mptcp_subflow_active().
> 
> The access to mptcp_subflow_active() is added in patch 1 "bpf: Register
> mptcp common kfunc set" in this set.

Ah OK, I didn't know I had to include them in patch 1: the commit
message mentions them, but it was not clear to me that I had to import
them from another patch when resolving the conflicts.
Next time, don't hesitate to add a squash-to patch before, removing the
code from one commit (even if it is going to be placed after), and
mention the order ;)


> 
> The whole order is (from bottom to top):
> 
> selftests/bpf: Add bpf scheduler test
> bpf: Add bpf_mptcp_sched_kfunc_set
> bpf: Add bpf_mptcp_sched_ops
> mptcp: add sched_data helpers
> selftests/bpf: Add mptcp_subflow bpf_iter subtest
> Squash to "selftests/bpf: Add bpf scheduler test"
> selftests/bpf: More endpoints for endpoint_init
> selftests/bpf: Add mptcp_subflow bpf_iter test prog
> bpf: Add mptcp_sock acquire and release helpers
> bpf: Add mptcp_subflow bpf_iter
> bpf: Register mptcp common kfunc set
> selftests/bpf: Add mptcp subflow subtest
> selftests/bpf: Add getsockopt to inspect mptcp subflow
> selftests/bpf: Add mptcp subflow example

Thanks, that's clearer!

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.