| Message ID | 20241210040404.10606-6-alibuda@linux.alibaba.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | net/smc: Introduce smc_ops | expand |
On Mon, Dec 9, 2024 at 8:04 PM D. Wythe <alibuda@linux.alibaba.com> wrote: > > +SEC("struct_ops/bpf_smc_set_tcp_option_cond") > +int BPF_PROG(bpf_smc_set_tcp_option_cond, const struct tcp_sock *tp, struct inet_request_sock *ireq) > +{ > + return 0; > +} > + > +SEC("struct_ops/bpf_smc_set_tcp_option") > +int BPF_PROG(bpf_smc_set_tcp_option, struct tcp_sock *tp) > +{ > + return 1; > +} > + > +SEC(".struct_ops.link") > +struct smc_ops sample_smc_ops = { > + .name = "sample", > + .set_option = (void *) bpf_smc_set_tcp_option, > + .set_option_cond = (void *) bpf_smc_set_tcp_option_cond, > +}; These stubs don't inspire confidence that smc_ops api will be sufficient. Please implement a real bpf prog that demonstrates the actual use case. See how bpf_cubic was done. On the day one it was implemented as a parity to builtin cubic cong control. And over years we didn't need to touch tcp_congestion_ops. To be fair that api was already solid due to in-kernel cc modules, but bpf comes with its own limitations, so it wasn't a guarantee that tcp_congestion_ops would be enough. Here you're proposing a brand new smc_ops api while bpf progs are nothing but stubs. That's not sufficient to prove that api is viable long term. In terms of look and feel the smc_ops look ok. The change from v1 to v2 was a good step. pw-bot: cr
On Tue, Dec 10, 2024 at 10:01:38AM -0800, Alexei Starovoitov wrote: > On Mon, Dec 9, 2024 at 8:04 PM D. Wythe <alibuda@linux.alibaba.com> wrote: > > > > +SEC("struct_ops/bpf_smc_set_tcp_option_cond") > > +int BPF_PROG(bpf_smc_set_tcp_option_cond, const struct tcp_sock *tp, struct inet_request_sock *ireq) > > +{ > > + return 0; > > +} > > + > > +SEC("struct_ops/bpf_smc_set_tcp_option") > > +int BPF_PROG(bpf_smc_set_tcp_option, struct tcp_sock *tp) > > +{ > > + return 1; > > +} > > + > > +SEC(".struct_ops.link") > > +struct smc_ops sample_smc_ops = { > > + .name = "sample", > > + .set_option = (void *) bpf_smc_set_tcp_option, > > + .set_option_cond = (void *) bpf_smc_set_tcp_option_cond, > > +}; > > These stubs don't inspire confidence that smc_ops api > will be sufficient. > Please implement a real bpf prog that demonstrates the actual use case. > > See how bpf_cubic was done. On the day one it was implemented > as a parity to builtin cubic cong control. > And over years we didn't need to touch tcp_congestion_ops. > To be fair that api was already solid due to in-kernel cc modules, > but bpf comes with its own limitations, so it wasn't a guarantee > that tcp_congestion_ops would be enough. > Here you're proposing a brand new smc_ops api while bpf progs > are nothing but stubs. That's not sufficient to prove that api > is viable long term. Hi Alexei, Thanks a lot for your advices. I will add actual cases in the next version to prove why we need it. > > In terms of look and feel the smc_ops look ok. > The change from v1 to v2 was a good step. I'm glad that you feel it looks okay. If you have any questions, please let me know. Thanks, D. Wythe > > pw-bot: cr
diff --git a/tools/testing/selftests/bpf/config b/tools/testing/selftests/bpf/config index c378d5d07e02..99f1cf10475f 100644 --- a/tools/testing/selftests/bpf/config +++ b/tools/testing/selftests/bpf/config @@ -113,3 +113,6 @@ CONFIG_XDP_SOCKETS=y CONFIG_XFRM_INTERFACE=y CONFIG_TCP_CONG_DCTCP=y CONFIG_TCP_CONG_BBR=y +CONFIG_INFINIBAND=m +CONFIG_SMC=m +CONFIG_SMC_OPS=y \ No newline at end of file diff --git a/tools/testing/selftests/bpf/prog_tests/test_bpf_smc.c b/tools/testing/selftests/bpf/prog_tests/test_bpf_smc.c new file mode 100644 index 000000000000..b24da7e8db66 --- /dev/null +++ b/tools/testing/selftests/bpf/prog_tests/test_bpf_smc.c @@ -0,0 +1,25 @@ +// SPDX-License-Identifier: GPL-2.0 +#include <test_progs.h> + +#include "bpf_smc.skel.h" + +static void load(void) +{ + struct bpf_smc *skel; + int ret; + + skel = bpf_smc__open_and_load(); + if (!ASSERT_OK_PTR(skel, "bpf_smc__open_and_load")) + return; + + ret = bpf_smc__attach(skel); + ASSERT_EQ(ret, 0, "bpf_smc__attach"); + + bpf_smc__destroy(skel); +} + +void test_bpf_smc(void) +{ + if (test__start_subtest("load")) + load(); +} diff --git a/tools/testing/selftests/bpf/progs/bpf_smc.c b/tools/testing/selftests/bpf/progs/bpf_smc.c new file mode 100644 index 000000000000..32d15596f209 --- /dev/null +++ b/tools/testing/selftests/bpf/progs/bpf_smc.c @@ -0,0 +1,27 @@ +// SPDX-License-Identifier: GPL-2.0 + +#include "vmlinux.h" + +#include <bpf/bpf_helpers.h> +#include <bpf/bpf_tracing.h> + +char _license[] SEC("license") = "GPL"; + +SEC("struct_ops/bpf_smc_set_tcp_option_cond") +int BPF_PROG(bpf_smc_set_tcp_option_cond, const struct tcp_sock *tp, struct inet_request_sock *ireq) +{ + return 0; +} + +SEC("struct_ops/bpf_smc_set_tcp_option") +int BPF_PROG(bpf_smc_set_tcp_option, struct tcp_sock *tp) +{ + return 1; +} + +SEC(".struct_ops.link") +struct smc_ops sample_smc_ops = { + .name = "sample", + .set_option = (void *) bpf_smc_set_tcp_option, + .set_option_cond = (void *) bpf_smc_set_tcp_option_cond, +};
This PATCH adds a tiny selftest for bpf_smc_ops, to verify the ability to load and attach. Follow the steps below to run this test. make -C tools/testing/selftests/bpf cd tools/testing/selftests/bpf sudo ./test_progs -t smc Results shows: Summary: 1/1 PASSED, 0 SKIPPED, 0 FAILED Signed-off-by: D. Wythe <alibuda@linux.alibaba.com> --- tools/testing/selftests/bpf/config | 3 +++ .../selftests/bpf/prog_tests/test_bpf_smc.c | 25 +++++++++++++++++ tools/testing/selftests/bpf/progs/bpf_smc.c | 27 +++++++++++++++++++ 3 files changed, 55 insertions(+) create mode 100644 tools/testing/selftests/bpf/prog_tests/test_bpf_smc.c create mode 100644 tools/testing/selftests/bpf/progs/bpf_smc.c