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

[Matthieu Baerts <matttbe@kernel.org>]

On 15/10/2024 11:38, Geliang Tang wrote:
> 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?
>>
>>> 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 :)
>>
>>> 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 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", it will not have access to mptcp_subflow_active().
>> I
>> still need to check your reply on the patch 4/7, but it would be
>> easier
>> if the test doesn't depend on "mptcp_subflow_active()": it's just a
> 
> Do you mean only drop "mptcp_subflow_active" and
> "mptcp_subflow_set_scheduled" in the test or drop all the helpers,
> include "bpf_mptcp_subflow_tcp_sock" and "bpf_mptcp_subflow_ctx"?

I initially didn't understand these kfunc would be included in patch 1,
so I thought the test would not work if this series was placed before
the other BPF scheduler patches we have in the tree.

But thinking about that, I still think it is better to avoid using
anything linked to the BPF scheduler. So yes, I think it is best not to
include these scheduler-related kfunc in this series: not in the kernel
code, nor in the BPF selftests.


> If it is the latter, the test is simplified like this, right?
> 
>         if (bpf_get_current_pid_tgid() >> 32 != pid)
>                 return 0;
> 
>         if (msk->pm.server_side || !msk->pm.subflows)
>                 return 0;
> 
>         ids = 0;
> 
>         msk = bpf_mptcp_sock_acquire(msk);
>         if (!msk)
>                 return 0;
>         bpf_for_each(mptcp_subflow, subflow, msk) {
>                 ids += subflow->subflow_id;
>         bpf_mptcp_sock_release(msk);
> 
>         return 0;
> 
> To me both are fine, which one do you prefer? I can refactor the code
> and send the squash-to patches. Or maybe a v10 is better?

Yes for the simpler test (see my reply attached to the patch 4/7)
without any references to the packet scheduler.

And yes for the v10, it will be easier. Also including your other
squash-to patch you sent: Squash to "selftests/bpf: More endpoints for
endpoint_init".

Thanks!
Matt
-- 
Sponsored by the NGI0 Core fund.