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

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

[Geliang Tang]

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

-- 
2.43.0

[PATCH mptcp-next v9 1/7] bpf: Register mptcp common kfunc set

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

Helpers mptcp_subflow_ctx() and mptcp_subflow_tcp_sock() are useful in
MPTCP BPF programs. This patch defines corresponding wrappers of them,
and put the wrappers into mptcp common kfunc set and register the set
with BPF_PROG_TYPE_UNSPEC.

Two other kfuncs mptcp_subflow_active() and mptcp_subflow_set_scheduled()
are going to be used in mptcp_subflow bpf_iter selftest, put them into
mptcp common kfunc set too.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 net/mptcp/bpf.c | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c
index 6414824402e6..71ab1efeed40 100644
--- a/net/mptcp/bpf.c
+++ b/net/mptcp/bpf.c
@@ -205,6 +205,18 @@ __diag_push();
 __diag_ignore_all("-Wmissing-prototypes",
 		  "kfuncs which will be used in BPF programs");
 
+__bpf_kfunc struct mptcp_subflow_context *
+bpf_mptcp_subflow_ctx(const struct sock *sk)
+{
+	return mptcp_subflow_ctx(sk);
+}
+
+__bpf_kfunc struct sock *
+bpf_mptcp_subflow_tcp_sock(const struct mptcp_subflow_context *subflow)
+{
+	return mptcp_subflow_tcp_sock(subflow);
+}
+
 __bpf_kfunc struct mptcp_subflow_context *
 bpf_mptcp_subflow_ctx_by_pos(const struct mptcp_sched_data *data, unsigned int pos)
 {
@@ -220,10 +232,20 @@ __bpf_kfunc bool bpf_mptcp_subflow_queues_empty(struct sock *sk)
 
 __diag_pop();
 
-BTF_KFUNCS_START(bpf_mptcp_sched_kfunc_ids)
+BTF_KFUNCS_START(bpf_mptcp_common_kfunc_ids)
+BTF_ID_FLAGS(func, bpf_mptcp_subflow_ctx)
+BTF_ID_FLAGS(func, bpf_mptcp_subflow_tcp_sock)
 BTF_ID_FLAGS(func, mptcp_subflow_set_scheduled)
-BTF_ID_FLAGS(func, bpf_mptcp_subflow_ctx_by_pos)
 BTF_ID_FLAGS(func, mptcp_subflow_active)
+BTF_KFUNCS_END(bpf_mptcp_common_kfunc_ids)
+
+static const struct btf_kfunc_id_set bpf_mptcp_common_kfunc_set = {
+	.owner	= THIS_MODULE,
+	.set	= &bpf_mptcp_common_kfunc_ids,
+};
+
+BTF_KFUNCS_START(bpf_mptcp_sched_kfunc_ids)
+BTF_ID_FLAGS(func, bpf_mptcp_subflow_ctx_by_pos)
 BTF_ID_FLAGS(func, mptcp_set_timeout)
 BTF_ID_FLAGS(func, mptcp_wnd_end)
 BTF_ID_FLAGS(func, tcp_stream_memory_free)
@@ -241,6 +263,8 @@ static int __init bpf_mptcp_kfunc_init(void)
 	int ret;
 
 	ret = register_btf_fmodret_id_set(&bpf_mptcp_fmodret_set);
+	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_UNSPEC,
+					       &bpf_mptcp_common_kfunc_set);
 	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_STRUCT_OPS,
 					       &bpf_mptcp_sched_kfunc_set);
 #ifdef CONFIG_BPF_JIT
-- 
2.43.0

[PATCH mptcp-next v9 2/7] bpf: Add mptcp_subflow bpf_iter

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

It's necessary to traverse all subflows on the conn_list of an MPTCP
socket and then call kfunc to modify the fields of each subflow. In
kernel space, mptcp_for_each_subflow() helper is used for this:

	mptcp_for_each_subflow(msk, subflow)
		kfunc(subflow);

But in the MPTCP BPF program, this has not yet been implemented. As
Martin suggested recently, this conn_list walking + modify-by-kfunc
usage fits the bpf_iter use case.

So this patch adds a new bpf_iter type named "mptcp_subflow" to do
this and implements its helpers bpf_iter_mptcp_subflow_new()/_next()/
_destroy(). And register these bpf_iter mptcp_subflow into mptcp
common kfunc set. Then bpf_for_each() for mptcp_subflow can be used
in BPF program like this:

	bpf_for_each(mptcp_subflow, subflow, msk)
		kfunc(subflow);

v2: remove msk->pm.lock in _new() and _destroy() (Martin)
    drop DEFINE_BPF_ITER_FUNC, change opaque[3] to opaque[2] (Andrii)
v3: drop bpf_iter__mptcp_subflow
v4: if msk is NULL, initialize kit->msk to NULL in _new() and check it in
    _next() (Andrii)
v5: use list_is_last() instead of list_entry_is_head()
    add KF_ITER_NEW/NEXT/DESTROY flags
    add msk_owned_by_me in _new()
v6: add KF_TRUSTED_ARGS flag (Andrii, Martin)

"Suggested-by: Martin KaFai Lau <martin.lau@kernel.org>"
Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 net/mptcp/bpf.c | 49 +++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 45 insertions(+), 4 deletions(-)

diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c
index 71ab1efeed40..fe2ee03ff1ee 100644
--- a/net/mptcp/bpf.c
+++ b/net/mptcp/bpf.c
@@ -201,9 +201,16 @@ static const struct btf_kfunc_id_set bpf_mptcp_fmodret_set = {
 	.set   = &bpf_mptcp_fmodret_ids,
 };
 
-__diag_push();
-__diag_ignore_all("-Wmissing-prototypes",
-		  "kfuncs which will be used in BPF programs");
+struct bpf_iter_mptcp_subflow {
+	__u64 __opaque[2];
+} __attribute__((aligned(8)));
+
+struct bpf_iter_mptcp_subflow_kern {
+	struct mptcp_sock *msk;
+	struct list_head *pos;
+} __attribute__((aligned(8)));
+
+__bpf_kfunc_start_defs();
 
 __bpf_kfunc struct mptcp_subflow_context *
 bpf_mptcp_subflow_ctx(const struct sock *sk)
@@ -217,6 +224,37 @@ bpf_mptcp_subflow_tcp_sock(const struct mptcp_subflow_context *subflow)
 	return mptcp_subflow_tcp_sock(subflow);
 }
 
+__bpf_kfunc int bpf_iter_mptcp_subflow_new(struct bpf_iter_mptcp_subflow *it,
+					   struct mptcp_sock *msk)
+{
+	struct bpf_iter_mptcp_subflow_kern *kit = (void *)it;
+
+	kit->msk = msk;
+	if (!msk)
+		return -EINVAL;
+
+	msk_owned_by_me(msk);
+
+	kit->pos = &msk->conn_list;
+	return 0;
+}
+
+__bpf_kfunc struct mptcp_subflow_context *
+bpf_iter_mptcp_subflow_next(struct bpf_iter_mptcp_subflow *it)
+{
+	struct bpf_iter_mptcp_subflow_kern *kit = (void *)it;
+
+	if (!kit->msk || list_is_last(kit->pos, &kit->msk->conn_list))
+		return NULL;
+
+	kit->pos = kit->pos->next;
+	return list_entry(kit->pos, struct mptcp_subflow_context, node);
+}
+
+__bpf_kfunc void bpf_iter_mptcp_subflow_destroy(struct bpf_iter_mptcp_subflow *it)
+{
+}
+
 __bpf_kfunc struct mptcp_subflow_context *
 bpf_mptcp_subflow_ctx_by_pos(const struct mptcp_sched_data *data, unsigned int pos)
 {
@@ -230,13 +268,16 @@ __bpf_kfunc bool bpf_mptcp_subflow_queues_empty(struct sock *sk)
 	return tcp_rtx_queue_empty(sk);
 }
 
-__diag_pop();
+__bpf_kfunc_end_defs();
 
 BTF_KFUNCS_START(bpf_mptcp_common_kfunc_ids)
 BTF_ID_FLAGS(func, bpf_mptcp_subflow_ctx)
 BTF_ID_FLAGS(func, bpf_mptcp_subflow_tcp_sock)
 BTF_ID_FLAGS(func, mptcp_subflow_set_scheduled)
 BTF_ID_FLAGS(func, mptcp_subflow_active)
+BTF_ID_FLAGS(func, bpf_iter_mptcp_subflow_new, KF_ITER_NEW | KF_TRUSTED_ARGS)
+BTF_ID_FLAGS(func, bpf_iter_mptcp_subflow_next, KF_ITER_NEXT | KF_RET_NULL)
+BTF_ID_FLAGS(func, bpf_iter_mptcp_subflow_destroy, KF_ITER_DESTROY)
 BTF_KFUNCS_END(bpf_mptcp_common_kfunc_ids)
 
 static const struct btf_kfunc_id_set bpf_mptcp_common_kfunc_set = {
-- 
2.43.0

[PATCH mptcp-next v9 3/7] bpf: Add mptcp_sock acquire and release helpers

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

KF_TRUSTED_ARGS flag is used for bpf_iter_mptcp_subflow_new, it indicates
that the all pointer arguments are valid. It's necessary to add a
KF_ACQUIRE helper to get valid "msk".

This patch adds bpf_mptcp_sock_acquire() and bpf_mptcp_sock_release()
helpers for this. Increase sk->sk_refcnt in _acquire() and decrease it
in _release(). Register them with KF_ACQUIRE flag and KF_RELEASE flag.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 net/mptcp/bpf.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c
index fe2ee03ff1ee..9b87eee13955 100644
--- a/net/mptcp/bpf.c
+++ b/net/mptcp/bpf.c
@@ -255,6 +255,22 @@ __bpf_kfunc void bpf_iter_mptcp_subflow_destroy(struct bpf_iter_mptcp_subflow *i
 {
 }
 
+__bpf_kfunc struct mptcp_sock *bpf_mptcp_sock_acquire(struct mptcp_sock *msk)
+{
+	struct sock *sk = (struct sock *)msk;
+
+	if (sk && refcount_inc_not_zero(&sk->sk_refcnt))
+		return msk;
+	return NULL;
+}
+
+__bpf_kfunc void bpf_mptcp_sock_release(struct mptcp_sock *msk)
+{
+	struct sock *sk = (struct sock *)msk;
+
+	WARN_ON_ONCE(!sk || !refcount_dec_not_one(&sk->sk_refcnt));
+}
+
 __bpf_kfunc struct mptcp_subflow_context *
 bpf_mptcp_subflow_ctx_by_pos(const struct mptcp_sched_data *data, unsigned int pos)
 {
@@ -278,6 +294,8 @@ BTF_ID_FLAGS(func, mptcp_subflow_active)
 BTF_ID_FLAGS(func, bpf_iter_mptcp_subflow_new, KF_ITER_NEW | KF_TRUSTED_ARGS)
 BTF_ID_FLAGS(func, bpf_iter_mptcp_subflow_next, KF_ITER_NEXT | KF_RET_NULL)
 BTF_ID_FLAGS(func, bpf_iter_mptcp_subflow_destroy, KF_ITER_DESTROY)
+BTF_ID_FLAGS(func, bpf_mptcp_sock_acquire, KF_ACQUIRE | KF_RET_NULL)
+BTF_ID_FLAGS(func, bpf_mptcp_sock_release, KF_RELEASE)
 BTF_KFUNCS_END(bpf_mptcp_common_kfunc_ids)
 
 static const struct btf_kfunc_id_set bpf_mptcp_common_kfunc_set = {
-- 
2.43.0

[PATCH mptcp-next v9 4/7] selftests/bpf: Add mptcp_subflow bpf_iter test prog

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

This patch adds a ftrace hook for mptcp_sched_get_send() to test the newly
added mptcp_subflow bpf_iter. This test simulates a typical mptcp packet
scheduler, which selects a subflow from multiple subflows of an mptcp
socket to send data.

Export mptcp_subflow helpers bpf_iter_mptcp_subflow_new/_next/_destroy,
bpf_mptcp_sock_acquire/_release and other helpers into bpf_experimental.h.

Use _acquire() to acquire the msk, then use bpf_for_each(mptcp_subflow) to
walk the subflow list of this msk. Invoke kfuncs mptcp_subflow_active() and
bpf_mptcp_subflow_tcp_sock() in the loop to pick a subsocket. Finally use
bpf_mptcp_subflow_ctx() to get the subflow context of this subsocket and
use mptcp_subflow_set_scheduled() to set it as being scheduled.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 .../testing/selftests/bpf/bpf_experimental.h  |  7 ++++
 tools/testing/selftests/bpf/progs/mptcp_bpf.h |  9 ++++
 .../bpf/progs/mptcp_bpf_iters_subflow.c       | 42 +++++++++++++++++++
 3 files changed, 58 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/progs/mptcp_bpf_iters_subflow.c

diff --git a/tools/testing/selftests/bpf/bpf_experimental.h b/tools/testing/selftests/bpf/bpf_experimental.h
index b0668f29f7b3..d43690b17468 100644
--- a/tools/testing/selftests/bpf/bpf_experimental.h
+++ b/tools/testing/selftests/bpf/bpf_experimental.h
@@ -575,6 +575,13 @@ extern int bpf_iter_css_new(struct bpf_iter_css *it,
 extern struct cgroup_subsys_state *bpf_iter_css_next(struct bpf_iter_css *it) __weak __ksym;
 extern void bpf_iter_css_destroy(struct bpf_iter_css *it) __weak __ksym;
 
+struct bpf_iter_mptcp_subflow;
+extern int bpf_iter_mptcp_subflow_new(struct bpf_iter_mptcp_subflow *it,
+				      struct mptcp_sock *msk) __weak __ksym;
+extern struct mptcp_subflow_context *
+bpf_iter_mptcp_subflow_next(struct bpf_iter_mptcp_subflow *it) __weak __ksym;
+extern void bpf_iter_mptcp_subflow_destroy(struct bpf_iter_mptcp_subflow *it) __weak __ksym;
+
 extern int bpf_wq_init(struct bpf_wq *wq, void *p__map, unsigned int flags) __weak __ksym;
 extern int bpf_wq_start(struct bpf_wq *wq, unsigned int flags) __weak __ksym;
 extern int bpf_wq_set_callback_impl(struct bpf_wq *wq,
diff --git a/tools/testing/selftests/bpf/progs/mptcp_bpf.h b/tools/testing/selftests/bpf/progs/mptcp_bpf.h
index c3800f986ae1..e18796361394 100644
--- a/tools/testing/selftests/bpf/progs/mptcp_bpf.h
+++ b/tools/testing/selftests/bpf/progs/mptcp_bpf.h
@@ -43,9 +43,18 @@ mptcp_subflow_tcp_sock(const struct mptcp_subflow_context *subflow)
 }
 
 /* ksym */
+extern bool mptcp_subflow_active(struct mptcp_subflow_context *subflow) __ksym;
 extern void mptcp_subflow_set_scheduled(struct mptcp_subflow_context *subflow,
 					bool scheduled) __ksym;
 
+extern struct mptcp_sock *bpf_mptcp_sock_acquire(struct mptcp_sock *msk) __ksym;
+extern void bpf_mptcp_sock_release(struct mptcp_sock *msk) __ksym;
+
+extern struct mptcp_subflow_context *
+bpf_mptcp_subflow_ctx(const struct sock *sk) __ksym;
+extern struct sock *
+bpf_mptcp_subflow_tcp_sock(const struct mptcp_subflow_context *subflow) __ksym;
+
 extern struct mptcp_subflow_context *
 bpf_mptcp_subflow_ctx_by_pos(const struct mptcp_sched_data *data, unsigned int pos) __ksym;
 
diff --git a/tools/testing/selftests/bpf/progs/mptcp_bpf_iters_subflow.c b/tools/testing/selftests/bpf/progs/mptcp_bpf_iters_subflow.c
new file mode 100644
index 000000000000..4268e4604c5a
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/mptcp_bpf_iters_subflow.c
@@ -0,0 +1,42 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2024, Kylin Software */
+
+/* vmlinux.h, bpf_helpers.h and other 'define' */
+#include "bpf_tracing_net.h"
+#include "mptcp_bpf.h"
+
+char _license[] SEC("license") = "GPL";
+int subflows;
+int pid;
+
+SEC("fentry/mptcp_sched_get_send")
+int BPF_PROG(trace_mptcp_sched_get_send, struct mptcp_sock *msk)
+{
+	struct mptcp_subflow_context *subflow;
+	struct sock *ssk = NULL;
+
+	if (bpf_get_current_pid_tgid() >> 32 != pid)
+		return 0;
+
+	msk = bpf_mptcp_sock_acquire(msk);
+	if (!msk)
+		return 0;
+	bpf_for_each(mptcp_subflow, subflow, msk) {
+		if (subflow->token != msk->token)
+			break;
+
+		if (!mptcp_subflow_active(subflow))
+			continue;
+
+		ssk = bpf_mptcp_subflow_tcp_sock(subflow);
+	}
+	bpf_mptcp_sock_release(msk);
+
+	if (!ssk)
+		return 0;
+	subflow = bpf_mptcp_subflow_ctx(ssk);
+	mptcp_subflow_set_scheduled(subflow, true);
+	subflows = subflow->subflow_id;
+
+	return 0;
+}
-- 
2.43.0

[PATCH mptcp-next v9 5/7] selftests/bpf: More endpoints for endpoint_init

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

This patch adds two more test addresses ADDR_3 and ADDR_4, and adds a new
helper address_init() to initialize all these addresses.

Add a new parameter "endpoints" for endpoint_init() to control how many
endpoints are used for the tests. This makes it more flexible. Update the
parameters of endpoint_init() in test_subflow().

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 .../testing/selftests/bpf/prog_tests/mptcp.c  | 46 +++++++++++++++++--
 1 file changed, 41 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/mptcp.c b/tools/testing/selftests/bpf/prog_tests/mptcp.c
index acd79be134cd..076903680e75 100644
--- a/tools/testing/selftests/bpf/prog_tests/mptcp.c
+++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c
@@ -20,6 +20,8 @@
 #define NS_TEST "mptcp_ns"
 #define ADDR_1	"10.0.1.1"
 #define ADDR_2	"10.0.1.2"
+#define ADDR_3	"10.0.1.3"
+#define ADDR_4	"10.0.1.4"
 #define PORT_1	10001
 #define WITH_DATA	true
 #define WITHOUT_DATA	false
@@ -351,22 +353,56 @@ static void test_mptcpify(void)
 	close(cgroup_fd);
 }
 
-static int endpoint_init(char *flags)
+static int address_init(void)
 {
 	SYS(fail, "ip -net %s link add veth1 type veth peer name veth2", NS_TEST);
 	SYS(fail, "ip -net %s addr add %s/24 dev veth1", NS_TEST, ADDR_1);
 	SYS(fail, "ip -net %s link set dev veth1 up", NS_TEST);
 	SYS(fail, "ip -net %s addr add %s/24 dev veth2", NS_TEST, ADDR_2);
 	SYS(fail, "ip -net %s link set dev veth2 up", NS_TEST);
-	if (SYS_NOFAIL("ip -net %s mptcp endpoint add %s %s", NS_TEST, ADDR_2, flags)) {
+
+	SYS(fail, "ip -net %s link add veth3 type veth peer name veth4", NS_TEST);
+	SYS(fail, "ip -net %s addr add %s/24 dev veth3", NS_TEST, ADDR_3);
+	SYS(fail, "ip -net %s link set dev veth3 up", NS_TEST);
+	SYS(fail, "ip -net %s addr add %s/24 dev veth4", NS_TEST, ADDR_4);
+	SYS(fail, "ip -net %s link set dev veth4 up", NS_TEST);
+
+	return 0;
+fail:
+	return -1;
+}
+
+static int endpoint_add(char *addr, char *flags)
+{
+	return SYS_NOFAIL("ip -net %s mptcp endpoint add %s %s", NS_TEST, addr, flags);
+}
+
+static int endpoint_init(char *flags, u8 endpoints)
+{
+	int ret = -1;
+
+	if (!endpoints || endpoints > 4)
+		goto fail;
+
+	if (address_init())
+		goto fail;
+
+	if (SYS_NOFAIL("ip -net %s mptcp limits set add_addr_accepted 4 subflows 4",
+		       NS_TEST)) {
 		printf("'ip mptcp' not supported, skip this test.\n");
 		test__skip();
 		goto fail;
 	}
 
-	return 0;
+	if (endpoints > 1)
+		ret = endpoint_add(ADDR_2, flags);
+	if (endpoints > 2)
+		ret = ret ?: endpoint_add(ADDR_3, flags);
+	if (endpoints > 3)
+		ret = ret ?: endpoint_add(ADDR_4, flags);
+
 fail:
-	return -1;
+	return ret;
 }
 
 static void wait_for_new_subflows(int fd)
@@ -452,7 +488,7 @@ static void test_subflow(void)
 	if (!ASSERT_OK_PTR(nstoken, "create_netns: mptcp_subflow"))
 		goto skel_destroy;
 
-	if (endpoint_init("subflow") < 0)
+	if (endpoint_init("subflow", 2) < 0)
 		goto close_netns;
 
 	run_subflow();
-- 
2.43.0

[PATCH mptcp-next v9 6/7] Squash to "selftests/bpf: Add bpf scheduler test"

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

Update endpoint_init() in sched_init().

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 tools/testing/selftests/bpf/prog_tests/mptcp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/mptcp.c b/tools/testing/selftests/bpf/prog_tests/mptcp.c
index 076903680e75..64b5ce25e1a1 100644
--- a/tools/testing/selftests/bpf/prog_tests/mptcp.c
+++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c
@@ -509,7 +509,7 @@ static struct nstoken *sched_init(char *flags, char *sched)
 	if (!ASSERT_OK_PTR(nstoken, "create_netns"))
 		return NULL;
 
-	if (endpoint_init("subflow") < 0)
+	if (endpoint_init("subflow", 2) < 0)
 		goto fail;
 
 	SYS(fail, "ip netns exec %s sysctl -qw net.mptcp.scheduler=%s", NS_TEST, sched);
-- 
2.43.0

[PATCH mptcp-next v9 7/7] selftests/bpf: Add mptcp_subflow bpf_iter subtest

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

This patch adds a subtest named test_iters_subflow to load and verify the
newly added mptcp_subflow type bpf_iter example in test_mptcp. Use the
helper endpoint_init() to add 3 new subflow endpoints. Send a byte of
message to start the mptcp connection, a new helper recv_byte() is added
to receive this message. send_byte() is invoked again to trigger the ftrace
hook for mptcp_sched_get_send() after wait_for_new_subflows(). Check if
skel->bss->subflows equals 4 to verify whether the "fentry" program loops
fourth as expected.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 .../testing/selftests/bpf/prog_tests/mptcp.c  | 78 +++++++++++++++++++
 1 file changed, 78 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/mptcp.c b/tools/testing/selftests/bpf/prog_tests/mptcp.c
index 64b5ce25e1a1..589420f42845 100644
--- a/tools/testing/selftests/bpf/prog_tests/mptcp.c
+++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c
@@ -11,6 +11,7 @@
 #include "mptcp_sock.skel.h"
 #include "mptcpify.skel.h"
 #include "mptcp_subflow.skel.h"
+#include "mptcp_bpf_iters_subflow.skel.h"
 #include "mptcp_bpf_first.skel.h"
 #include "mptcp_bpf_bkup.skel.h"
 #include "mptcp_bpf_rr.skel.h"
@@ -261,6 +262,19 @@ static void send_byte(int fd)
 	ASSERT_EQ(write(fd, &b, sizeof(b)), 1, "send single byte");
 }
 
+static int recv_byte(int fd)
+{
+	char buf[1];
+	ssize_t n;
+
+	n = recv(fd, buf, sizeof(buf), 0);
+	if (CHECK(n <= 0, "recv_byte", "recv")) {
+		log_err("failed/partial recv");
+		return -1;
+	}
+	return 0;
+}
+
 static int verify_mptcpify(int server_fd, int client_fd)
 {
 	struct __mptcp_info info;
@@ -501,6 +515,68 @@ static void test_subflow(void)
 	close(cgroup_fd);
 }
 
+static void run_iters_subflow(void)
+{
+	int server_fd, client_fd, accept_fd;
+
+	server_fd = start_mptcp_server(AF_INET, ADDR_1, PORT_1, 0);
+	if (!ASSERT_OK_FD(server_fd, "start_mptcp_server"))
+		return;
+
+	client_fd = connect_to_fd(server_fd, 0);
+	if (!ASSERT_OK_FD(client_fd, "connect_to_fd"))
+		goto close_server;
+
+	accept_fd = accept(server_fd, NULL, NULL);
+	if (!ASSERT_OK_FD(accept_fd, "accept"))
+		goto close_client;
+
+	send_byte(client_fd);
+	wait_for_new_subflows(client_fd);
+	recv_byte(accept_fd);
+	send_byte(client_fd);
+
+	close(accept_fd);
+close_client:
+	close(client_fd);
+close_server:
+	close(server_fd);
+}
+
+static void test_iters_subflow(void)
+{
+	struct mptcp_bpf_iters_subflow *skel;
+	struct nstoken *nstoken;
+	int subflows = 4;
+	int err;
+
+	skel = mptcp_bpf_iters_subflow__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "skel_open_load: iters_subflow"))
+		return;
+
+	skel->bss->pid = getpid();
+
+	err = mptcp_bpf_iters_subflow__attach(skel);
+	if (!ASSERT_OK(err, "skel_attach: iters_subflow"))
+		goto skel_destroy;
+
+	nstoken = create_netns();
+	if (!ASSERT_OK_PTR(nstoken, "create_netns: iters_subflow"))
+		goto skel_destroy;
+
+	if (endpoint_init("subflow", subflows) < 0)
+		goto close_netns;
+
+	run_iters_subflow();
+
+	ASSERT_EQ(skel->bss->subflows, subflows, "subflows");
+
+close_netns:
+	cleanup_netns(nstoken);
+skel_destroy:
+	mptcp_bpf_iters_subflow__destroy(skel);
+}
+
 static struct nstoken *sched_init(char *flags, char *sched)
 {
 	struct nstoken *nstoken;
@@ -682,6 +758,8 @@ void test_mptcp(void)
 		test_mptcpify();
 	if (test__start_subtest("subflow"))
 		test_subflow();
+	if (test__start_subtest("iters_subflow"))
+		test_iters_subflow();
 	if (test__start_subtest("default"))
 		test_default();
 	if (test__start_subtest("first"))
-- 
2.43.0

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

[MPTCP CI]

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)

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

[MPTCP CI]

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)

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

[Mat Martineau]

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

Re: [PATCH mptcp-next v9 4/7] selftests/bpf: Add mptcp_subflow bpf_iter test prog

[Matthieu Baerts]

Hi Geliang,

On 09/10/2024 11:45, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> This patch adds a ftrace hook for mptcp_sched_get_send() to test the newly
> added mptcp_subflow bpf_iter. This test simulates a typical mptcp packet
> scheduler, which selects a subflow from multiple subflows of an mptcp
> socket to send data.
> 
> Export mptcp_subflow helpers bpf_iter_mptcp_subflow_new/_next/_destroy,
> bpf_mptcp_sock_acquire/_release and other helpers into bpf_experimental.h.
> 
> Use _acquire() to acquire the msk, then use bpf_for_each(mptcp_subflow) to
> walk the subflow list of this msk. Invoke kfuncs mptcp_subflow_active() and
> bpf_mptcp_subflow_tcp_sock() in the loop to pick a subsocket. Finally use
> bpf_mptcp_subflow_ctx() to get the subflow context of this subsocket and
> use mptcp_subflow_set_scheduled() to set it as being scheduled.

Do you think we could have a test not depending on scheduler helpers?
Without this dependence, we could already upstream this series, and get
feedback without having to wait for the new scheduler API.

If you remove the use of mptcp_subflow_active() and
mptcp_subflow_set_scheduled(), that's enough, no?

> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  .../testing/selftests/bpf/bpf_experimental.h  |  7 ++++
>  tools/testing/selftests/bpf/progs/mptcp_bpf.h |  9 ++++
>  .../bpf/progs/mptcp_bpf_iters_subflow.c       | 42 +++++++++++++++++++
>  3 files changed, 58 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/progs/mptcp_bpf_iters_subflow.c
> 
> diff --git a/tools/testing/selftests/bpf/bpf_experimental.h b/tools/testing/selftests/bpf/bpf_experimental.h
> index b0668f29f7b3..d43690b17468 100644
> --- a/tools/testing/selftests/bpf/bpf_experimental.h
> +++ b/tools/testing/selftests/bpf/bpf_experimental.h
> @@ -575,6 +575,13 @@ extern int bpf_iter_css_new(struct bpf_iter_css *it,
>  extern struct cgroup_subsys_state *bpf_iter_css_next(struct bpf_iter_css *it) __weak __ksym;
>  extern void bpf_iter_css_destroy(struct bpf_iter_css *it) __weak __ksym;
>  
> +struct bpf_iter_mptcp_subflow;
> +extern int bpf_iter_mptcp_subflow_new(struct bpf_iter_mptcp_subflow *it,
> +				      struct mptcp_sock *msk) __weak __ksym;
> +extern struct mptcp_subflow_context *
> +bpf_iter_mptcp_subflow_next(struct bpf_iter_mptcp_subflow *it) __weak __ksym;
> +extern void bpf_iter_mptcp_subflow_destroy(struct bpf_iter_mptcp_subflow *it) __weak __ksym;
> +
>  extern int bpf_wq_init(struct bpf_wq *wq, void *p__map, unsigned int flags) __weak __ksym;
>  extern int bpf_wq_start(struct bpf_wq *wq, unsigned int flags) __weak __ksym;
>  extern int bpf_wq_set_callback_impl(struct bpf_wq *wq,
> diff --git a/tools/testing/selftests/bpf/progs/mptcp_bpf.h b/tools/testing/selftests/bpf/progs/mptcp_bpf.h
> index c3800f986ae1..e18796361394 100644
> --- a/tools/testing/selftests/bpf/progs/mptcp_bpf.h
> +++ b/tools/testing/selftests/bpf/progs/mptcp_bpf.h
> @@ -43,9 +43,18 @@ mptcp_subflow_tcp_sock(const struct mptcp_subflow_context *subflow)
>  }
>  
>  /* ksym */
> +extern bool mptcp_subflow_active(struct mptcp_subflow_context *subflow) __ksym;
>  extern void mptcp_subflow_set_scheduled(struct mptcp_subflow_context *subflow,
>  					bool scheduled) __ksym;
>  
> +extern struct mptcp_sock *bpf_mptcp_sock_acquire(struct mptcp_sock *msk) __ksym;
> +extern void bpf_mptcp_sock_release(struct mptcp_sock *msk) __ksym;
> +
> +extern struct mptcp_subflow_context *
> +bpf_mptcp_subflow_ctx(const struct sock *sk) __ksym;
> +extern struct sock *
> +bpf_mptcp_subflow_tcp_sock(const struct mptcp_subflow_context *subflow) __ksym;
> +
>  extern struct mptcp_subflow_context *
>  bpf_mptcp_subflow_ctx_by_pos(const struct mptcp_sched_data *data, unsigned int pos) __ksym;
>  
> diff --git a/tools/testing/selftests/bpf/progs/mptcp_bpf_iters_subflow.c b/tools/testing/selftests/bpf/progs/mptcp_bpf_iters_subflow.c
> new file mode 100644
> index 000000000000..4268e4604c5a
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/mptcp_bpf_iters_subflow.c
> @@ -0,0 +1,42 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2024, Kylin Software */
> +
> +/* vmlinux.h, bpf_helpers.h and other 'define' */
> +#include "bpf_tracing_net.h"
> +#include "mptcp_bpf.h"
> +
> +char _license[] SEC("license") = "GPL";
> +int subflows;
> +int pid;
> +
> +SEC("fentry/mptcp_sched_get_send")

If I understand correctly, this hook will be called twice on the client
connection for this test because the client is sending two times one
byte, right?

> +int BPF_PROG(trace_mptcp_sched_get_send, struct mptcp_sock *msk)
> +{
> +	struct mptcp_subflow_context *subflow;
> +	struct sock *ssk = NULL;
> +
> +	if (bpf_get_current_pid_tgid() >> 32 != pid)
> +		return 0;
> +
> +	msk = bpf_mptcp_sock_acquire(msk);
> +	if (!msk)
> +		return 0;
> +	bpf_for_each(mptcp_subflow, subflow, msk) {
> +		if (subflow->token != msk->token)
> +			break;

Out of curiosity, why is this needed? Is it needed for the verifier? Or
just an extra check?
Can you add a comment here explaining why this is there please?

> +
> +		if (!mptcp_subflow_active(subflow))
> +			continue;
> +
> +		ssk = bpf_mptcp_subflow_tcp_sock(subflow);

Do you get 'ssk' just to use bpf_mptcp_subflow_tcp_sock() and
bpf_mptcp_subflow_ctx()?

> +	}
> +	bpf_mptcp_sock_release(msk);
> +
> +	if (!ssk)
> +		return 0;
> +	subflow = bpf_mptcp_subflow_ctx(ssk);
> +	mptcp_subflow_set_scheduled(subflow, true);
> +	subflows = subflow->subflow_id;

Here, it looks like you only check if the last subflow is in the list.

Would it not be better to count the number of subflows in the list? Or
if you want to read something from 'subflow', you could add all subflow_id?

  subflows = 0;

  (...)

  bpf_for_each(mptcp_subflow, subflow, msk)
      subflows += subflow->subflow_id;

By doing that, we will catch if one is missing, and the order is not
important.

If still want to use bpf_mptcp_subflow_tcp_sock() and
bpf_mptcp_subflow_ctx(), maybe you can save other fields from the last
subflow: the token to compare it with the one in the msk? Or the port
number or the address?

> +
> +	return 0;
> +}

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

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

[Matthieu Baerts]

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
-- 
Sponsored by the NGI0 Core fund.

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

[Geliang Tang]

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

Re: [PATCH mptcp-next v9 4/7] selftests/bpf: Add mptcp_subflow bpf_iter test prog

[Geliang Tang]

On Mon, 2024-10-14 at 18:06 +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 09/10/2024 11:45, Geliang Tang wrote:
> > From: Geliang Tang <tanggeliang@kylinos.cn>
> > 
> > This patch adds a ftrace hook for mptcp_sched_get_send() to test
> > the newly
> > added mptcp_subflow bpf_iter. This test simulates a typical mptcp
> > packet
> > scheduler, which selects a subflow from multiple subflows of an
> > mptcp
> > socket to send data.
> > 
> > Export mptcp_subflow helpers
> > bpf_iter_mptcp_subflow_new/_next/_destroy,
> > bpf_mptcp_sock_acquire/_release and other helpers into
> > bpf_experimental.h.
> > 
> > Use _acquire() to acquire the msk, then use
> > bpf_for_each(mptcp_subflow) to
> > walk the subflow list of this msk. Invoke kfuncs
> > mptcp_subflow_active() and
> > bpf_mptcp_subflow_tcp_sock() in the loop to pick a subsocket.
> > Finally use
> > bpf_mptcp_subflow_ctx() to get the subflow context of this
> > subsocket and
> > use mptcp_subflow_set_scheduled() to set it as being scheduled.
> 
> Do you think we could have a test not depending on scheduler helpers?
> Without this dependence, we could already upstream this series, and
> get
> feedback without having to wait for the new scheduler API.

This set can be upstream as is, no need to wait for the new scheduler
API.

If no scheduler helpers are used in this test, no need to add this
mptcp_subflow bpf_iter at all. mptcp_for_each_subflow() helper in
progs/mptcp_bpf.h can do that.

mptcp_for_each_subflow(msk, subflow) {
	subflow = bpf_core_cast(subflow, struct
mptcp_subflow_context);
	subflows += subflow->subflow_id;
}

No need to use this:

bpf_for_each(mptcp_subflow, subflow, msk)
	subflows += subflow->subflow_id;

> 
> If you remove the use of mptcp_subflow_active() and
> mptcp_subflow_set_scheduled(), that's enough, no?

But "subflow" in mptcp_for_each_subflow() loop can't be passed to a
kernel function:

mptcp_for_each_subflow(msk, subflow) {
	subflow = bpf_core_cast(subflow, struct
mptcp_subflow_context);
	mptcp_subflow_active(subflows);
}

This is not allowed by BPF.

So we add this iter to do this:

bpf_for_each(mptcp_subflow, subflow, msk)
	kfunc(subflow);

In this test, I must pick some kfuncs accepted "subflow" argument to do
this test, so mptcp_subflow_active and mptcp_subflow_set_scheduled are
picked.

Also, This iter is for the BPF packet scheduler, why not test the
actual usage of scheduler helpers in this test?

> 
> > 
> > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> > ---
> >  .../testing/selftests/bpf/bpf_experimental.h  |  7 ++++
> >  tools/testing/selftests/bpf/progs/mptcp_bpf.h |  9 ++++
> >  .../bpf/progs/mptcp_bpf_iters_subflow.c       | 42
> > +++++++++++++++++++
> >  3 files changed, 58 insertions(+)
> >  create mode 100644
> > tools/testing/selftests/bpf/progs/mptcp_bpf_iters_subflow.c
> > 
> > diff --git a/tools/testing/selftests/bpf/bpf_experimental.h
> > b/tools/testing/selftests/bpf/bpf_experimental.h
> > index b0668f29f7b3..d43690b17468 100644
> > --- a/tools/testing/selftests/bpf/bpf_experimental.h
> > +++ b/tools/testing/selftests/bpf/bpf_experimental.h
> > @@ -575,6 +575,13 @@ extern int bpf_iter_css_new(struct
> > bpf_iter_css *it,
> >  extern struct cgroup_subsys_state *bpf_iter_css_next(struct
> > bpf_iter_css *it) __weak __ksym;
> >  extern void bpf_iter_css_destroy(struct bpf_iter_css *it) __weak
> > __ksym;
> >  
> > +struct bpf_iter_mptcp_subflow;
> > +extern int bpf_iter_mptcp_subflow_new(struct
> > bpf_iter_mptcp_subflow *it,
> > +				      struct mptcp_sock *msk)
> > __weak __ksym;
> > +extern struct mptcp_subflow_context *
> > +bpf_iter_mptcp_subflow_next(struct bpf_iter_mptcp_subflow *it)
> > __weak __ksym;
> > +extern void bpf_iter_mptcp_subflow_destroy(struct
> > bpf_iter_mptcp_subflow *it) __weak __ksym;
> > +
> >  extern int bpf_wq_init(struct bpf_wq *wq, void *p__map, unsigned
> > int flags) __weak __ksym;
> >  extern int bpf_wq_start(struct bpf_wq *wq, unsigned int flags)
> > __weak __ksym;
> >  extern int bpf_wq_set_callback_impl(struct bpf_wq *wq,
> > diff --git a/tools/testing/selftests/bpf/progs/mptcp_bpf.h
> > b/tools/testing/selftests/bpf/progs/mptcp_bpf.h
> > index c3800f986ae1..e18796361394 100644
> > --- a/tools/testing/selftests/bpf/progs/mptcp_bpf.h
> > +++ b/tools/testing/selftests/bpf/progs/mptcp_bpf.h
> > @@ -43,9 +43,18 @@ mptcp_subflow_tcp_sock(const struct
> > mptcp_subflow_context *subflow)
> >  }
> >  
> >  /* ksym */
> > +extern bool mptcp_subflow_active(struct mptcp_subflow_context
> > *subflow) __ksym;
> >  extern void mptcp_subflow_set_scheduled(struct
> > mptcp_subflow_context *subflow,
> >  					bool scheduled) __ksym;
> >  
> > +extern struct mptcp_sock *bpf_mptcp_sock_acquire(struct mptcp_sock
> > *msk) __ksym;
> > +extern void bpf_mptcp_sock_release(struct mptcp_sock *msk) __ksym;
> > +
> > +extern struct mptcp_subflow_context *
> > +bpf_mptcp_subflow_ctx(const struct sock *sk) __ksym;
> > +extern struct sock *
> > +bpf_mptcp_subflow_tcp_sock(const struct mptcp_subflow_context
> > *subflow) __ksym;
> > +
> >  extern struct mptcp_subflow_context *
> >  bpf_mptcp_subflow_ctx_by_pos(const struct mptcp_sched_data *data,
> > unsigned int pos) __ksym;
> >  
> > diff --git
> > a/tools/testing/selftests/bpf/progs/mptcp_bpf_iters_subflow.c
> > b/tools/testing/selftests/bpf/progs/mptcp_bpf_iters_subflow.c
> > new file mode 100644
> > index 000000000000..4268e4604c5a
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/progs/mptcp_bpf_iters_subflow.c
> > @@ -0,0 +1,42 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/* Copyright (c) 2024, Kylin Software */
> > +
> > +/* vmlinux.h, bpf_helpers.h and other 'define' */
> > +#include "bpf_tracing_net.h"
> > +#include "mptcp_bpf.h"
> > +
> > +char _license[] SEC("license") = "GPL";
> > +int subflows;
> > +int pid;
> > +
> > +SEC("fentry/mptcp_sched_get_send")
> 
> If I understand correctly, this hook will be called twice on the
> client
> connection for this test because the client is sending two times one
> byte, right?

Yes, in the first call, the subflows are not been added yet, so it is
called twice. I will add this check to skip the first call:

       if (msk->pm.server_side || !msk->pm.subflows)
               return 0;

> 
> > +int BPF_PROG(trace_mptcp_sched_get_send, struct mptcp_sock *msk)
> > +{
> > +	struct mptcp_subflow_context *subflow;
> > +	struct sock *ssk = NULL;
> > +
> > +	if (bpf_get_current_pid_tgid() >> 32 != pid)
> > +		return 0;
> > +
> > +	msk = bpf_mptcp_sock_acquire(msk);
> > +	if (!msk)
> > +		return 0;
> > +	bpf_for_each(mptcp_subflow, subflow, msk) {
> > +		if (subflow->token != msk->token)
> > +			break;
> 
> Out of curiosity, why is this needed? Is it needed for the verifier?
> Or
> just an extra check?
> Can you add a comment here explaining why this is there please?

I'll drop it.

> 
> > +
> > +		if (!mptcp_subflow_active(subflow))
> > +			continue;
> > +
> > +		ssk = bpf_mptcp_subflow_tcp_sock(subflow);
> 
> Do you get 'ssk' just to use bpf_mptcp_subflow_tcp_sock() and
> bpf_mptcp_subflow_ctx()?

Yes, almost every BPF scheduler use these helpers, so test them here.

> 
> > +	}
> > +	bpf_mptcp_sock_release(msk);
> > +
> > +	if (!ssk)
> > +		return 0;
> > +	subflow = bpf_mptcp_subflow_ctx(ssk);
> > +	mptcp_subflow_set_scheduled(subflow, true);
> > +	subflows = subflow->subflow_id;
> 
> Here, it looks like you only check if the last subflow is in the
> list.
> 
> Would it not be better to count the number of subflows in the list?
> Or
> if you want to read something from 'subflow', you could add all
> subflow_id?
> 
>   subflows = 0;
> 
>   (...)
> 
>   bpf_for_each(mptcp_subflow, subflow, msk)
>       subflows += subflow->subflow_id;
> 
> By doing that, we will catch if one is missing, and the order is not
> important.

Yes, "subflows += subflow->subflow_id" is much better.

> 
> If still want to use bpf_mptcp_subflow_tcp_sock() and
> bpf_mptcp_subflow_ctx(), maybe you can save other fields from the
> last
> subflow: the token to compare it with the one in the msk? Or the port
> number or the address?

Sorry, I don't fully understand your last paragraph. But I stored the
dport number in the code below. Is it the same as what you thought? If
so, I'll send a squash-to patch for it.

int pid;
int ids;

SEC("fentry/mptcp_sched_get_send")
int BPF_PROG(trace_mptcp_sched_get_send, struct mptcp_sock *msk)
{
        struct mptcp_subflow_context *subflow;
        struct sock *sk = (struct sock *)msk;
        struct sock *ssk = NULL;
        int subflows = 0;
        __be16 dport;

        if (bpf_get_current_pid_tgid() >> 32 != pid)
                return 0;

        if (msk->pm.server_side || !msk->pm.subflows)
                return 0;

        msk = bpf_mptcp_sock_acquire(msk);
        if (!msk)
                return 0;
        bpf_for_each(mptcp_subflow, subflow, msk) {
                if (!mptcp_subflow_active(subflow))
                        continue;

                subflows += subflow->subflow_id;
                ssk = bpf_mptcp_subflow_tcp_sock(subflow);
                dport = ssk->sk_dport;
        }

        if (!ssk)
                goto out;
        subflow = bpf_mptcp_subflow_ctx(ssk);
        if (dport != ssk->sk_dport || dport != sk->sk_dport)
                goto out;
        mptcp_subflow_set_scheduled(subflow, true);
        ids = subflows;

out:
        bpf_mptcp_sock_release(msk);
        return 0;
}

Thanks,
-Geliang

> 
> > +
> > +	return 0;
> > +}
> 
> Cheers,
> Matt

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

[Matthieu Baerts]

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.

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

[Geliang Tang]

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

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

[Geliang Tang]

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

Re: [PATCH mptcp-next v9 4/7] selftests/bpf: Add mptcp_subflow bpf_iter test prog

[Matthieu Baerts]

Hi Geliang,

On 15/10/2024 09:59, Geliang Tang wrote:
> On Mon, 2024-10-14 at 18:06 +0200, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> On 09/10/2024 11:45, Geliang Tang wrote:
>>> From: Geliang Tang <tanggeliang@kylinos.cn>
>>>
>>> This patch adds a ftrace hook for mptcp_sched_get_send() to test
>>> the newly
>>> added mptcp_subflow bpf_iter. This test simulates a typical mptcp
>>> packet
>>> scheduler, which selects a subflow from multiple subflows of an
>>> mptcp
>>> socket to send data.
>>>
>>> Export mptcp_subflow helpers
>>> bpf_iter_mptcp_subflow_new/_next/_destroy,
>>> bpf_mptcp_sock_acquire/_release and other helpers into
>>> bpf_experimental.h.
>>>
>>> Use _acquire() to acquire the msk, then use
>>> bpf_for_each(mptcp_subflow) to
>>> walk the subflow list of this msk. Invoke kfuncs
>>> mptcp_subflow_active() and
>>> bpf_mptcp_subflow_tcp_sock() in the loop to pick a subsocket.
>>> Finally use
>>> bpf_mptcp_subflow_ctx() to get the subflow context of this
>>> subsocket and
>>> use mptcp_subflow_set_scheduled() to set it as being scheduled.
>>
>> Do you think we could have a test not depending on scheduler helpers?
>> Without this dependence, we could already upstream this series, and
>> get
>> feedback without having to wait for the new scheduler API.
> 
> This set can be upstream as is, no need to wait for the new scheduler
> API.
> 
> If no scheduler helpers are used in this test, no need to add this
> mptcp_subflow bpf_iter at all. mptcp_for_each_subflow() helper in
> progs/mptcp_bpf.h can do that.
> 
> mptcp_for_each_subflow(msk, subflow) {
> 	subflow = bpf_core_cast(subflow, struct
> mptcp_subflow_context);
> 	subflows += subflow->subflow_id;
> }
> 
> No need to use this:
> 
> bpf_for_each(mptcp_subflow, subflow, msk)
> 	subflows += subflow->subflow_id;

I agree, but for me, I see this as a preparation step, and then it is
fine if this test is not doing anything useful for the moment. I think
it would be enough to add a comment in the code like:

  /* Here MPTCP-specific kfunc can be called */

And add in the commit message that these kfunc will be added later one,
as a next step.

>> If you remove the use of mptcp_subflow_active() and
>> mptcp_subflow_set_scheduled(), that's enough, no?
> 
> But "subflow" in mptcp_for_each_subflow() loop can't be passed to a
> kernel function:
> 
> mptcp_for_each_subflow(msk, subflow) {
> 	subflow = bpf_core_cast(subflow, struct
> mptcp_subflow_context);
> 	mptcp_subflow_active(subflows);
> }
> 
> This is not allowed by BPF.
> 
> So we add this iter to do this:
> 
> bpf_for_each(mptcp_subflow, subflow, msk)
> 	kfunc(subflow);
> 
> In this test, I must pick some kfuncs accepted "subflow" argument to do
> this test, so mptcp_subflow_active and mptcp_subflow_set_scheduled are
> picked.

I think that can be done later on, the first step is to have these MPTCP
bpf_iter upstreamed I think.

> Also, This iter is for the BPF packet scheduler, why not test the
> actual usage of scheduler helpers in this test?

I understand that, but I think it will be easier to avoid these helpers
for the moment, and make the test as simpler as possible.

(...)

>>> diff --git
>>> a/tools/testing/selftests/bpf/progs/mptcp_bpf_iters_subflow.c
>>> b/tools/testing/selftests/bpf/progs/mptcp_bpf_iters_subflow.c
>>> new file mode 100644
>>> index 000000000000..4268e4604c5a
>>> --- /dev/null
>>> +++ b/tools/testing/selftests/bpf/progs/mptcp_bpf_iters_subflow.c
>>> @@ -0,0 +1,42 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/* Copyright (c) 2024, Kylin Software */
>>> +
>>> +/* vmlinux.h, bpf_helpers.h and other 'define' */
>>> +#include "bpf_tracing_net.h"
>>> +#include "mptcp_bpf.h"
>>> +
>>> +char _license[] SEC("license") = "GPL";
>>> +int subflows;
>>> +int pid;
>>> +
>>> +SEC("fentry/mptcp_sched_get_send")
>>
>> If I understand correctly, this hook will be called twice on the
>> client
>> connection for this test because the client is sending two times one
>> byte, right?
> 
> Yes, in the first call, the subflows are not been added yet, so it is
> called twice. I will add this check to skip the first call:
> 
>        if (msk->pm.server_side || !msk->pm.subflows)
>                return 0;

Good idea! Please add a comment above to explain you added that to only
do the rest once.

If we want to have a simpler test without the scheduler helpers, we can
also use another hook, e.g. just before closing the different subflows?
Up to you.

Also, just to be sure: is this BPF program only used (loaded, running)
by the corresponding test?
Can you remind me why is there a PID check? Is it because all BPF
programs are always loaded for all tests?

>>> +int BPF_PROG(trace_mptcp_sched_get_send, struct mptcp_sock *msk)
>>> +{
>>> +	struct mptcp_subflow_context *subflow;
>>> +	struct sock *ssk = NULL;
>>> +
>>> +	if (bpf_get_current_pid_tgid() >> 32 != pid)
>>> +		return 0;
>>> +
>>> +	msk = bpf_mptcp_sock_acquire(msk);
>>> +	if (!msk)
>>> +		return 0;
>>> +	bpf_for_each(mptcp_subflow, subflow, msk) {
>>> +		if (subflow->token != msk->token)
>>> +			break;
>>
>> Out of curiosity, why is this needed? Is it needed for the verifier?
>> Or
>> just an extra check?
>> Can you add a comment here explaining why this is there please?
> 
> I'll drop it.

Just to be clear: I'm not asking to drop it, but to understand why it is
needed. The main reason is to have other people using it in future
programs, because they saw it was used here. If it is here as an
"assert", something that cannot be wrong, that's fine, as long as it is
marked (by a comment) as is.

>>> +
>>> +		if (!mptcp_subflow_active(subflow))
>>> +			continue;
>>> +
>>> +		ssk = bpf_mptcp_subflow_tcp_sock(subflow);
>>
>> Do you get 'ssk' just to use bpf_mptcp_subflow_tcp_sock() and
>> bpf_mptcp_subflow_ctx()?
> 
> Yes, almost every BPF scheduler use these helpers, so test them here.

For me, that's fine to keep using these helpers, even if their usage is
limited in the test. You can keep it here, and keep
bpf_mptcp_subflow_ctx(ssk) below, then use 'subflow' below to compare
the token with the one of the msk for example. Feel free to add a
comment mentioning that it is just to show these helpers can be used in
the 'bpf_for_each()' and outside.

>>> +	}
>>> +	bpf_mptcp_sock_release(msk);
>>> +
>>> +	if (!ssk)
>>> +		return 0;
>>> +	subflow = bpf_mptcp_subflow_ctx(ssk);
>>> +	mptcp_subflow_set_scheduled(subflow, true);
>>> +	subflows = subflow->subflow_id;
>>
>> Here, it looks like you only check if the last subflow is in the
>> list.
>>
>> Would it not be better to count the number of subflows in the list?
>> Or
>> if you want to read something from 'subflow', you could add all
>> subflow_id?
>>
>>   subflows = 0;
>>
>>   (...)
>>
>>   bpf_for_each(mptcp_subflow, subflow, msk)
>>       subflows += subflow->subflow_id;
>>
>> By doing that, we will catch if one is missing, and the order is not
>> important.
> 
> Yes, "subflows += subflow->subflow_id" is much better.
> 
>>
>> If still want to use bpf_mptcp_subflow_tcp_sock() and
>> bpf_mptcp_subflow_ctx(), maybe you can save other fields from the
>> last
>> subflow: the token to compare it with the one in the msk? Or the port
>> number or the address?
> 
> Sorry, I don't fully understand your last paragraph. But I stored the
> dport number in the code below. Is it the same as what you thought? If
> so, I'll send a squash-to patch for it.

I was just thinking about something you could do to keep using these new
helpers: keep the ref to the last subflow, and compare with data from
the msk, e.g.

  struct mptcp_subflow_context *subflow;
  struct sock *sk = (struct sock *)msk;
  struct sock *ssk = NULL;
  int all_ids = 0;

  /* TODO: why is it needed? */
  if (bpf_get_current_pid_tgid() >> 32 != pid)
      return 0;

  /* to do the test only once: on the client side, at the 2nd send */
  if (msk->pm.server_side || !msk->pm.subflows)
      return 0;

  msk = bpf_mptcp_sock_acquire(msk);
  if (!msk)
      return 0;

  bpf_for_each(mptcp_subflow, subflow, msk) {
      /* Here MPTCP-specific kfunc can be called: this test is not doing
       * anything really useful, only to verify the iteration works.
       */

      /* assert: if not OK, something wrong on the kernel side */
      if (subflow->token != msk->token)
          break;

      /* to check we iterate over all subflows */
      local_ids += subflow->subflow_id;

      /* only to check the following kfunc works */
      ssk = bpf_mptcp_subflow_tcp_sock(subflow);
  }

  if (!ssk)
      goto out;

  /* assert: if not OK, something wrong on the kernel side */
  if (ssk->sk_dport != sk->sk_dport)
      goto out;

  /* only to check the following kfunc works */
  subflow = bpf_mptcp_subflow_ctx(ssk);
  if (subflow->token != msk->token)
      goto out;

  ids = local_ids;


(not tested)

→ So not using anything linked to the packet scheduler, just very simple
check to make sure 'bpf_iter' + bpf_mptcp_subflow_tcp_sock() and
bpf_mptcp_subflow_ctx() work as expected. Like that, it sounds easier to
upstream as it is.

WDYT?

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

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

[Matthieu Baerts]

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
-- 
Sponsored by the NGI0 Core fund.

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

[Matthieu Baerts]

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
-- 
Sponsored by the NGI0 Core fund.

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

[Matthieu Baerts]

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
-- 
Sponsored by the NGI0 Core fund.

Re: [PATCH mptcp-next v9 4/7] selftests/bpf: Add mptcp_subflow bpf_iter test prog

[Geliang Tang]

Hi Matt,

On Tue, 2024-10-15 at 12:47 +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 15/10/2024 09:59, Geliang Tang wrote:
> > On Mon, 2024-10-14 at 18:06 +0200, Matthieu Baerts wrote:
> > > Hi Geliang,
> > > 
> > > On 09/10/2024 11:45, Geliang Tang wrote:
> > > > From: Geliang Tang <tanggeliang@kylinos.cn>
> > > > 
> > > > This patch adds a ftrace hook for mptcp_sched_get_send() to
> > > > test
> > > > the newly
> > > > added mptcp_subflow bpf_iter. This test simulates a typical
> > > > mptcp
> > > > packet
> > > > scheduler, which selects a subflow from multiple subflows of an
> > > > mptcp
> > > > socket to send data.
> > > > 
> > > > Export mptcp_subflow helpers
> > > > bpf_iter_mptcp_subflow_new/_next/_destroy,
> > > > bpf_mptcp_sock_acquire/_release and other helpers into
> > > > bpf_experimental.h.
> > > > 
> > > > Use _acquire() to acquire the msk, then use
> > > > bpf_for_each(mptcp_subflow) to
> > > > walk the subflow list of this msk. Invoke kfuncs
> > > > mptcp_subflow_active() and
> > > > bpf_mptcp_subflow_tcp_sock() in the loop to pick a subsocket.
> > > > Finally use
> > > > bpf_mptcp_subflow_ctx() to get the subflow context of this
> > > > subsocket and
> > > > use mptcp_subflow_set_scheduled() to set it as being scheduled.
> > > 
> > > Do you think we could have a test not depending on scheduler
> > > helpers?
> > > Without this dependence, we could already upstream this series,
> > > and
> > > get
> > > feedback without having to wait for the new scheduler API.
> > 
> > This set can be upstream as is, no need to wait for the new
> > scheduler
> > API.
> > 
> > If no scheduler helpers are used in this test, no need to add this
> > mptcp_subflow bpf_iter at all. mptcp_for_each_subflow() helper in
> > progs/mptcp_bpf.h can do that.
> > 
> > mptcp_for_each_subflow(msk, subflow) {
> > 	subflow = bpf_core_cast(subflow, struct
> > mptcp_subflow_context);
> > 	subflows += subflow->subflow_id;
> > }
> > 
> > No need to use this:
> > 
> > bpf_for_each(mptcp_subflow, subflow, msk)
> > 	subflows += subflow->subflow_id;
> 
> I agree, but for me, I see this as a preparation step, and then it is
> fine if this test is not doing anything useful for the moment. I
> think
> it would be enough to add a comment in the code like:
> 
>   /* Here MPTCP-specific kfunc can be called */
> 
> And add in the commit message that these kfunc will be added later
> one,
> as a next step.
> 
> > > If you remove the use of mptcp_subflow_active() and
> > > mptcp_subflow_set_scheduled(), that's enough, no?
> > 
> > But "subflow" in mptcp_for_each_subflow() loop can't be passed to a
> > kernel function:
> > 
> > mptcp_for_each_subflow(msk, subflow) {
> > 	subflow = bpf_core_cast(subflow, struct
> > mptcp_subflow_context);
> > 	mptcp_subflow_active(subflows);
> > }
> > 
> > This is not allowed by BPF.
> > 
> > So we add this iter to do this:
> > 
> > bpf_for_each(mptcp_subflow, subflow, msk)
> > 	kfunc(subflow);
> > 
> > In this test, I must pick some kfuncs accepted "subflow" argument
> > to do
> > this test, so mptcp_subflow_active and mptcp_subflow_set_scheduled
> > are
> > picked.
> 
> I think that can be done later on, the first step is to have these
> MPTCP
> bpf_iter upstreamed I think.
> 
> > Also, This iter is for the BPF packet scheduler, why not test the
> > actual usage of scheduler helpers in this test?
> 
> I understand that, but I think it will be easier to avoid these
> helpers
> for the moment, and make the test as simpler as possible.
> 
> (...)
> 
> > > > diff --git
> > > > a/tools/testing/selftests/bpf/progs/mptcp_bpf_iters_subflow.c
> > > > b/tools/testing/selftests/bpf/progs/mptcp_bpf_iters_subflow.c
> > > > new file mode 100644
> > > > index 000000000000..4268e4604c5a
> > > > --- /dev/null
> > > > +++
> > > > b/tools/testing/selftests/bpf/progs/mptcp_bpf_iters_subflow.c
> > > > @@ -0,0 +1,42 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +/* Copyright (c) 2024, Kylin Software */
> > > > +
> > > > +/* vmlinux.h, bpf_helpers.h and other 'define' */
> > > > +#include "bpf_tracing_net.h"
> > > > +#include "mptcp_bpf.h"
> > > > +
> > > > +char _license[] SEC("license") = "GPL";
> > > > +int subflows;
> > > > +int pid;
> > > > +
> > > > +SEC("fentry/mptcp_sched_get_send")
> > > 
> > > If I understand correctly, this hook will be called twice on the
> > > client
> > > connection for this test because the client is sending two times
> > > one
> > > byte, right?
> > 
> > Yes, in the first call, the subflows are not been added yet, so it
> > is
> > called twice. I will add this check to skip the first call:
> > 
> >        if (msk->pm.server_side || !msk->pm.subflows)
> >                return 0;
> 
> Good idea! Please add a comment above to explain you added that to
> only
> do the rest once.
> 
> If we want to have a simpler test without the scheduler helpers, we
> can
> also use another hook, e.g. just before closing the different
> subflows?
> Up to you.
> 
> Also, just to be sure: is this BPF program only used (loaded,
> running)
> by the corresponding test?
> Can you remind me why is there a PID check? Is it because all BPF
> programs are always loaded for all tests?
> 
> > > > +int BPF_PROG(trace_mptcp_sched_get_send, struct mptcp_sock
> > > > *msk)
> > > > +{
> > > > +	struct mptcp_subflow_context *subflow;
> > > > +	struct sock *ssk = NULL;
> > > > +
> > > > +	if (bpf_get_current_pid_tgid() >> 32 != pid)
> > > > +		return 0;
> > > > +
> > > > +	msk = bpf_mptcp_sock_acquire(msk);
> > > > +	if (!msk)
> > > > +		return 0;
> > > > +	bpf_for_each(mptcp_subflow, subflow, msk) {
> > > > +		if (subflow->token != msk->token)
> > > > +			break;
> > > 
> > > Out of curiosity, why is this needed? Is it needed for the
> > > verifier?
> > > Or
> > > just an extra check?
> > > Can you add a comment here explaining why this is there please?
> > 
> > I'll drop it.
> 
> Just to be clear: I'm not asking to drop it, but to understand why it
> is
> needed. The main reason is to have other people using it in future
> programs, because they saw it was used here. If it is here as an
> "assert", something that cannot be wrong, that's fine, as long as it
> is
> marked (by a comment) as is.
> 
> > > > +
> > > > +		if (!mptcp_subflow_active(subflow))
> > > > +			continue;
> > > > +
> > > > +		ssk = bpf_mptcp_subflow_tcp_sock(subflow);
> > > 
> > > Do you get 'ssk' just to use bpf_mptcp_subflow_tcp_sock() and
> > > bpf_mptcp_subflow_ctx()?
> > 
> > Yes, almost every BPF scheduler use these helpers, so test them
> > here.
> 
> For me, that's fine to keep using these helpers, even if their usage
> is
> limited in the test. You can keep it here, and keep
> bpf_mptcp_subflow_ctx(ssk) below, then use 'subflow' below to compare
> the token with the one of the msk for example. Feel free to add a
> comment mentioning that it is just to show these helpers can be used
> in
> the 'bpf_for_each()' and outside.
> 
> > > > +	}
> > > > +	bpf_mptcp_sock_release(msk);
> > > > +
> > > > +	if (!ssk)
> > > > +		return 0;
> > > > +	subflow = bpf_mptcp_subflow_ctx(ssk);
> > > > +	mptcp_subflow_set_scheduled(subflow, true);
> > > > +	subflows = subflow->subflow_id;
> > > 
> > > Here, it looks like you only check if the last subflow is in the
> > > list.
> > > 
> > > Would it not be better to count the number of subflows in the
> > > list?
> > > Or
> > > if you want to read something from 'subflow', you could add all
> > > subflow_id?
> > > 
> > >   subflows = 0;
> > > 
> > >   (...)
> > > 
> > >   bpf_for_each(mptcp_subflow, subflow, msk)
> > >       subflows += subflow->subflow_id;
> > > 
> > > By doing that, we will catch if one is missing, and the order is
> > > not
> > > important.
> > 
> > Yes, "subflows += subflow->subflow_id" is much better.
> > 
> > > 
> > > If still want to use bpf_mptcp_subflow_tcp_sock() and
> > > bpf_mptcp_subflow_ctx(), maybe you can save other fields from the
> > > last
> > > subflow: the token to compare it with the one in the msk? Or the
> > > port
> > > number or the address?
> > 
> > Sorry, I don't fully understand your last paragraph. But I stored
> > the
> > dport number in the code below. Is it the same as what you thought?
> > If
> > so, I'll send a squash-to patch for it.
> 
> I was just thinking about something you could do to keep using these
> new
> helpers: keep the ref to the last subflow, and compare with data from
> the msk, e.g.
> 
>   struct mptcp_subflow_context *subflow;
>   struct sock *sk = (struct sock *)msk;
>   struct sock *ssk = NULL;
>   int all_ids = 0;
> 
>   /* TODO: why is it needed? */

This test program is a ftrace hook, it may affect other programs, so
the pid limitation is needed. v10 has changed the test program as
"cgroup/getsockopt", no need to add this limitation any more.

>   if (bpf_get_current_pid_tgid() >> 32 != pid)
>       return 0;
> 
>   /* to do the test only once: on the client side, at the 2nd send */
>   if (msk->pm.server_side || !msk->pm.subflows)
>       return 0;
> 
>   msk = bpf_mptcp_sock_acquire(msk);
>   if (!msk)
>       return 0;
> 
>   bpf_for_each(mptcp_subflow, subflow, msk) {
>       /* Here MPTCP-specific kfunc can be called: this test is not
> doing
>        * anything really useful, only to verify the iteration works.
>        */
> 
>       /* assert: if not OK, something wrong on the kernel side */
>       if (subflow->token != msk->token)
>           break;
> 
>       /* to check we iterate over all subflows */
>       local_ids += subflow->subflow_id;
> 
>       /* only to check the following kfunc works */
>       ssk = bpf_mptcp_subflow_tcp_sock(subflow);
>   }
> 
>   if (!ssk)
>       goto out;
> 
>   /* assert: if not OK, something wrong on the kernel side */
>   if (ssk->sk_dport != sk->sk_dport)
>       goto out;
> 
>   /* only to check the following kfunc works */
>   subflow = bpf_mptcp_subflow_ctx(ssk);
>   if (subflow->token != msk->token)
>       goto out;

token is checked twice. I dropped the first one in v10.

Thanks,
-Geliang

> 
>   ids = local_ids;
> 
> 
> (not tested)
> 
> → So not using anything linked to the packet scheduler, just very
> simple
> check to make sure 'bpf_iter' + bpf_mptcp_subflow_tcp_sock() and
> bpf_mptcp_subflow_ctx() work as expected. Like that, it sounds easier
> to
> upstream as it is.
> 
> WDYT?
> 
> Cheers,
> Matt

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

[Geliang Tang]

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

Re: [PATCH mptcp-next v9 4/7] selftests/bpf: Add mptcp_subflow bpf_iter test prog

[Matthieu Baerts]

Hi Geliang,

On 18/10/2024 03:22, Geliang Tang wrote:
> On Tue, 2024-10-15 at 12:47 +0200, Matthieu Baerts wrote:
>>   /* TODO: why is it needed? */
> 
> This test program is a ftrace hook, it may affect other programs, so
> the pid limitation is needed. v10 has changed the test program as
> "cgroup/getsockopt", no need to add this limitation any more.

OK. Do you mean that when running the BPF selftests, all BPF test
programs are loaded? Even the non MPTCP ones? Or only the ftrace ones?

Why is "cgroup/getsockopt" safer? Because it is only executed for a
specific CGroup?

(...)

>>   /* only to check the following kfunc works */
>>   subflow = bpf_mptcp_subflow_ctx(ssk);
>>   if (subflow->token != msk->token)
>>       goto out;
> 
> token is checked twice. I dropped the first one in v10.

OK!

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