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

[Matthieu Baerts <matttbe@kernel.org>]

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 test
in preparation to the packet scheduler, it is fine if it is not doing
interesting for the moment I think.

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