| Message ID | cover.1728466623.git.tanggeliang@kylinos.cn (mailing list archive) |
|---|---|
| Headers | show |
| Series | add mptcp_subflow bpf_iter | expand |
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 Status: failure Initiator: Patchew Applier Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/b00f1ec86da9 Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=897104 Feel free to reply to this email if you cannot access logs, if you need some support to fix the error, if this doesn't seem to be caused by your modifications or if the error is a false positive one. Cheers, MPTCP GH Action bot Bot operated by Matthieu Baerts (NGI0 Core)
Hi Geliang,
Thank you for your modifications, that's great!
Our CI did some validations and here is its report:
- KVM Validation: normal: Success! ✅
- KVM Validation: debug: Success! ✅
- KVM Validation: btf (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/11252652873
Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/b00f1ec86da9
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=897104
If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:
$ cd [kernel source code]
$ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
--pull always mptcp/mptcp-upstream-virtme-docker:latest \
auto-normal
For more details:
https://github.com/multipath-tcp/mptcp-upstream-virtme-docker
Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)
Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)
On Wed, 9 Oct 2024, Geliang Tang wrote: > From: Geliang Tang <tanggeliang@kylinos.cn> > > v9: > - rename bpf prog to "mptcp_bpf_iters_subflow.c" > - drop the helper bpf_mptcp_sk(), it's not used in this test. > Hi Geliang - Series LGTM, thanks for all the updates. Reviewed-by: Mat Martineau <martineau@kernel.org> > v8: > - Make the test prog in patch 4 more like a bpf packet scheduler. > - Export more mptcp helpers into BPF in patch 1. > > v7: > - As Martin recently replied, mptcp_stubflow bpf_iter can still be > used in tracing. So go back to the tracing selftest. > > v6: > - add KF_TRUSTED_ARGS flag in patch 2 (Andrii, Martin), then acquire > and release helpers (patch 3) are needed to be implemented. > - test mptcp_subflow bpf_iter in "cgroup/getsockopt" (patch 5), > instead of testing them in "tracing" (Martin). > > v5: > - update patch 1 as Andrii suggested: if msk is NULL, initialize > kit->msk to NULL in _new() and check it in _next(). > > v4: > - squash patch 1/5 and 3/5 in v3 together as Matt suggested. > - a new squash-to patch to drop mptcp_subflow_active declaration in > bpf_burst. > > v3: > - drop bpf_iter__mptcp_subflow, __diag_push, __diag_pop and > __diag_ignore_all > - drop declarations for bpf kfuncs > > v2: > - update patch 1 as Martin and Andrii suggested. > - fix warnings and errors reported by MPTCP CI. > > This patch set adds a mptcp_subflow type bpf_iter, and self tests. > > Geliang Tang (7): > bpf: Register mptcp common kfunc set > bpf: Add mptcp_subflow bpf_iter > bpf: Add mptcp_sock acquire and release helpers > selftests/bpf: Add mptcp_subflow bpf_iter test prog > selftests/bpf: More endpoints for endpoint_init > Squash to "selftests/bpf: Add bpf scheduler test" > selftests/bpf: Add mptcp_subflow bpf_iter subtest > > net/mptcp/bpf.c | 95 ++++++++++++- > .../testing/selftests/bpf/bpf_experimental.h | 7 + > .../testing/selftests/bpf/prog_tests/mptcp.c | 126 +++++++++++++++++- > tools/testing/selftests/bpf/progs/mptcp_bpf.h | 9 ++ > .../bpf/progs/mptcp_bpf_iters_subflow.c | 42 ++++++ > 5 files changed, 267 insertions(+), 12 deletions(-) > create mode 100644 tools/testing/selftests/bpf/progs/mptcp_bpf_iters_subflow.c > > -- > 2.43.0 > > >
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? I guess you are missing __bpf_kfunc_start_defs() and __bpf_kfunc_end_defs() around the declaration of the BPF dedicated kfunc, no? Also, where should I apply these patches? Before "mptcp: add sched_data helpers"? 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) Cheers, Matt
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. 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? It seems that it is because "-Wmissing-declarations" is not recognized by sparse > > 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. > > > 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". > 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(). > > Cheers, > Matt
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
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. > > > 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. > > > 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 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. 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 > 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. Thanks, -Geliang > > Cheers, > 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? > > > 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"? 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? Thanks, -Geliang > test > in preparation to the packet scheduler, it is fine if it is not doing > interesting for the moment I think. > > Cheers, > Matt
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
On 15/10/2024 12:59, Matthieu Baerts wrote: > On 15/10/2024 11:20, Geliang Tang wrote: (...) >> 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! I just noticed Daniel sent a pull request to include bpf-net/next into net-next: it means the 3 last patches from your list will be dropped from our tree. In this case, I don't have to re-order commit, and simply apply this series before "mptcp: add sched_data helpers". Cheers, Matt
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
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
From: Geliang Tang <tanggeliang@kylinos.cn> v9: - rename bpf prog to "mptcp_bpf_iters_subflow.c" - drop the helper bpf_mptcp_sk(), it's not used in this test. v8: - Make the test prog in patch 4 more like a bpf packet scheduler. - Export more mptcp helpers into BPF in patch 1. v7: - As Martin recently replied, mptcp_stubflow bpf_iter can still be used in tracing. So go back to the tracing selftest. v6: - add KF_TRUSTED_ARGS flag in patch 2 (Andrii, Martin), then acquire and release helpers (patch 3) are needed to be implemented. - test mptcp_subflow bpf_iter in "cgroup/getsockopt" (patch 5), instead of testing them in "tracing" (Martin). v5: - update patch 1 as Andrii suggested: if msk is NULL, initialize kit->msk to NULL in _new() and check it in _next(). v4: - squash patch 1/5 and 3/5 in v3 together as Matt suggested. - a new squash-to patch to drop mptcp_subflow_active declaration in bpf_burst. v3: - drop bpf_iter__mptcp_subflow, __diag_push, __diag_pop and __diag_ignore_all - drop declarations for bpf kfuncs v2: - update patch 1 as Martin and Andrii suggested. - fix warnings and errors reported by MPTCP CI. This patch set adds a mptcp_subflow type bpf_iter, and self tests. Geliang Tang (7): bpf: Register mptcp common kfunc set bpf: Add mptcp_subflow bpf_iter bpf: Add mptcp_sock acquire and release helpers selftests/bpf: Add mptcp_subflow bpf_iter test prog selftests/bpf: More endpoints for endpoint_init Squash to "selftests/bpf: Add bpf scheduler test" selftests/bpf: Add mptcp_subflow bpf_iter subtest net/mptcp/bpf.c | 95 ++++++++++++- .../testing/selftests/bpf/bpf_experimental.h | 7 + .../testing/selftests/bpf/prog_tests/mptcp.c | 126 +++++++++++++++++- tools/testing/selftests/bpf/progs/mptcp_bpf.h | 9 ++ .../bpf/progs/mptcp_bpf_iters_subflow.c | 42 ++++++ 5 files changed, 267 insertions(+), 12 deletions(-) create mode 100644 tools/testing/selftests/bpf/progs/mptcp_bpf_iters_subflow.c