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

[Geliang Tang <geliang@kernel.org>]

On Tue, 2024-10-15 at 12:59 +0200, Matthieu Baerts wrote:
> 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.

"static" works. I would have thought it would depend on
CONFIG_KALLSYMS_ALL, which is not included in
tools/testing/selftests/bpf/config. Looks like I was wrong. I added
"static" for all bpf kfuncs in v10.

> 
> 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.

Other helpers have these warnings too:

$ make C=1 -o kernel/bpf/helpers.o -j8

  CC      kernel/bpf/helpers.o
  CHECK   kernel/bpf/helpers.c

kernel/bpf/helpers.c:1883:29: warning: symbol
'bpf_get_current_task_proto' was not declared. Should it be static?
kernel/bpf/helpers.c:1884:29: warning: symbol
'bpf_get_current_task_btf_proto' was not declared. Should it be static?
kernel/bpf/helpers.c:1885:29: warning: symbol
'bpf_probe_read_user_proto' was not declared. Should it be static?
kernel/bpf/helpers.c:1886:29: warning: symbol
'bpf_probe_read_user_str_proto' was not declared. Should it be static?
kernel/bpf/helpers.c:1887:29: warning: symbol
'bpf_probe_read_kernel_proto' was not declared. Should it be static?
kernel/bpf/helpers.c:1888:29: warning: symbol
'bpf_probe_read_kernel_str_proto' was not declared. Should it be
static?
kernel/bpf/helpers.c:1889:29: warning: symbol 'bpf_task_pt_regs_proto'
was not declared. Should it be static?
kernel/bpf/helpers.c:2116:18: warning: symbol 'bpf_obj_new_impl' was
not declared. Should it be static?
kernel/bpf/helpers.c:2130:18: warning: symbol 'bpf_percpu_obj_new_impl'
was not declared. Should it be static?
kernel/bpf/helpers.c:2161:18: warning: symbol 'bpf_obj_drop_impl' was
not declared. Should it be static?
kernel/bpf/helpers.c:2169:18: warning: symbol
'bpf_percpu_obj_drop_impl' was not declared. Should it be static?
kernel/bpf/helpers.c:2175:18: warning: symbol
'bpf_refcount_acquire_impl' was not declared. Should it be static?
kernel/bpf/helpers.c:2220:17: warning: symbol
'bpf_list_push_front_impl' was not declared. Should it be static?
kernel/bpf/helpers.c:2230:17: warning: symbol 'bpf_list_push_back_impl'
was not declared. Should it be static?
kernel/bpf/helpers.c:2263:34: warning: symbol 'bpf_list_pop_front' was
not declared. Should it be static?
kernel/bpf/helpers.c:2268:34: warning: symbol 'bpf_list_pop_back' was
not declared. Should it be static?
kernel/bpf/helpers.c:2273:32: warning: symbol 'bpf_rbtree_remove' was
not declared. Should it be static?
kernel/bpf/helpers.c:2329:17: warning: symbol 'bpf_rbtree_add_impl' was
not declared. Should it be static?
kernel/bpf/helpers.c:2339:32: warning: symbol 'bpf_rbtree_first' was
not declared. Should it be static?
kernel/bpf/helpers.c:2352:32: warning: symbol 'bpf_task_acquire' was
not declared. Should it be static?
kernel/bpf/helpers.c:2363:18: warning: symbol 'bpf_task_release' was
not declared. Should it be static?
kernel/bpf/helpers.c:2368:18: warning: symbol 'bpf_task_release_dtor'
was not declared. Should it be static?
kernel/bpf/helpers.c:2381:27: warning: symbol 'bpf_cgroup_acquire' was
not declared. Should it be static?
kernel/bpf/helpers.c:2393:18: warning: symbol 'bpf_cgroup_release' was
not declared. Should it be static?
kernel/bpf/helpers.c:2398:18: warning: symbol 'bpf_cgroup_release_dtor'
was not declared. Should it be static?
kernel/bpf/helpers.c:2411:27: warning: symbol 'bpf_cgroup_ancestor' was
not declared. Should it be static?
kernel/bpf/helpers.c:2431:27: warning: symbol 'bpf_cgroup_from_id' was
not declared. Should it be static?
kernel/bpf/helpers.c:2451:18: warning: symbol 'bpf_task_under_cgroup'
was not declared. Should it be static?
kernel/bpf/helpers.c:2494:27: warning: symbol 'bpf_task_get_cgroup1'
was not declared. Should it be static?
kernel/bpf/helpers.c:2511:32: warning: symbol 'bpf_task_from_pid' was
not declared. Should it be static?
kernel/bpf/helpers.c:2552:18: warning: symbol 'bpf_dynptr_slice' was
not declared. Should it be static?
kernel/bpf/helpers.c:2637:18: warning: symbol 'bpf_dynptr_slice_rdwr'
was not declared. Should it be static?
kernel/bpf/helpers.c:2670:17: warning: symbol 'bpf_dynptr_adjust' was
not declared. Should it be static?
kernel/bpf/helpers.c:2689:18: warning: symbol 'bpf_dynptr_is_null' was
not declared. Should it be static?
kernel/bpf/helpers.c:2696:18: warning: symbol 'bpf_dynptr_is_rdonly'
was not declared. Should it be static?
kernel/bpf/helpers.c:2706:19: warning: symbol 'bpf_dynptr_size' was not
declared. Should it be static?
kernel/bpf/helpers.c:2716:17: warning: symbol 'bpf_dynptr_clone' was
not declared. Should it be static?
kernel/bpf/helpers.c:2732:18: warning: symbol 'bpf_cast_to_kern_ctx'
was not declared. Should it be static?
kernel/bpf/helpers.c:2737:18: warning: symbol 'bpf_rdonly_cast' was not
declared. Should it be static?
kernel/bpf/helpers.c:2742:18: warning: symbol 'bpf_rcu_read_lock' was
not declared. Should it be static?
kernel/bpf/helpers.c:2747:18: warning: symbol 'bpf_rcu_read_unlock' was
not declared. Should it be static?
kernel/bpf/helpers.c:2776:18: warning: symbol 'bpf_throw' was not
declared. Should it be static?
kernel/bpf/helpers.c:2795:17: warning: symbol 'bpf_wq_init' was not
declared. Should it be static?
kernel/bpf/helpers.c:2809:17: warning: symbol 'bpf_wq_start' was not
declared. Should it be static?
kernel/bpf/helpers.c:2826:17: warning: symbol
'bpf_wq_set_callback_impl' was not declared. Should it be static?
kernel/bpf/helpers.c:2840:18: warning: symbol 'bpf_preempt_disable' was
not declared. Should it be static?
kernel/bpf/helpers.c:2845:18: warning: symbol 'bpf_preempt_enable' was
not declared. Should it be static?
kernel/bpf/helpers.c:2878:1: warning: symbol 'bpf_iter_bits_new' was
not declared. Should it be static?
kernel/bpf/helpers.c:2930:17: warning: symbol 'bpf_iter_bits_next' was
not declared. Should it be static?
kernel/bpf/helpers.c:2957:18: warning: symbol 'bpf_iter_bits_destroy'
was not declared. Should it be static?
kernel/bpf/helpers.c:2981:17: warning: symbol 'bpf_copy_from_user_str'
was not declared. Should it be static?

We can send our "static" version to bpf-next and see their feedback.

Thanks,
-Geliang

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