[bpf-next,v2,5/5] bpf/selftests: add simple selftest for bpf_smc_ops

Commit Message

D. Wythe Dec. 10, 2024, 4:04 a.m. UTC

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 <[e-mail]>
---
 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

Comments

Alexei Starovoitov Dec. 10, 2024, 6:01 p.m. UTC | #1

On Mon, Dec 9, 2024 at 8:04 PM D. Wythe <[e-mail]> 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

D. Wythe Dec. 11, 2024, 5:17 a.m. UTC | #2

On Tue, Dec 10, 2024 at 10:01:38AM -0800, Alexei Starovoitov wrote:
> On Mon, Dec 9, 2024 at 8:04 PM D. Wythe <[e-mail]> 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

Patch

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,
+};