[PATCH mptcp-next v3 0/5] add bpf_stale scheduler

[PATCH mptcp-next v3 0/5] add bpf_stale scheduler

[Geliang Tang]

v3:
 - init sk_storage in .init, delete it in .release.
We invoke bpf_sk_storage_get() to get the sk_storage map many times. Only
the first call is the slow path (see bpf_local_storage_lookup in
kernel/bpf/bpf_local_storage.c), alloc the map and cache it. The
subsequent calls are all in fast path, the cache hits. So we should
first call bpf_sk_storage_get in .init, then call it many times in
.get_subflow.
 - if no subflow is scheduled, it should return '-1' to indicate an error.

v2:
 - store subflow ids instead of storing subflow pointers in sk_storage.

v1:
- This patchset adds the new bpf_stale scheduler. Use sk_storage to save
the stale map instead of using subflow->stale flag to manage it.

Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/349

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

 tools/testing/selftests/bpf/bpf_tcp_helpers.h |   1 +
 .../testing/selftests/bpf/prog_tests/mptcp.c  |  38 +++++
 .../selftests/bpf/progs/mptcp_bpf_bkup.c      |   4 +-
 .../selftests/bpf/progs/mptcp_bpf_burst.c     |   2 +
 .../selftests/bpf/progs/mptcp_bpf_rr.c        |   2 +
 .../selftests/bpf/progs/mptcp_bpf_stale.c     | 152 ++++++++++++++++++
 6 files changed, 198 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/bpf/progs/mptcp_bpf_stale.c

-- 
2.35.3

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

[Geliang Tang]

Return -1 when no subflow is selected.

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 tools/testing/selftests/bpf/progs/mptcp_bpf_bkup.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/progs/mptcp_bpf_bkup.c b/tools/testing/selftests/bpf/progs/mptcp_bpf_bkup.c
index 7e66ab839d14..904186fb6750 100644
--- a/tools/testing/selftests/bpf/progs/mptcp_bpf_bkup.c
+++ b/tools/testing/selftests/bpf/progs/mptcp_bpf_bkup.c
@@ -36,8 +36,10 @@ int BPF_STRUCT_OPS(bpf_bkup_get_subflow, struct mptcp_sock *msk,
 		}
 	}
 
-	if (nr != -1)
+	if (nr != -1) {
 		mptcp_subflow_set_scheduled(mptcp_subflow_ctx_by_pos(data, nr), true);
+		return -1;
+	}
 	return 0;
 }
 
-- 
2.35.3

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

[Geliang Tang]

Alloc and init mptcp_rr_map first in mptcp_sched_rr_init().

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 tools/testing/selftests/bpf/progs/mptcp_bpf_rr.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/testing/selftests/bpf/progs/mptcp_bpf_rr.c b/tools/testing/selftests/bpf/progs/mptcp_bpf_rr.c
index 7a5c058d2408..347ffad90860 100644
--- a/tools/testing/selftests/bpf/progs/mptcp_bpf_rr.c
+++ b/tools/testing/selftests/bpf/progs/mptcp_bpf_rr.c
@@ -20,6 +20,8 @@ struct {
 SEC("struct_ops/mptcp_sched_rr_init")
 void BPF_PROG(mptcp_sched_rr_init, struct mptcp_sock *msk)
 {
+	bpf_sk_storage_get(&mptcp_rr_map, msk, 0,
+			   BPF_LOCAL_STORAGE_GET_F_CREATE);
 }
 
 SEC("struct_ops/mptcp_sched_rr_release")
-- 
2.35.3

[PATCH mptcp-next v3 3/5] Squash to "selftests/bpf: Add bpf_burst scheduler"

[Geliang Tang]

Alloc and init sk_storage in data_init().

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 tools/testing/selftests/bpf/progs/mptcp_bpf_burst.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/testing/selftests/bpf/progs/mptcp_bpf_burst.c b/tools/testing/selftests/bpf/progs/mptcp_bpf_burst.c
index 2962067568e7..b6a8a051741d 100644
--- a/tools/testing/selftests/bpf/progs/mptcp_bpf_burst.c
+++ b/tools/testing/selftests/bpf/progs/mptcp_bpf_burst.c
@@ -69,6 +69,8 @@ static __always_inline bool sk_stream_memory_free(const struct sock *sk)
 SEC("struct_ops/mptcp_sched_burst_init")
 void BPF_PROG(mptcp_sched_burst_init, struct mptcp_sock *msk)
 {
+	bpf_sk_storage_get(&mptcp_burst_map, msk, 0,
+			   BPF_LOCAL_STORAGE_GET_F_CREATE);
 }
 
 SEC("struct_ops/mptcp_sched_burst_release")
-- 
2.35.3

[PATCH mptcp-next v3 4/5] selftests/bpf: Add bpf_stale scheduler

[Geliang Tang]

This patch implements the setting a subflow as stale/unstale in BPF MPTCP
scheduler, named bpf_stale. The staled subflow id will be added into a
map in sk_storage.

Two helper mptcp_subflow_set_stale() and mptcp_subflow_clear_stale() are
added.

In this test, subflow 1 is set as stale in bpf_stale_data_init(). Each
subflow is checked whether it's a stale one in bpf_stale_get_subflow() to
select a unstale subflow to send data.

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 tools/testing/selftests/bpf/bpf_tcp_helpers.h |   1 +
 .../selftests/bpf/progs/mptcp_bpf_stale.c     | 152 ++++++++++++++++++
 2 files changed, 153 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/progs/mptcp_bpf_stale.c

diff --git a/tools/testing/selftests/bpf/bpf_tcp_helpers.h b/tools/testing/selftests/bpf/bpf_tcp_helpers.h
index b687f91f2da8..33246629fa36 100644
--- a/tools/testing/selftests/bpf/bpf_tcp_helpers.h
+++ b/tools/testing/selftests/bpf/bpf_tcp_helpers.h
@@ -239,6 +239,7 @@ struct mptcp_subflow_context {
 	unsigned long avg_pacing_rate;
 	__u32	backup : 1;
 	__u8	stale_count;
+	__u32	subflow_id;
 	struct	sock *tcp_sock;	    /* tcp sk backpointer */
 } __attribute__((preserve_access_index));
 
diff --git a/tools/testing/selftests/bpf/progs/mptcp_bpf_stale.c b/tools/testing/selftests/bpf/progs/mptcp_bpf_stale.c
new file mode 100644
index 000000000000..08c857f79221
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/mptcp_bpf_stale.c
@@ -0,0 +1,152 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2023, SUSE. */
+
+#include <linux/bpf.h>
+#include "bpf_tcp_helpers.h"
+
+char _license[] SEC("license") = "GPL";
+
+struct mptcp_stale_storage {
+	__u8 nr;
+	__u32 ids[MPTCP_SUBFLOWS_MAX];
+};
+
+struct {
+	__uint(type, BPF_MAP_TYPE_SK_STORAGE);
+	__uint(map_flags, BPF_F_NO_PREALLOC);
+	__type(key, int);
+	__type(value, struct mptcp_stale_storage);
+} mptcp_stale_map SEC(".maps");
+
+static void mptcp_subflow_set_stale(struct mptcp_stale_storage *storage,
+				    __u32 subflow_id)
+{
+	if (!subflow_id)
+		return;
+
+	for (int i = 0; i < storage->nr && i < MPTCP_SUBFLOWS_MAX; i++) {
+		if (storage->ids[i] == subflow_id)
+			return;
+	}
+
+	if (storage->nr < MPTCP_SUBFLOWS_MAX - 1)
+		storage->ids[storage->nr++] = subflow_id;
+}
+
+static void mptcp_subflow_clear_stale(struct mptcp_stale_storage *storage,
+				      __u32 subflow_id)
+{
+	if (!subflow_id)
+		return;
+
+	for (int i = 0; i < storage->nr && i < MPTCP_SUBFLOWS_MAX; i++) {
+		if (storage->ids[i] == subflow_id) {
+			for (int j = i; j < MPTCP_SUBFLOWS_MAX - 1; j++) {
+				if (!storage->ids[j + 1])
+					break;
+				storage->ids[j] = storage->ids[j + 1];
+				storage->ids[j + 1] = 0;
+			}
+			storage->nr--;
+			return;
+		}
+	}
+}
+
+static bool mptcp_subflow_is_stale(struct mptcp_stale_storage *storage,
+				   __u32 subflow_id)
+{
+	for (int i = 0; i < storage->nr && i < MPTCP_SUBFLOWS_MAX; i++) {
+		if (storage->ids[i] == subflow_id)
+			return true;
+	}
+
+	return false;
+}
+
+static bool mptcp_subflow_is_active(struct mptcp_sched_data *data,
+				    __u32 subflow_id)
+{
+	for (int i = 0; i < data->subflows && i < MPTCP_SUBFLOWS_MAX; i++) {
+		struct mptcp_subflow_context *subflow;
+
+		subflow = mptcp_subflow_ctx_by_pos(data, i);
+		if (!subflow)
+			break;
+		if (subflow->subflow_id == subflow_id)
+			return true;
+	}
+
+	return false;
+}
+
+SEC("struct_ops/mptcp_sched_stale_init")
+void BPF_PROG(mptcp_sched_stale_init, struct mptcp_sock *msk)
+{
+	struct mptcp_stale_storage *storage;
+
+	storage = bpf_sk_storage_get(&mptcp_stale_map, msk, 0,
+				     BPF_LOCAL_STORAGE_GET_F_CREATE);
+	if (!storage)
+		return;
+
+	storage->nr = 0;
+}
+
+SEC("struct_ops/mptcp_sched_stale_release")
+void BPF_PROG(mptcp_sched_stale_release, struct mptcp_sock *msk)
+{
+	bpf_sk_storage_delete(&mptcp_stale_map, msk);
+}
+
+int BPF_STRUCT_OPS(bpf_stale_get_subflow, struct mptcp_sock *msk,
+		   struct mptcp_sched_data *data)
+{
+	struct mptcp_subflow_context *subflow;
+	struct mptcp_stale_storage *storage;
+	int nr = -1;
+
+	storage = bpf_sk_storage_get(&mptcp_stale_map, msk, 0,
+				     BPF_LOCAL_STORAGE_GET_F_CREATE);
+	if (!storage)
+		return -1;
+
+	mptcp_sched_data_set_contexts(msk, data);
+
+	/* Handle invalid subflow ids for subflows that have been closed */
+	for (int i = 0; i < storage->nr && i < MPTCP_SUBFLOWS_MAX; i++) {
+		if (!mptcp_subflow_is_active(data, storage->ids[i]))
+			mptcp_subflow_clear_stale(storage, storage->ids[i]);
+	}
+
+	subflow = mptcp_subflow_ctx_by_pos(data, 1);
+	if (subflow)
+		mptcp_subflow_set_stale(storage, subflow->subflow_id);
+
+	for (int i = 0; i < data->subflows && i < MPTCP_SUBFLOWS_MAX; i++) {
+		struct mptcp_subflow_context *subflow;
+
+		subflow = mptcp_subflow_ctx_by_pos(data, i);
+		if (!subflow)
+			break;
+
+		if (mptcp_subflow_is_stale(storage, subflow->subflow_id))
+			continue;
+
+		nr = i;
+	}
+
+	if (nr != -1) {
+		mptcp_subflow_set_scheduled(mptcp_subflow_ctx_by_pos(data, nr), true);
+		return -1;
+	}
+	return 0;
+}
+
+SEC(".struct_ops")
+struct mptcp_sched_ops stale = {
+	.init		= (void *)mptcp_sched_stale_init,
+	.release	= (void *)mptcp_sched_stale_release,
+	.get_subflow	= (void *)bpf_stale_get_subflow,
+	.name		= "bpf_stale",
+};
-- 
2.35.3

[PATCH mptcp-next v3 5/5] selftests/bpf: Add bpf_stale test

[Geliang Tang]

This patch adds the bpf_stale scheduler test: test_stale(). Use sysctl to
set net.mptcp.scheduler to use this sched. Add two veth net devices to
simulate the multiple addresses case. Use 'ip mptcp endpoint' command to
add the new endpoint ADDR_2 to PM netlink. Send data and check bytes_sent
of 'ss' output after it to make sure the data has been only sent on ADDR_1
since ADDR_2 is set as stale.

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 .../testing/selftests/bpf/prog_tests/mptcp.c  | 38 +++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/mptcp.c b/tools/testing/selftests/bpf/prog_tests/mptcp.c
index 3879eaa4fb32..08a6c4c5184a 100644
--- a/tools/testing/selftests/bpf/prog_tests/mptcp.c
+++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c
@@ -15,6 +15,7 @@
 #include "mptcp_bpf_rr.skel.h"
 #include "mptcp_bpf_red.skel.h"
 #include "mptcp_bpf_burst.skel.h"
+#include "mptcp_bpf_stale.skel.h"
 
 #define NS_TEST "mptcp_ns"
 
@@ -651,6 +652,41 @@ static void test_burst(void)
 	mptcp_bpf_burst__destroy(burst_skel);
 }
 
+static void test_stale(void)
+{
+	struct mptcp_bpf_stale *stale_skel;
+	int server_fd, client_fd;
+	struct nstoken *nstoken;
+	struct bpf_link *link;
+
+	stale_skel = mptcp_bpf_stale__open_and_load();
+	if (!ASSERT_OK_PTR(stale_skel, "bpf_stale__open_and_load"))
+		return;
+
+	link = bpf_map__attach_struct_ops(stale_skel->maps.stale);
+	if (!ASSERT_OK_PTR(link, "bpf_map__attach_struct_ops")) {
+		mptcp_bpf_stale__destroy(stale_skel);
+		return;
+	}
+
+	nstoken = sched_init("subflow", "bpf_stale");
+	if (!ASSERT_OK_PTR(nstoken, "sched_init:bpf_stale"))
+		goto fail;
+	server_fd = start_mptcp_server(AF_INET, ADDR_1, PORT_1, 0);
+	client_fd = connect_to_fd(server_fd, 0);
+
+	send_data(server_fd, client_fd, "bpf_stale");
+	ASSERT_OK(has_bytes_sent(ADDR_1), "has_bytes_sent addr_1");
+	ASSERT_GT(has_bytes_sent(ADDR_2), 0, "has_bytes_sent addr_2");
+
+	close(client_fd);
+	close(server_fd);
+fail:
+	cleanup_netns(nstoken);
+	bpf_link__destroy(link);
+	mptcp_bpf_stale__destroy(stale_skel);
+}
+
 void test_mptcp(void)
 {
 	if (test__start_subtest("base"))
@@ -669,4 +705,6 @@ void test_mptcp(void)
 		test_red();
 	if (test__start_subtest("burst"))
 		test_burst();
+	if (test__start_subtest("stale"))
+		test_stale();
 }
-- 
2.35.3

Re: [PATCH mptcp-next v3 0/5] add bpf_stale scheduler

[Mat Martineau]

On Fri, 18 Aug 2023, Geliang Tang wrote:

> v3:
> - init sk_storage in .init, delete it in .release.
> We invoke bpf_sk_storage_get() to get the sk_storage map many times. Only
> the first call is the slow path (see bpf_local_storage_lookup in
> kernel/bpf/bpf_local_storage.c), alloc the map and cache it. The
> subsequent calls are all in fast path, the cache hits. So we should
> first call bpf_sk_storage_get in .init, then call it many times in
> .get_subflow.
> - if no subflow is scheduled, it should return '-1' to indicate an error.

Thanks Geliang, patches 1-3 are ok to squash.

For 4-5, see my reply to patch 4.


- Mat


>
> v2:
> - store subflow ids instead of storing subflow pointers in sk_storage.
>
> v1:
> - This patchset adds the new bpf_stale scheduler. Use sk_storage to save
> the stale map instead of using subflow->stale flag to manage it.
>
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/349
>
> Geliang Tang (5):
>  Squash to "selftests/bpf: Add bpf_bkup scheduler"
>  Squash to "selftests/bpf: Add bpf_rr scheduler"
>  Squash to "selftests/bpf: Add bpf_burst scheduler"
>  selftests/bpf: Add bpf_stale scheduler
>  selftests/bpf: Add bpf_stale test
>
> tools/testing/selftests/bpf/bpf_tcp_helpers.h |   1 +
> .../testing/selftests/bpf/prog_tests/mptcp.c  |  38 +++++
> .../selftests/bpf/progs/mptcp_bpf_bkup.c      |   4 +-
> .../selftests/bpf/progs/mptcp_bpf_burst.c     |   2 +
> .../selftests/bpf/progs/mptcp_bpf_rr.c        |   2 +
> .../selftests/bpf/progs/mptcp_bpf_stale.c     | 152 ++++++++++++++++++
> 6 files changed, 198 insertions(+), 1 deletion(-)
> create mode 100644 tools/testing/selftests/bpf/progs/mptcp_bpf_stale.c
>
> -- 
> 2.35.3
>
>
>

Re: [PATCH mptcp-next v3 4/5] selftests/bpf: Add bpf_stale scheduler

[Mat Martineau]

On Fri, 18 Aug 2023, Geliang Tang wrote:

> This patch implements the setting a subflow as stale/unstale in BPF MPTCP
> scheduler, named bpf_stale. The staled subflow id will be added into a
> map in sk_storage.
>
> Two helper mptcp_subflow_set_stale() and mptcp_subflow_clear_stale() are
> added.
>
> In this test, subflow 1 is set as stale in bpf_stale_data_init(). Each
> subflow is checked whether it's a stale one in bpf_stale_get_subflow() to
> select a unstale subflow to send data.
>
> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
> tools/testing/selftests/bpf/bpf_tcp_helpers.h |   1 +
> .../selftests/bpf/progs/mptcp_bpf_stale.c     | 152 ++++++++++++++++++
> 2 files changed, 153 insertions(+)
> create mode 100644 tools/testing/selftests/bpf/progs/mptcp_bpf_stale.c
>
> diff --git a/tools/testing/selftests/bpf/bpf_tcp_helpers.h b/tools/testing/selftests/bpf/bpf_tcp_helpers.h
> index b687f91f2da8..33246629fa36 100644
> --- a/tools/testing/selftests/bpf/bpf_tcp_helpers.h
> +++ b/tools/testing/selftests/bpf/bpf_tcp_helpers.h
> @@ -239,6 +239,7 @@ struct mptcp_subflow_context {
> 	unsigned long avg_pacing_rate;
> 	__u32	backup : 1;
> 	__u8	stale_count;
> +	__u32	subflow_id;
> 	struct	sock *tcp_sock;	    /* tcp sk backpointer */
> } __attribute__((preserve_access_index));
>
> diff --git a/tools/testing/selftests/bpf/progs/mptcp_bpf_stale.c b/tools/testing/selftests/bpf/progs/mptcp_bpf_stale.c
> new file mode 100644
> index 000000000000..08c857f79221
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/mptcp_bpf_stale.c
> @@ -0,0 +1,152 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2023, SUSE. */
> +
> +#include <linux/bpf.h>
> +#include "bpf_tcp_helpers.h"
> +
> +char _license[] SEC("license") = "GPL";
> +
> +struct mptcp_stale_storage {
> +	__u8 nr;
> +	__u32 ids[MPTCP_SUBFLOWS_MAX];
> +};
> +
> +struct {
> +	__uint(type, BPF_MAP_TYPE_SK_STORAGE);
> +	__uint(map_flags, BPF_F_NO_PREALLOC);
> +	__type(key, int);
> +	__type(value, struct mptcp_stale_storage);
> +} mptcp_stale_map SEC(".maps");
> +
> +static void mptcp_subflow_set_stale(struct mptcp_stale_storage *storage,
> +				    __u32 subflow_id)
> +{
> +	if (!subflow_id)
> +		return;
> +
> +	for (int i = 0; i < storage->nr && i < MPTCP_SUBFLOWS_MAX; i++) {
> +		if (storage->ids[i] == subflow_id)
> +			return;
> +	}
> +
> +	if (storage->nr < MPTCP_SUBFLOWS_MAX - 1)
> +		storage->ids[storage->nr++] = subflow_id;
> +}
> +
> +static void mptcp_subflow_clear_stale(struct mptcp_stale_storage *storage,
> +				      __u32 subflow_id)
> +{
> +	if (!subflow_id)
> +		return;
> +
> +	for (int i = 0; i < storage->nr && i < MPTCP_SUBFLOWS_MAX; i++) {
> +		if (storage->ids[i] == subflow_id) {
> +			for (int j = i; j < MPTCP_SUBFLOWS_MAX - 1; j++) {
> +				if (!storage->ids[j + 1])
> +					break;
> +				storage->ids[j] = storage->ids[j + 1];
> +				storage->ids[j + 1] = 0;
> +			}
> +			storage->nr--;
> +			return;
> +		}
> +	}
> +}
> +
> +static bool mptcp_subflow_is_stale(struct mptcp_stale_storage *storage,
> +				   __u32 subflow_id)
> +{
> +	for (int i = 0; i < storage->nr && i < MPTCP_SUBFLOWS_MAX; i++) {
> +		if (storage->ids[i] == subflow_id)
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
> +static bool mptcp_subflow_is_active(struct mptcp_sched_data *data,
> +				    __u32 subflow_id)
> +{
> +	for (int i = 0; i < data->subflows && i < MPTCP_SUBFLOWS_MAX; i++) {
> +		struct mptcp_subflow_context *subflow;
> +
> +		subflow = mptcp_subflow_ctx_by_pos(data, i);
> +		if (!subflow)
> +			break;
> +		if (subflow->subflow_id == subflow_id)
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
> +SEC("struct_ops/mptcp_sched_stale_init")
> +void BPF_PROG(mptcp_sched_stale_init, struct mptcp_sock *msk)
> +{
> +	struct mptcp_stale_storage *storage;
> +
> +	storage = bpf_sk_storage_get(&mptcp_stale_map, msk, 0,
> +				     BPF_LOCAL_STORAGE_GET_F_CREATE);
> +	if (!storage)
> +		return;
> +
> +	storage->nr = 0;
> +}
> +
> +SEC("struct_ops/mptcp_sched_stale_release")
> +void BPF_PROG(mptcp_sched_stale_release, struct mptcp_sock *msk)
> +{
> +	bpf_sk_storage_delete(&mptcp_stale_map, msk);
> +}
> +
> +int BPF_STRUCT_OPS(bpf_stale_get_subflow, struct mptcp_sock *msk,
> +		   struct mptcp_sched_data *data)
> +{
> +	struct mptcp_subflow_context *subflow;
> +	struct mptcp_stale_storage *storage;
> +	int nr = -1;
> +
> +	storage = bpf_sk_storage_get(&mptcp_stale_map, msk, 0,
> +				     BPF_LOCAL_STORAGE_GET_F_CREATE);
> +	if (!storage)
> +		return -1;
> +
> +	mptcp_sched_data_set_contexts(msk, data);

Should this call be moved to sched.c, right before the calls to 
msk->sched->get_subflow() in the mptcp_sched_get functions?

It looks like every BPF scheduler example has to call this, so it would be 
more efficient to set up the data before calling the BPF functions.

> +
> +	/* Handle invalid subflow ids for subflows that have been closed */
> +	for (int i = 0; i < storage->nr && i < MPTCP_SUBFLOWS_MAX; i++) {
> +		if (!mptcp_subflow_is_active(data, storage->ids[i]))
> +			mptcp_subflow_clear_stale(storage, storage->ids[i]);
> +	}
> +
> +	subflow = mptcp_subflow_ctx_by_pos(data, 1);
> +	if (subflow)
> +		mptcp_subflow_set_stale(storage, subflow->subflow_id);

Since this is always marking the subflow in position 1 as stale...

> +
> +	for (int i = 0; i < data->subflows && i < MPTCP_SUBFLOWS_MAX; i++) {
> +		struct mptcp_subflow_context *subflow;
> +
> +		subflow = mptcp_subflow_ctx_by_pos(data, i);
> +		if (!subflow)
> +			break;
> +
> +		if (mptcp_subflow_is_stale(storage, subflow->subflow_id))

...and then looking up that subflow again, this scheduler is not doing 
anything with actual stale subflows. The other BPF schedulers are already 
exercising everything in the BPF infrastructure that is done here (with 
sk_storage, etc.).

If there's a way to check for real stuck/stale subflows, then I think it 
would be good to have a sample scheduler that does that. But for now, I 
don't think this code adds test coverage or helps demonstrate the BPF 
scheduler.



> +			continue;
> +
> +		nr = i;
> +	}
> +
> +	if (nr != -1) {
> +		mptcp_subflow_set_scheduled(mptcp_subflow_ctx_by_pos(data, nr), true);
> +		return -1;
> +	}
> +	return 0;
> +}
> +
> +SEC(".struct_ops")
> +struct mptcp_sched_ops stale = {
> +	.init		= (void *)mptcp_sched_stale_init,
> +	.release	= (void *)mptcp_sched_stale_release,
> +	.get_subflow	= (void *)bpf_stale_get_subflow,
> +	.name		= "bpf_stale",
> +};
> -- 
> 2.35.3
>
>
>

Re: [PATCH mptcp-next v3 0/5] add bpf_stale scheduler

[Matthieu Baerts]

Hi Geliang, Mat,

On 02/09/2023 01:00, Mat Martineau wrote:
> On Fri, 18 Aug 2023, Geliang Tang wrote:
> 
>> v3:
>> - init sk_storage in .init, delete it in .release.
>> We invoke bpf_sk_storage_get() to get the sk_storage map many times. Only
>> the first call is the slow path (see bpf_local_storage_lookup in
>> kernel/bpf/bpf_local_storage.c), alloc the map and cache it. The
>> subsequent calls are all in fast path, the cache hits. So we should
>> first call bpf_sk_storage_get in .init, then call it many times in
>> .get_subflow.
>> - if no subflow is scheduled, it should return '-1' to indicate an error.
> 
> Thanks Geliang, patches 1-3 are ok to squash.

Thank you for the patches and your patience!

Now in our tree:

New patches for t/upstream:
- b2afd09b16a1: "squashed" patch 1/5 in "selftests/bpf: Add bpf_bkup
scheduler"
- ff989d90d2ea: "squashed" patch 2/5 in "selftests/bpf: Add bpf_rr
scheduler"
- ff0afb0f9763: "squashed" patch 3/5 in "selftests/bpf: Add bpf_burst
scheduler"
- Results: 86fa15fc9c4f..07a689bb4cbf (export)

Tests are now in progress:

https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20230909T152638

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net