[PATCH mptcp-next v7 0/5] use bpf_iter in bpf schedulers

[PATCH mptcp-next v7 0/5] use bpf_iter in bpf schedulers

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

v7:
 - move cleanup patches out of this set.
 - rebased.

v6:
 - rebased to "add mptcp_subflow bpf_iter" v10

v5:
 - patch 2, drop mptcp_sock_type and mptcp_subflow_type.
 - patch 3, revert "bpf: Export more bpf_burst related functions"
 - patch 4, merge "bpf: Export more bpf_burst related functions" into it.

v4:
 - patch 2, a new cleanup for "bpf: Add bpf_mptcp_sched_ops".
 - patch 3 should be reverted.
 - patch 8, register kfunc_set.

v3:
 - rebased.
 - put the "drop has_bytes_sent" squash-to patch into this set.

v2:
 - update bpf_rr and bpf_burst

With the newly added mptcp_subflow bpf_iter, we can get rid of the
subflows array "contexts" in struct mptcp_sched_data. This set
uses bpf_for_each(mptcp_subflow) helper to update all the bpf
schedules:

        bpf_for_each(mptcp_subflow, subflow, msk) {
                ... ...
                mptcp_subflow_set_scheduled(subflow, true);
        }

Geliang Tang (5):
  Squash to "selftests/bpf: Add bpf_bkup scheduler & test"
  Squash to "selftests/bpf: Add bpf_rr scheduler & test"
  Squash to "selftests/bpf: Add bpf_red scheduler & test"
  Squash to "selftests/bpf: Add bpf_burst scheduler & test"
  Squash to "selftests/bpf: Add bpf_first scheduler & test"

 tools/testing/selftests/bpf/progs/mptcp_bpf.h |  3 -
 .../selftests/bpf/progs/mptcp_bpf_bkup.c      | 18 +----
 .../selftests/bpf/progs/mptcp_bpf_burst.c     | 79 ++++++++++---------
 .../selftests/bpf/progs/mptcp_bpf_first.c     | 10 ++-
 .../selftests/bpf/progs/mptcp_bpf_red.c       | 10 +--
 .../selftests/bpf/progs/mptcp_bpf_rr.c        | 26 +++---
 6 files changed, 67 insertions(+), 79 deletions(-)

-- 
2.45.2

[PATCH mptcp-next v7 1/5] Squash to "selftests/bpf: Add bpf_bkup scheduler & test"

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

Use the newly added bpf_for_each() helper to walk the conn_list.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 .../selftests/bpf/progs/mptcp_bpf_bkup.c       | 18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)

diff --git a/tools/testing/selftests/bpf/progs/mptcp_bpf_bkup.c b/tools/testing/selftests/bpf/progs/mptcp_bpf_bkup.c
index 296f0318d843..0c32ce623bab 100644
--- a/tools/testing/selftests/bpf/progs/mptcp_bpf_bkup.c
+++ b/tools/testing/selftests/bpf/progs/mptcp_bpf_bkup.c
@@ -20,30 +20,20 @@ SEC("struct_ops")
 int BPF_PROG(bpf_bkup_get_subflow, struct mptcp_sock *msk,
 	     struct mptcp_sched_data *data)
 {
-	int nr = -1;
-
-	for (int i = 0; i < data->subflows && i < MPTCP_SUBFLOWS_MAX; i++) {
-		struct mptcp_subflow_context *subflow;
-
-		subflow = bpf_mptcp_subflow_ctx_by_pos(data, i);
-		if (!subflow)
-			break;
+	struct mptcp_subflow_context *subflow;
 
+	bpf_for_each(mptcp_subflow, subflow, msk) {
 		if (!BPF_CORE_READ_BITFIELD_PROBED(subflow, backup) ||
 		    !BPF_CORE_READ_BITFIELD_PROBED(subflow, request_bkup)) {
-			nr = i;
+			mptcp_subflow_set_scheduled(subflow, true);
 			break;
 		}
 	}
 
-	if (nr != -1) {
-		mptcp_subflow_set_scheduled(bpf_mptcp_subflow_ctx_by_pos(data, nr), true);
-		return -1;
-	}
 	return 0;
 }
 
-SEC(".struct_ops")
+SEC(".struct_ops.link")
 struct mptcp_sched_ops bkup = {
 	.init		= (void *)mptcp_sched_bkup_init,
 	.release	= (void *)mptcp_sched_bkup_release,
-- 
2.45.2

[PATCH mptcp-next v7 2/5] Squash to "selftests/bpf: Add bpf_rr scheduler & test"

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

Use the newly added bpf_for_each() helper to walk the conn_list.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 .../selftests/bpf/progs/mptcp_bpf_rr.c        | 26 ++++++++-----------
 1 file changed, 11 insertions(+), 15 deletions(-)

diff --git a/tools/testing/selftests/bpf/progs/mptcp_bpf_rr.c b/tools/testing/selftests/bpf/progs/mptcp_bpf_rr.c
index 638ea6aa63b7..e02b8cfb262b 100644
--- a/tools/testing/selftests/bpf/progs/mptcp_bpf_rr.c
+++ b/tools/testing/selftests/bpf/progs/mptcp_bpf_rr.c
@@ -34,10 +34,9 @@ SEC("struct_ops")
 int BPF_PROG(bpf_rr_get_subflow, struct mptcp_sock *msk,
 	     struct mptcp_sched_data *data)
 {
-	struct mptcp_subflow_context *subflow;
+	struct mptcp_subflow_context *subflow, *next;
 	struct mptcp_rr_storage *ptr;
 	struct sock *last_snd = NULL;
-	int nr = 0;
 
 	ptr = bpf_sk_storage_get(&mptcp_rr_map, msk, 0,
 				 BPF_LOCAL_STORAGE_GET_F_CREATE);
@@ -45,31 +44,28 @@ int BPF_PROG(bpf_rr_get_subflow, struct mptcp_sock *msk,
 		return -1;
 
 	last_snd = ptr->last_snd;
+	next = bpf_mptcp_subflow_ctx(msk->first);
 
-	for (int i = 0; i < data->subflows && i < MPTCP_SUBFLOWS_MAX; i++) {
-		subflow = bpf_mptcp_subflow_ctx_by_pos(data, i);
-		if (!last_snd || !subflow)
+	bpf_for_each(mptcp_subflow, subflow, msk) {
+		if (!last_snd)
 			break;
 
-		if (mptcp_subflow_tcp_sock(subflow) == last_snd) {
-			if (i + 1 == MPTCP_SUBFLOWS_MAX ||
-			    !bpf_mptcp_subflow_ctx_by_pos(data, i + 1))
+		if (bpf_mptcp_subflow_tcp_sock(subflow) == last_snd) {
+			subflow = bpf_iter_mptcp_subflow_next(&___it);
+			if (!subflow)
 				break;
 
-			nr = i + 1;
+			next = subflow;
 			break;
 		}
 	}
 
-	subflow = bpf_mptcp_subflow_ctx_by_pos(data, nr);
-	if (!subflow)
-		return -1;
-	mptcp_subflow_set_scheduled(subflow, true);
-	ptr->last_snd = mptcp_subflow_tcp_sock(subflow);
+	mptcp_subflow_set_scheduled(next, true);
+	ptr->last_snd = bpf_mptcp_subflow_tcp_sock(next);
 	return 0;
 }
 
-SEC(".struct_ops")
+SEC(".struct_ops.link")
 struct mptcp_sched_ops rr = {
 	.init		= (void *)mptcp_sched_rr_init,
 	.release	= (void *)mptcp_sched_rr_release,
-- 
2.45.2

[PATCH mptcp-next v7 3/5] Squash to "selftests/bpf: Add bpf_red scheduler & test"

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

Use the newly added bpf_for_each() helper to walk the conn_list.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 tools/testing/selftests/bpf/progs/mptcp_bpf_red.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/bpf/progs/mptcp_bpf_red.c b/tools/testing/selftests/bpf/progs/mptcp_bpf_red.c
index cc0aab732fc4..35150771e174 100644
--- a/tools/testing/selftests/bpf/progs/mptcp_bpf_red.c
+++ b/tools/testing/selftests/bpf/progs/mptcp_bpf_red.c
@@ -20,17 +20,15 @@ SEC("struct_ops")
 int BPF_PROG(bpf_red_get_subflow, struct mptcp_sock *msk,
 	     struct mptcp_sched_data *data)
 {
-	for (int i = 0; i < data->subflows && i < MPTCP_SUBFLOWS_MAX; i++) {
-		if (!bpf_mptcp_subflow_ctx_by_pos(data, i))
-			break;
+	struct mptcp_subflow_context *subflow;
 
-		mptcp_subflow_set_scheduled(bpf_mptcp_subflow_ctx_by_pos(data, i), true);
-	}
+	bpf_for_each(mptcp_subflow, subflow, msk)
+		mptcp_subflow_set_scheduled(subflow, true);
 
 	return 0;
 }
 
-SEC(".struct_ops")
+SEC(".struct_ops.link")
 struct mptcp_sched_ops red = {
 	.init		= (void *)mptcp_sched_red_init,
 	.release	= (void *)mptcp_sched_red_release,
-- 
2.45.2

[PATCH mptcp-next v7 4/5] Squash to "selftests/bpf: Add bpf_burst scheduler & test"

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

Use the newly added bpf_for_each() helper to walk the conn_list.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 .../selftests/bpf/progs/mptcp_bpf_burst.c     | 79 ++++++++++---------
 1 file changed, 40 insertions(+), 39 deletions(-)

diff --git a/tools/testing/selftests/bpf/progs/mptcp_bpf_burst.c b/tools/testing/selftests/bpf/progs/mptcp_bpf_burst.c
index eb21119aa8f7..e7df5f048aa4 100644
--- a/tools/testing/selftests/bpf/progs/mptcp_bpf_burst.c
+++ b/tools/testing/selftests/bpf/progs/mptcp_bpf_burst.c
@@ -11,6 +11,10 @@ char _license[] SEC("license") = "GPL";
 
 #define min(a, b) ((a) < (b) ? (a) : (b))
 
+#define SSK_MODE_ACTIVE	0
+#define SSK_MODE_BACKUP	1
+#define SSK_MODE_MAX	2
+
 struct bpf_subflow_send_info {
 	__u8 subflow_id;
 	__u64 linger_time;
@@ -23,10 +27,6 @@ extern bool tcp_stream_memory_free(const struct sock *sk, int wake) __ksym;
 extern bool bpf_mptcp_subflow_queues_empty(struct sock *sk) __ksym;
 extern void mptcp_pm_subflow_chk_stale(const struct mptcp_sock *msk, struct sock *ssk) __ksym;
 
-#define SSK_MODE_ACTIVE	0
-#define SSK_MODE_BACKUP	1
-#define SSK_MODE_MAX	2
-
 static __always_inline __u64 div_u64(__u64 dividend, __u32 divisor)
 {
 	return dividend / divisor;
@@ -57,6 +57,19 @@ static __always_inline bool sk_stream_memory_free(const struct sock *sk)
 	return __sk_stream_memory_free(sk, 0);
 }
 
+static struct mptcp_subflow_context *
+mptcp_lookup_subflow_by_id(struct mptcp_sock *msk, unsigned int id)
+{
+	struct mptcp_subflow_context *subflow;
+
+	bpf_for_each(mptcp_subflow, subflow, msk) {
+		if (subflow->subflow_id == id)
+			return subflow;
+	}
+
+	return NULL;
+}
+
 SEC("struct_ops")
 void BPF_PROG(mptcp_sched_burst_init, struct mptcp_sock *msk)
 {
@@ -67,8 +80,7 @@ void BPF_PROG(mptcp_sched_burst_release, struct mptcp_sock *msk)
 {
 }
 
-static int bpf_burst_get_send(struct mptcp_sock *msk,
-			      struct mptcp_sched_data *data)
+static int bpf_burst_get_send(struct mptcp_sock *msk)
 {
 	struct bpf_subflow_send_info send_info[SSK_MODE_MAX];
 	struct mptcp_subflow_context *subflow;
@@ -84,16 +96,10 @@ static int bpf_burst_get_send(struct mptcp_sock *msk,
 		send_info[i].linger_time = -1;
 	}
 
-	for (i = 0; i < data->subflows && i < MPTCP_SUBFLOWS_MAX; i++) {
-		bool backup;
+	bpf_for_each(mptcp_subflow, subflow, msk) {
+		bool backup = subflow->backup || subflow->request_bkup;
 
-		subflow = bpf_mptcp_subflow_ctx_by_pos(data, i);
-		if (!subflow)
-			break;
-
-		backup = subflow->backup || subflow->request_bkup;
-
-		ssk = mptcp_subflow_tcp_sock(subflow);
+		ssk = bpf_mptcp_subflow_tcp_sock(subflow);
 		if (!mptcp_subflow_active(subflow))
 			continue;
 
@@ -109,7 +115,7 @@ static int bpf_burst_get_send(struct mptcp_sock *msk,
 
 		linger_time = div_u64((__u64)ssk->sk_wmem_queued << 32, pace);
 		if (linger_time < send_info[backup].linger_time) {
-			send_info[backup].subflow_id = i;
+			send_info[backup].subflow_id = subflow->subflow_id;
 			send_info[backup].linger_time = linger_time;
 		}
 	}
@@ -119,10 +125,10 @@ static int bpf_burst_get_send(struct mptcp_sock *msk,
 	if (!nr_active)
 		send_info[SSK_MODE_ACTIVE].subflow_id = send_info[SSK_MODE_BACKUP].subflow_id;
 
-	subflow = bpf_mptcp_subflow_ctx_by_pos(data, send_info[SSK_MODE_ACTIVE].subflow_id);
+	subflow = mptcp_lookup_subflow_by_id(msk, send_info[SSK_MODE_ACTIVE].subflow_id);
 	if (!subflow)
 		return -1;
-	ssk = mptcp_subflow_tcp_sock(subflow);
+	ssk = bpf_mptcp_subflow_tcp_sock(subflow);
 	if (!ssk || !sk_stream_memory_free(ssk))
 		return -1;
 
@@ -141,23 +147,18 @@ static int bpf_burst_get_send(struct mptcp_sock *msk,
 	return 0;
 }
 
-static int bpf_burst_get_retrans(struct mptcp_sock *msk,
-				 struct mptcp_sched_data *data)
+static int bpf_burst_get_retrans(struct mptcp_sock *msk)
 {
-	int backup = MPTCP_SUBFLOWS_MAX, pick = MPTCP_SUBFLOWS_MAX, subflow_id;
+	struct sock *backup = NULL, *pick = NULL;
 	struct mptcp_subflow_context *subflow;
 	int min_stale_count = INT_MAX;
-	struct sock *ssk;
 
-	for (int i = 0; i < data->subflows && i < MPTCP_SUBFLOWS_MAX; i++) {
-		subflow = bpf_mptcp_subflow_ctx_by_pos(data, i);
-		if (!subflow)
-			break;
+	bpf_for_each(mptcp_subflow, subflow, msk) {
+		struct sock *ssk = bpf_mptcp_subflow_tcp_sock(subflow);
 
 		if (!mptcp_subflow_active(subflow))
 			continue;
 
-		ssk = mptcp_subflow_tcp_sock(subflow);
 		/* still data outstanding at TCP level? skip this */
 		if (!tcp_rtx_and_write_queues_empty(ssk)) {
 			mptcp_pm_subflow_chk_stale(msk, ssk);
@@ -166,23 +167,23 @@ static int bpf_burst_get_retrans(struct mptcp_sock *msk,
 		}
 
 		if (subflow->backup || subflow->request_bkup) {
-			if (backup == MPTCP_SUBFLOWS_MAX)
-				backup = i;
+			if (!backup)
+				backup = ssk;
 			continue;
 		}
 
-		if (pick == MPTCP_SUBFLOWS_MAX)
-			pick = i;
+		if (!pick)
+			pick = ssk;
 	}
 
-	if (pick < MPTCP_SUBFLOWS_MAX) {
-		subflow_id = pick;
+	if (pick)
 		goto out;
-	}
-	subflow_id = min_stale_count > 1 ? backup : MPTCP_SUBFLOWS_MAX;
+	pick = min_stale_count > 1 ? backup : NULL;
 
 out:
-	subflow = bpf_mptcp_subflow_ctx_by_pos(data, subflow_id);
+	if (!pick)
+		return -1;
+	subflow = bpf_mptcp_subflow_ctx(pick);
 	if (!subflow)
 		return -1;
 	mptcp_subflow_set_scheduled(subflow, true);
@@ -194,11 +195,11 @@ int BPF_PROG(bpf_burst_get_subflow, struct mptcp_sock *msk,
 	     struct mptcp_sched_data *data)
 {
 	if (data->reinject)
-		return bpf_burst_get_retrans(msk, data);
-	return bpf_burst_get_send(msk, data);
+		return bpf_burst_get_retrans(msk);
+	return bpf_burst_get_send(msk);
 }
 
-SEC(".struct_ops")
+SEC(".struct_ops.link")
 struct mptcp_sched_ops burst = {
 	.init		= (void *)mptcp_sched_burst_init,
 	.release	= (void *)mptcp_sched_burst_release,
-- 
2.45.2

[PATCH mptcp-next v7 5/5] Squash to "selftests/bpf: Add bpf_first scheduler & test"

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

Use the newly added bpf_for_each() helper to walk the conn_list.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 tools/testing/selftests/bpf/progs/mptcp_bpf.h       |  3 ---
 tools/testing/selftests/bpf/progs/mptcp_bpf_first.c | 10 ++++++++--
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/bpf/progs/mptcp_bpf.h b/tools/testing/selftests/bpf/progs/mptcp_bpf.h
index 376979a9c4f0..52bc8ac03508 100644
--- a/tools/testing/selftests/bpf/progs/mptcp_bpf.h
+++ b/tools/testing/selftests/bpf/progs/mptcp_bpf.h
@@ -60,7 +60,4 @@ extern bool bpf_ipv6_addr_v4mapped(const struct mptcp_addr_info *a) __ksym;
 extern void mptcp_subflow_set_scheduled(struct mptcp_subflow_context *subflow,
 					bool scheduled) __ksym;
 
-extern struct mptcp_subflow_context *
-bpf_mptcp_subflow_ctx_by_pos(const struct mptcp_sched_data *data, unsigned int pos) __ksym;
-
 #endif
diff --git a/tools/testing/selftests/bpf/progs/mptcp_bpf_first.c b/tools/testing/selftests/bpf/progs/mptcp_bpf_first.c
index d57399b407a7..f2f9e66455b6 100644
--- a/tools/testing/selftests/bpf/progs/mptcp_bpf_first.c
+++ b/tools/testing/selftests/bpf/progs/mptcp_bpf_first.c
@@ -20,11 +20,17 @@ SEC("struct_ops")
 int BPF_PROG(bpf_first_get_subflow, struct mptcp_sock *msk,
 	     struct mptcp_sched_data *data)
 {
-	mptcp_subflow_set_scheduled(bpf_mptcp_subflow_ctx_by_pos(data, 0), true);
+	struct mptcp_subflow_context *subflow;
+
+	bpf_for_each(mptcp_subflow, subflow, msk) {
+		mptcp_subflow_set_scheduled(subflow, true);
+		break;
+	}
+
 	return 0;
 }
 
-SEC(".struct_ops")
+SEC(".struct_ops.link")
 struct mptcp_sched_ops first = {
 	.init		= (void *)mptcp_sched_first_init,
 	.release	= (void *)mptcp_sched_first_release,
-- 
2.45.2

Re: [PATCH mptcp-next v7 0/5] use bpf_iter in bpf schedulers

[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-normal (only bpftest_all): Success! ✅
- KVM Validation: btf-debug (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/11455976017

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/6d7d2f41f644
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=901714


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 v7 2/5] Squash to "selftests/bpf: Add bpf_rr scheduler & test"

[Mat Martineau]

On Tue, 22 Oct 2024, Geliang Tang wrote:

> From: Geliang Tang <tanggeliang@kylinos.cn>
>
> Use the newly added bpf_for_each() helper to walk the conn_list.
>
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
> .../selftests/bpf/progs/mptcp_bpf_rr.c        | 26 ++++++++-----------
> 1 file changed, 11 insertions(+), 15 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/progs/mptcp_bpf_rr.c b/tools/testing/selftests/bpf/progs/mptcp_bpf_rr.c
> index 638ea6aa63b7..e02b8cfb262b 100644
> --- a/tools/testing/selftests/bpf/progs/mptcp_bpf_rr.c
> +++ b/tools/testing/selftests/bpf/progs/mptcp_bpf_rr.c
> @@ -34,10 +34,9 @@ SEC("struct_ops")
> int BPF_PROG(bpf_rr_get_subflow, struct mptcp_sock *msk,
> 	     struct mptcp_sched_data *data)
> {
> -	struct mptcp_subflow_context *subflow;
> +	struct mptcp_subflow_context *subflow, *next;
> 	struct mptcp_rr_storage *ptr;
> 	struct sock *last_snd = NULL;
> -	int nr = 0;
>
> 	ptr = bpf_sk_storage_get(&mptcp_rr_map, msk, 0,
> 				 BPF_LOCAL_STORAGE_GET_F_CREATE);

Hi Geliang -

Thanks for the series. The bpf_iter code simplifies these scheduler 
patches significantly!

> @@ -45,31 +44,28 @@ int BPF_PROG(bpf_rr_get_subflow, struct mptcp_sock *msk,
> 		return -1;
>
> 	last_snd = ptr->last_snd;
> +	next = bpf_mptcp_subflow_ctx(msk->first);
>
> -	for (int i = 0; i < data->subflows && i < MPTCP_SUBFLOWS_MAX; i++) {
> -		subflow = bpf_mptcp_subflow_ctx_by_pos(data, i);
> -		if (!last_snd || !subflow)
> +	bpf_for_each(mptcp_subflow, subflow, msk) {
> +		if (!last_snd)

last_snd is never changed inside the loop, it's more efficient to check it 
before bpf_for_each().

- Mat

> 			break;
>
> -		if (mptcp_subflow_tcp_sock(subflow) == last_snd) {
> -			if (i + 1 == MPTCP_SUBFLOWS_MAX ||
> -			    !bpf_mptcp_subflow_ctx_by_pos(data, i + 1))
> +		if (bpf_mptcp_subflow_tcp_sock(subflow) == last_snd) {
> +			subflow = bpf_iter_mptcp_subflow_next(&___it);
> +			if (!subflow)
> 				break;
>
> -			nr = i + 1;
> +			next = subflow;
> 			break;
> 		}
> 	}
>
> -	subflow = bpf_mptcp_subflow_ctx_by_pos(data, nr);
> -	if (!subflow)
> -		return -1;
> -	mptcp_subflow_set_scheduled(subflow, true);
> -	ptr->last_snd = mptcp_subflow_tcp_sock(subflow);
> +	mptcp_subflow_set_scheduled(next, true);
> +	ptr->last_snd = bpf_mptcp_subflow_tcp_sock(next);
> 	return 0;
> }
>
> -SEC(".struct_ops")
> +SEC(".struct_ops.link")
> struct mptcp_sched_ops rr = {
> 	.init		= (void *)mptcp_sched_rr_init,
> 	.release	= (void *)mptcp_sched_rr_release,
> -- 
> 2.45.2
>
>
>

Re: [PATCH mptcp-next v7 4/5] Squash to "selftests/bpf: Add bpf_burst scheduler & test"

[Mat Martineau]

On Tue, 22 Oct 2024, Geliang Tang wrote:

> From: Geliang Tang <tanggeliang@kylinos.cn>
>
> Use the newly added bpf_for_each() helper to walk the conn_list.
>
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
> .../selftests/bpf/progs/mptcp_bpf_burst.c     | 79 ++++++++++---------
> 1 file changed, 40 insertions(+), 39 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/progs/mptcp_bpf_burst.c b/tools/testing/selftests/bpf/progs/mptcp_bpf_burst.c
> index eb21119aa8f7..e7df5f048aa4 100644
> --- a/tools/testing/selftests/bpf/progs/mptcp_bpf_burst.c
> +++ b/tools/testing/selftests/bpf/progs/mptcp_bpf_burst.c
> @@ -11,6 +11,10 @@ char _license[] SEC("license") = "GPL";
>
> #define min(a, b) ((a) < (b) ? (a) : (b))
>
> +#define SSK_MODE_ACTIVE	0
> +#define SSK_MODE_BACKUP	1
> +#define SSK_MODE_MAX	2
> +

Hi Geliang -

> struct bpf_subflow_send_info {
> 	__u8 subflow_id;

If you store a subflow pointer here instead of an index, there's no need 
to add the mptcp_lookup_subflow_id() helper function, an extra 
iteration over conn_list is eliminated, and the code is more like 
mptcp_subflow_get_send().

- Mat

> 	__u64 linger_time;
> @@ -23,10 +27,6 @@ extern bool tcp_stream_memory_free(const struct sock *sk, int wake) __ksym;
> extern bool bpf_mptcp_subflow_queues_empty(struct sock *sk) __ksym;
> extern void mptcp_pm_subflow_chk_stale(const struct mptcp_sock *msk, struct sock *ssk) __ksym;
>
> -#define SSK_MODE_ACTIVE	0
> -#define SSK_MODE_BACKUP	1
> -#define SSK_MODE_MAX	2
> -
> static __always_inline __u64 div_u64(__u64 dividend, __u32 divisor)
> {
> 	return dividend / divisor;
> @@ -57,6 +57,19 @@ static __always_inline bool sk_stream_memory_free(const struct sock *sk)
> 	return __sk_stream_memory_free(sk, 0);
> }
>
> +static struct mptcp_subflow_context *
> +mptcp_lookup_subflow_by_id(struct mptcp_sock *msk, unsigned int id)
> +{
> +	struct mptcp_subflow_context *subflow;
> +
> +	bpf_for_each(mptcp_subflow, subflow, msk) {
> +		if (subflow->subflow_id == id)
> +			return subflow;
> +	}
> +
> +	return NULL;
> +}
> +
> SEC("struct_ops")
> void BPF_PROG(mptcp_sched_burst_init, struct mptcp_sock *msk)
> {
> @@ -67,8 +80,7 @@ void BPF_PROG(mptcp_sched_burst_release, struct mptcp_sock *msk)
> {
> }
>
> -static int bpf_burst_get_send(struct mptcp_sock *msk,
> -			      struct mptcp_sched_data *data)
> +static int bpf_burst_get_send(struct mptcp_sock *msk)
> {
> 	struct bpf_subflow_send_info send_info[SSK_MODE_MAX];
> 	struct mptcp_subflow_context *subflow;
> @@ -84,16 +96,10 @@ static int bpf_burst_get_send(struct mptcp_sock *msk,
> 		send_info[i].linger_time = -1;
> 	}
>
> -	for (i = 0; i < data->subflows && i < MPTCP_SUBFLOWS_MAX; i++) {
> -		bool backup;
> +	bpf_for_each(mptcp_subflow, subflow, msk) {
> +		bool backup = subflow->backup || subflow->request_bkup;
>
> -		subflow = bpf_mptcp_subflow_ctx_by_pos(data, i);
> -		if (!subflow)
> -			break;
> -
> -		backup = subflow->backup || subflow->request_bkup;
> -
> -		ssk = mptcp_subflow_tcp_sock(subflow);
> +		ssk = bpf_mptcp_subflow_tcp_sock(subflow);
> 		if (!mptcp_subflow_active(subflow))
> 			continue;
>
> @@ -109,7 +115,7 @@ static int bpf_burst_get_send(struct mptcp_sock *msk,
>
> 		linger_time = div_u64((__u64)ssk->sk_wmem_queued << 32, pace);
> 		if (linger_time < send_info[backup].linger_time) {
> -			send_info[backup].subflow_id = i;
> +			send_info[backup].subflow_id = subflow->subflow_id;
> 			send_info[backup].linger_time = linger_time;
> 		}
> 	}
> @@ -119,10 +125,10 @@ static int bpf_burst_get_send(struct mptcp_sock *msk,
> 	if (!nr_active)
> 		send_info[SSK_MODE_ACTIVE].subflow_id = send_info[SSK_MODE_BACKUP].subflow_id;
>
> -	subflow = bpf_mptcp_subflow_ctx_by_pos(data, send_info[SSK_MODE_ACTIVE].subflow_id);
> +	subflow = mptcp_lookup_subflow_by_id(msk, send_info[SSK_MODE_ACTIVE].subflow_id);
> 	if (!subflow)
> 		return -1;
> -	ssk = mptcp_subflow_tcp_sock(subflow);
> +	ssk = bpf_mptcp_subflow_tcp_sock(subflow);
> 	if (!ssk || !sk_stream_memory_free(ssk))
> 		return -1;
>
> @@ -141,23 +147,18 @@ static int bpf_burst_get_send(struct mptcp_sock *msk,
> 	return 0;
> }
>
> -static int bpf_burst_get_retrans(struct mptcp_sock *msk,
> -				 struct mptcp_sched_data *data)
> +static int bpf_burst_get_retrans(struct mptcp_sock *msk)
> {
> -	int backup = MPTCP_SUBFLOWS_MAX, pick = MPTCP_SUBFLOWS_MAX, subflow_id;
> +	struct sock *backup = NULL, *pick = NULL;
> 	struct mptcp_subflow_context *subflow;
> 	int min_stale_count = INT_MAX;
> -	struct sock *ssk;
>
> -	for (int i = 0; i < data->subflows && i < MPTCP_SUBFLOWS_MAX; i++) {
> -		subflow = bpf_mptcp_subflow_ctx_by_pos(data, i);
> -		if (!subflow)
> -			break;
> +	bpf_for_each(mptcp_subflow, subflow, msk) {
> +		struct sock *ssk = bpf_mptcp_subflow_tcp_sock(subflow);
>
> 		if (!mptcp_subflow_active(subflow))
> 			continue;
>
> -		ssk = mptcp_subflow_tcp_sock(subflow);
> 		/* still data outstanding at TCP level? skip this */
> 		if (!tcp_rtx_and_write_queues_empty(ssk)) {
> 			mptcp_pm_subflow_chk_stale(msk, ssk);
> @@ -166,23 +167,23 @@ static int bpf_burst_get_retrans(struct mptcp_sock *msk,
> 		}
>
> 		if (subflow->backup || subflow->request_bkup) {
> -			if (backup == MPTCP_SUBFLOWS_MAX)
> -				backup = i;
> +			if (!backup)
> +				backup = ssk;
> 			continue;
> 		}
>
> -		if (pick == MPTCP_SUBFLOWS_MAX)
> -			pick = i;
> +		if (!pick)
> +			pick = ssk;
> 	}
>
> -	if (pick < MPTCP_SUBFLOWS_MAX) {
> -		subflow_id = pick;
> +	if (pick)
> 		goto out;
> -	}
> -	subflow_id = min_stale_count > 1 ? backup : MPTCP_SUBFLOWS_MAX;
> +	pick = min_stale_count > 1 ? backup : NULL;
>
> out:
> -	subflow = bpf_mptcp_subflow_ctx_by_pos(data, subflow_id);
> +	if (!pick)
> +		return -1;
> +	subflow = bpf_mptcp_subflow_ctx(pick);
> 	if (!subflow)
> 		return -1;
> 	mptcp_subflow_set_scheduled(subflow, true);
> @@ -194,11 +195,11 @@ int BPF_PROG(bpf_burst_get_subflow, struct mptcp_sock *msk,
> 	     struct mptcp_sched_data *data)
> {
> 	if (data->reinject)
> -		return bpf_burst_get_retrans(msk, data);
> -	return bpf_burst_get_send(msk, data);
> +		return bpf_burst_get_retrans(msk);
> +	return bpf_burst_get_send(msk);
> }
>
> -SEC(".struct_ops")
> +SEC(".struct_ops.link")
> struct mptcp_sched_ops burst = {
> 	.init		= (void *)mptcp_sched_burst_init,
> 	.release	= (void *)mptcp_sched_burst_release,
> -- 
> 2.45.2
>
>
>

Re: [PATCH mptcp-next v7 5/5] Squash to "selftests/bpf: Add bpf_first scheduler & test"

[Mat Martineau]

On Tue, 22 Oct 2024, Geliang Tang wrote:

> From: Geliang Tang <tanggeliang@kylinos.cn>
>
> Use the newly added bpf_for_each() helper to walk the conn_list.
>
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
> tools/testing/selftests/bpf/progs/mptcp_bpf.h       |  3 ---
> tools/testing/selftests/bpf/progs/mptcp_bpf_first.c | 10 ++++++++--
> 2 files changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/progs/mptcp_bpf.h b/tools/testing/selftests/bpf/progs/mptcp_bpf.h
> index 376979a9c4f0..52bc8ac03508 100644
> --- a/tools/testing/selftests/bpf/progs/mptcp_bpf.h
> +++ b/tools/testing/selftests/bpf/progs/mptcp_bpf.h
> @@ -60,7 +60,4 @@ extern bool bpf_ipv6_addr_v4mapped(const struct mptcp_addr_info *a) __ksym;
> extern void mptcp_subflow_set_scheduled(struct mptcp_subflow_context *subflow,
> 					bool scheduled) __ksym;
>

Hi Geliang -

> -extern struct mptcp_subflow_context *
> -bpf_mptcp_subflow_ctx_by_pos(const struct mptcp_sched_data *data, unsigned int pos) __ksym;
> -

After this is removed, there are no remaining users of 
bpf_mptcp_subflow_ctx_by_pos(). I suggest removing that function.

> #endif
> diff --git a/tools/testing/selftests/bpf/progs/mptcp_bpf_first.c b/tools/testing/selftests/bpf/progs/mptcp_bpf_first.c
> index d57399b407a7..f2f9e66455b6 100644
> --- a/tools/testing/selftests/bpf/progs/mptcp_bpf_first.c
> +++ b/tools/testing/selftests/bpf/progs/mptcp_bpf_first.c
> @@ -20,11 +20,17 @@ SEC("struct_ops")
> int BPF_PROG(bpf_first_get_subflow, struct mptcp_sock *msk,
> 	     struct mptcp_sched_data *data)
> {
> -	mptcp_subflow_set_scheduled(bpf_mptcp_subflow_ctx_by_pos(data, 0), true);
> +	struct mptcp_subflow_context *subflow;
> +
> +	bpf_for_each(mptcp_subflow, subflow, msk) {
> +		mptcp_subflow_set_scheduled(subflow, true);
> +		break;
> +	}
> +

The iterator makes this more complicated. Use 
bpf_mptcp_subflow_ctx(msk->first) like bpf_rr_get_subflow()?

- Mat

> 	return 0;
> }
>
> -SEC(".struct_ops")
> +SEC(".struct_ops.link")
> struct mptcp_sched_ops first = {
> 	.init		= (void *)mptcp_sched_first_init,
> 	.release	= (void *)mptcp_sched_first_release,
> -- 
> 2.45.2
>
>
>

Re: [PATCH mptcp-next v7 2/5] Squash to "selftests/bpf: Add bpf_rr scheduler & test"

[Geliang Tang]

Hi Mat,

Thanks for your review.

On Tue, 2024-10-22 at 16:52 -0700, Mat Martineau wrote:
> On Tue, 22 Oct 2024, Geliang Tang wrote:
> 
> > From: Geliang Tang <tanggeliang@kylinos.cn>
> > 
> > Use the newly added bpf_for_each() helper to walk the conn_list.
> > 
> > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> > ---
> > .../selftests/bpf/progs/mptcp_bpf_rr.c        | 26 ++++++++--------
> > ---
> > 1 file changed, 11 insertions(+), 15 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/bpf/progs/mptcp_bpf_rr.c
> > b/tools/testing/selftests/bpf/progs/mptcp_bpf_rr.c
> > index 638ea6aa63b7..e02b8cfb262b 100644
> > --- a/tools/testing/selftests/bpf/progs/mptcp_bpf_rr.c
> > +++ b/tools/testing/selftests/bpf/progs/mptcp_bpf_rr.c
> > @@ -34,10 +34,9 @@ SEC("struct_ops")
> > int BPF_PROG(bpf_rr_get_subflow, struct mptcp_sock *msk,
> > 	     struct mptcp_sched_data *data)
> > {
> > -	struct mptcp_subflow_context *subflow;
> > +	struct mptcp_subflow_context *subflow, *next;
> > 	struct mptcp_rr_storage *ptr;
> > 	struct sock *last_snd = NULL;
> > -	int nr = 0;
> > 
> > 	ptr = bpf_sk_storage_get(&mptcp_rr_map, msk, 0,
> > 				 BPF_LOCAL_STORAGE_GET_F_CREATE);
> 
> Hi Geliang -
> 
> Thanks for the series. The bpf_iter code simplifies these scheduler 
> patches significantly!
> 
> > @@ -45,31 +44,28 @@ int BPF_PROG(bpf_rr_get_subflow, struct
> > mptcp_sock *msk,
> > 		return -1;
> > 
> > 	last_snd = ptr->last_snd;
> > +	next = bpf_mptcp_subflow_ctx(msk->first);
> > 
> > -	for (int i = 0; i < data->subflows && i <
> > MPTCP_SUBFLOWS_MAX; i++) {
> > -		subflow = bpf_mptcp_subflow_ctx_by_pos(data, i);
> > -		if (!last_snd || !subflow)
> > +	bpf_for_each(mptcp_subflow, subflow, msk) {
> > +		if (!last_snd)
> 
> last_snd is never changed inside the loop, it's more efficient to
> check it 
> before bpf_for_each().

This is indeed better, updated in v8.

> 
> - Mat
> 
> > 			break;
> > 
> > -		if (mptcp_subflow_tcp_sock(subflow) == last_snd) {
> > -			if (i + 1 == MPTCP_SUBFLOWS_MAX ||
> > -			    !bpf_mptcp_subflow_ctx_by_pos(data, i
> > + 1))
> > +		if (bpf_mptcp_subflow_tcp_sock(subflow) ==
> > last_snd) {
> > +			subflow =
> > bpf_iter_mptcp_subflow_next(&___it);
> > +			if (!subflow)
> > 				break;
> > 
> > -			nr = i + 1;
> > +			next = subflow;
> > 			break;
> > 		}
> > 	}
> > 
> > -	subflow = bpf_mptcp_subflow_ctx_by_pos(data, nr);
> > -	if (!subflow)
> > -		return -1;
> > -	mptcp_subflow_set_scheduled(subflow, true);
> > -	ptr->last_snd = mptcp_subflow_tcp_sock(subflow);
> > +	mptcp_subflow_set_scheduled(next, true);
> > +	ptr->last_snd = bpf_mptcp_subflow_tcp_sock(next);
> > 	return 0;
> > }
> > 
> > -SEC(".struct_ops")
> > +SEC(".struct_ops.link")
> > struct mptcp_sched_ops rr = {
> > 	.init		= (void *)mptcp_sched_rr_init,
> > 	.release	= (void *)mptcp_sched_rr_release,
> > -- 
> > 2.45.2
> > 
> > 
> > 

Re: [PATCH mptcp-next v7 4/5] Squash to "selftests/bpf: Add bpf_burst scheduler & test"

[Geliang Tang]

On Tue, 2024-10-22 at 17:03 -0700, Mat Martineau wrote:
> On Tue, 22 Oct 2024, Geliang Tang wrote:
> 
> > From: Geliang Tang <tanggeliang@kylinos.cn>
> > 
> > Use the newly added bpf_for_each() helper to walk the conn_list.
> > 
> > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> > ---
> > .../selftests/bpf/progs/mptcp_bpf_burst.c     | 79 ++++++++++------
> > ---
> > 1 file changed, 40 insertions(+), 39 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/bpf/progs/mptcp_bpf_burst.c
> > b/tools/testing/selftests/bpf/progs/mptcp_bpf_burst.c
> > index eb21119aa8f7..e7df5f048aa4 100644
> > --- a/tools/testing/selftests/bpf/progs/mptcp_bpf_burst.c
> > +++ b/tools/testing/selftests/bpf/progs/mptcp_bpf_burst.c
> > @@ -11,6 +11,10 @@ char _license[] SEC("license") = "GPL";
> > 
> > #define min(a, b) ((a) < (b) ? (a) : (b))
> > 
> > +#define SSK_MODE_ACTIVE	0
> > +#define SSK_MODE_BACKUP	1
> > +#define SSK_MODE_MAX	2
> > +
> 
> Hi Geliang -
> 
> > struct bpf_subflow_send_info {
> > 	__u8 subflow_id;
> 
> If you store a subflow pointer here instead of an index, there's no
> need 
> to add the mptcp_lookup_subflow_id() helper function, an extra 
> iteration over conn_list is eliminated, and the code is more like 
> mptcp_subflow_get_send().

I thought so too. Two changes were needed to achieve this.

1. Move sk_stream_memory_free check inside bpf_for_each() loop.
2. Implement mptcp_subflow_set_scheduled helper in BPF.

> 
> - Mat
> 
> > 	__u64 linger_time;
> > @@ -23,10 +27,6 @@ extern bool tcp_stream_memory_free(const struct
> > sock *sk, int wake) __ksym;
> > extern bool bpf_mptcp_subflow_queues_empty(struct sock *sk) __ksym;
> > extern void mptcp_pm_subflow_chk_stale(const struct mptcp_sock
> > *msk, struct sock *ssk) __ksym;
> > 
> > -#define SSK_MODE_ACTIVE	0
> > -#define SSK_MODE_BACKUP	1
> > -#define SSK_MODE_MAX	2
> > -
> > static __always_inline __u64 div_u64(__u64 dividend, __u32 divisor)
> > {
> > 	return dividend / divisor;
> > @@ -57,6 +57,19 @@ static __always_inline bool
> > sk_stream_memory_free(const struct sock *sk)
> > 	return __sk_stream_memory_free(sk, 0);
> > }
> > 
> > +static struct mptcp_subflow_context *
> > +mptcp_lookup_subflow_by_id(struct mptcp_sock *msk, unsigned int
> > id)
> > +{
> > +	struct mptcp_subflow_context *subflow;
> > +
> > +	bpf_for_each(mptcp_subflow, subflow, msk) {
> > +		if (subflow->subflow_id == id)
> > +			return subflow;
> > +	}
> > +
> > +	return NULL;
> > +}
> > +
> > SEC("struct_ops")
> > void BPF_PROG(mptcp_sched_burst_init, struct mptcp_sock *msk)
> > {
> > @@ -67,8 +80,7 @@ void BPF_PROG(mptcp_sched_burst_release, struct
> > mptcp_sock *msk)
> > {
> > }
> > 
> > -static int bpf_burst_get_send(struct mptcp_sock *msk,
> > -			      struct mptcp_sched_data *data)
> > +static int bpf_burst_get_send(struct mptcp_sock *msk)
> > {
> > 	struct bpf_subflow_send_info send_info[SSK_MODE_MAX];
> > 	struct mptcp_subflow_context *subflow;
> > @@ -84,16 +96,10 @@ static int bpf_burst_get_send(struct mptcp_sock
> > *msk,
> > 		send_info[i].linger_time = -1;
> > 	}
> > 
> > -	for (i = 0; i < data->subflows && i < MPTCP_SUBFLOWS_MAX;
> > i++) {
> > -		bool backup;
> > +	bpf_for_each(mptcp_subflow, subflow, msk) {
> > +		bool backup = subflow->backup || subflow-
> > >request_bkup;
> > 
> > -		subflow = bpf_mptcp_subflow_ctx_by_pos(data, i);
> > -		if (!subflow)
> > -			break;
> > -
> > -		backup = subflow->backup || subflow->request_bkup;
> > -
> > -		ssk = mptcp_subflow_tcp_sock(subflow);
> > +		ssk = bpf_mptcp_subflow_tcp_sock(subflow);
> > 		if (!mptcp_subflow_active(subflow))
> > 			continue;
> > 
> > @@ -109,7 +115,7 @@ static int bpf_burst_get_send(struct mptcp_sock
> > *msk,
> > 
> > 		linger_time = div_u64((__u64)ssk->sk_wmem_queued
> > << 32, pace);
> > 		if (linger_time < send_info[backup].linger_time) {
> > -			send_info[backup].subflow_id = i;
> > +			send_info[backup].subflow_id = subflow-
> > >subflow_id;
> > 			send_info[backup].linger_time =
> > linger_time;
> > 		}
> > 	}
> > @@ -119,10 +125,10 @@ static int bpf_burst_get_send(struct
> > mptcp_sock *msk,
> > 	if (!nr_active)
> > 		send_info[SSK_MODE_ACTIVE].subflow_id =
> > send_info[SSK_MODE_BACKUP].subflow_id;
> > 
> > -	subflow = bpf_mptcp_subflow_ctx_by_pos(data,
> > send_info[SSK_MODE_ACTIVE].subflow_id);
> > +	subflow = mptcp_lookup_subflow_by_id(msk,
> > send_info[SSK_MODE_ACTIVE].subflow_id);

BPF does not allow access to subflow in this way:

	subflow = send_info[SSK_MODE_ACTIVE].subflow;

So I have to use bpf_core_cast() here as:

	subflow = bpf_core_cast(send_info[SSK_MODE_ACTIVE].subflow,
				struct mptcp_subflow_context);

> > 	if (!subflow)
> > 		return -1;
> > -	ssk = mptcp_subflow_tcp_sock(subflow);
> > +	ssk = bpf_mptcp_subflow_tcp_sock(subflow);

Here BPF doesn't allow passing a cast pointer (subflow) to a kfunc
(bpf_mptcp_subflow_tcp_sock). Fortunately we can use
mptcp_subflow_tcp_sock instead, which is a BPF helper, not a kfunc:

	 ssk = mptcp_subflow_tcp_sock(subflow);

> > 	if (!ssk || !sk_stream_memory_free(ssk))

Again, BPF does not allow passing a cast pointer to a kfunc,
sk_stream_memory_free(ssk) fails.

It's not possible to implement a sk_stream_memory_free function in BPF,
it's too complicated. So the approach I took in v8 was to move this
sk_stream_memory_free check forward to the position of
mptcp_subflow_active in bpf_for_each() loop. I think this doesn't
change the logic of the burst scheduler, but I'd like to hear your
opinion.

After this, mptcp_subflow_set_scheduled(subflow, true) is not allowed
too. So I have to implement a mptcp_subflow_set_scheduled helper in
BPF. It's easy since WRITE_ONCE() is defined in progs/map_kptr.c.

> > 		return -1;
> > 
> > @@ -141,23 +147,18 @@ static int bpf_burst_get_send(struct
> > mptcp_sock *msk,
> > 	return 0;
> > }
> > 
> > -static int bpf_burst_get_retrans(struct mptcp_sock *msk,
> > -				 struct mptcp_sched_data *data)
> > +static int bpf_burst_get_retrans(struct mptcp_sock *msk)
> > {
> > -	int backup = MPTCP_SUBFLOWS_MAX, pick =
> > MPTCP_SUBFLOWS_MAX, subflow_id;
> > +	struct sock *backup = NULL, *pick = NULL;
> > 	struct mptcp_subflow_context *subflow;
> > 	int min_stale_count = INT_MAX;
> > -	struct sock *ssk;
> > 
> > -	for (int i = 0; i < data->subflows && i <
> > MPTCP_SUBFLOWS_MAX; i++) {
> > -		subflow = bpf_mptcp_subflow_ctx_by_pos(data, i);
> > -		if (!subflow)
> > -			break;
> > +	bpf_for_each(mptcp_subflow, subflow, msk) {
> > +		struct sock *ssk =
> > bpf_mptcp_subflow_tcp_sock(subflow);
> > 
> > 		if (!mptcp_subflow_active(subflow))
> > 			continue;
> > 
> > -		ssk = mptcp_subflow_tcp_sock(subflow);
> > 		/* still data outstanding at TCP level? skip this
> > */
> > 		if (!tcp_rtx_and_write_queues_empty(ssk)) {
> > 			mptcp_pm_subflow_chk_stale(msk, ssk);
> > @@ -166,23 +167,23 @@ static int bpf_burst_get_retrans(struct
> > mptcp_sock *msk,
> > 		}
> > 
> > 		if (subflow->backup || subflow->request_bkup) {
> > -			if (backup == MPTCP_SUBFLOWS_MAX)
> > -				backup = i;
> > +			if (!backup)
> > +				backup = ssk;
> > 			continue;
> > 		}
> > 
> > -		if (pick == MPTCP_SUBFLOWS_MAX)
> > -			pick = i;
> > +		if (!pick)
> > +			pick = ssk;
> > 	}
> > 
> > -	if (pick < MPTCP_SUBFLOWS_MAX) {
> > -		subflow_id = pick;
> > +	if (pick)
> > 		goto out;
> > -	}
> > -	subflow_id = min_stale_count > 1 ? backup :
> > MPTCP_SUBFLOWS_MAX;
> > +	pick = min_stale_count > 1 ? backup : NULL;
> > 
> > out:
> > -	subflow = bpf_mptcp_subflow_ctx_by_pos(data, subflow_id);
> > +	if (!pick)
> > +		return -1;
> > +	subflow = bpf_mptcp_subflow_ctx(pick);
> > 	if (!subflow)
> > 		return -1;
> > 	mptcp_subflow_set_scheduled(subflow, true);
> > @@ -194,11 +195,11 @@ int BPF_PROG(bpf_burst_get_subflow, struct
> > mptcp_sock *msk,
> > 	     struct mptcp_sched_data *data)
> > {
> > 	if (data->reinject)
> > -		return bpf_burst_get_retrans(msk, data);
> > -	return bpf_burst_get_send(msk, data);
> > +		return bpf_burst_get_retrans(msk);
> > +	return bpf_burst_get_send(msk);
> > }
> > 
> > -SEC(".struct_ops")
> > +SEC(".struct_ops.link")
> > struct mptcp_sched_ops burst = {
> > 	.init		= (void *)mptcp_sched_burst_init,
> > 	.release	= (void *)mptcp_sched_burst_release,
> > -- 
> > 2.45.2
> > 
> > 
> > 

Re: [PATCH mptcp-next v7 5/5] Squash to "selftests/bpf: Add bpf_first scheduler & test"

[Geliang Tang]

On Tue, 2024-10-22 at 17:10 -0700, Mat Martineau wrote:
> On Tue, 22 Oct 2024, Geliang Tang wrote:
> 
> > From: Geliang Tang <tanggeliang@kylinos.cn>
> > 
> > Use the newly added bpf_for_each() helper to walk the conn_list.
> > 
> > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> > ---
> > tools/testing/selftests/bpf/progs/mptcp_bpf.h       |  3 ---
> > tools/testing/selftests/bpf/progs/mptcp_bpf_first.c | 10 ++++++++--
> > 2 files changed, 8 insertions(+), 5 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/bpf/progs/mptcp_bpf.h
> > b/tools/testing/selftests/bpf/progs/mptcp_bpf.h
> > index 376979a9c4f0..52bc8ac03508 100644
> > --- a/tools/testing/selftests/bpf/progs/mptcp_bpf.h
> > +++ b/tools/testing/selftests/bpf/progs/mptcp_bpf.h
> > @@ -60,7 +60,4 @@ extern bool bpf_ipv6_addr_v4mapped(const struct
> > mptcp_addr_info *a) __ksym;
> > extern void mptcp_subflow_set_scheduled(struct
> > mptcp_subflow_context *subflow,
> > 					bool scheduled) __ksym;
> > 
> 
> Hi Geliang -
> 
> > -extern struct mptcp_subflow_context *
> > -bpf_mptcp_subflow_ctx_by_pos(const struct mptcp_sched_data *data,
> > unsigned int pos) __ksym;
> > -
> 
> After this is removed, there are no remaining users of 
> bpf_mptcp_subflow_ctx_by_pos(). I suggest removing that function.

Indeed, I added these cleanups patches to v8 as well.

Thanks again.
-Geliang

> 
> > #endif
> > diff --git a/tools/testing/selftests/bpf/progs/mptcp_bpf_first.c
> > b/tools/testing/selftests/bpf/progs/mptcp_bpf_first.c
> > index d57399b407a7..f2f9e66455b6 100644
> > --- a/tools/testing/selftests/bpf/progs/mptcp_bpf_first.c
> > +++ b/tools/testing/selftests/bpf/progs/mptcp_bpf_first.c
> > @@ -20,11 +20,17 @@ SEC("struct_ops")
> > int BPF_PROG(bpf_first_get_subflow, struct mptcp_sock *msk,
> > 	     struct mptcp_sched_data *data)
> > {
> > -
> > 	mptcp_subflow_set_scheduled(bpf_mptcp_subflow_ctx_by_pos(data, 0),true);
> > +	struct mptcp_subflow_context *subflow;
> > +
> > +	bpf_for_each(mptcp_subflow, subflow, msk) {
> > +		mptcp_subflow_set_scheduled(subflow, true);
> > +		break;
> > +	}
> > +
> 
> The iterator makes this more complicated. Use 
> bpf_mptcp_subflow_ctx(msk->first) like bpf_rr_get_subflow()?
> 
> - Mat
> 
> > 	return 0;
> > }
> > 
> > -SEC(".struct_ops")
> > +SEC(".struct_ops.link")
> > struct mptcp_sched_ops first = {
> > 	.init		= (void *)mptcp_sched_first_init,
> > 	.release	= (void *)mptcp_sched_first_release,
> > -- 
> > 2.45.2
> > 
> > 
> >