[PATCH mptcp-next v7 0/8] BPF packet scheduler

[PATCH mptcp-next v7 0/8] BPF packet scheduler

[Geliang Tang]

v7:
 - add bpf_try_module_get in mptcp_init_sched.
 - add bpf_module_put in mptcp_release_sched.
 - rename bpf_first to mptcp_bpf_first.
 - update commit logs.
 - depends on: "add skc_to_mptcp_sock" v14.
 - export/20220325T055307

v6:
 - still use pernet sched_list, use current->nsproxy->net_ns in BPF
   context instead of using init_net.
 - patch 1:
   - use rcu_read_lock instead of spin_lock in mptcp_sched_find as Florian suggested.
   - drop synchronize_rcu in sched_exit_net as Florian suggested.
   - keep synchronize_rcu in mptcp_unregister_scheduler, otherwise, got
     a workqueue lockup in my test.
   - update Makefile as Mat suggested.
 - patch 2:
   - add mptcp_sched_data_init to register default sched, instead of
     registering it in init_net.
 - patch 5:
   - move mptcp_sched_get_subflow to protocol.h as Mat suggested.
 - patch 6:
   - use current->nsproxy->net_ns instead of init_net.
 - patch 8:
   - add send_data to send more data, instead of send_byte.

v5:
 - patch 1: define per-namespace sched_list (but only used init_net
   namespace. It is difficult to get 'net' in bpf_mptcp_sched_reg and
   bpf_mptcp_sched_unreg. I need some suggestions here.)
 - patch 2: skip mptcp_sched_default in mptcp_unregister_scheduler.
 - patch 8: add tests into mptcp.c, instead of bpf_tcp_ca.c.

v4:
 - set msk->sched to &mptcp_sched_default when the sched argument is NULL
in mptcp_init_sched().

v3:
 - add mptcp_release_sched helper in patch 4.
 - rename mptcp_set_sched to mptcp_init_sched in patch 4.
 - add mptcp_sched_first_release in patch 7.
 - do some cleanups.

v2:
 - split into more small patches.
 - change all parameters of mptcp_sched_ops from sk to msk:
       void (*init)(struct mptcp_sock *msk);
       void (*release)(struct mptcp_sock *msk);
       struct sock *   (*get_subflow)(struct mptcp_sock *msk);
 - add tests in bpf_tcp_ca.c, instead of adding a new one.

v1:
 - Addressed to the commends in the RFC version.

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

Geliang Tang (8):
  mptcp: add struct mptcp_sched_ops
  mptcp: register default scheduler
  mptcp: add a new sysctl scheduler
  mptcp: add sched in mptcp_sock
  mptcp: add get_subflow wrapper
  mptcp: add bpf_mptcp_sched_ops
  selftests: bpf: add mptcp_bpf_first scheduler
  selftests: bpf: add mptcp_bpf_first test

 Documentation/networking/mptcp-sysctl.rst     |   8 +
 include/net/mptcp.h                           |  13 ++
 kernel/bpf/bpf_struct_ops_types.h             |   4 +
 net/mptcp/Makefile                            |   2 +-
 net/mptcp/bpf.c                               | 103 ++++++++++++
 net/mptcp/ctrl.c                              |  14 ++
 net/mptcp/protocol.c                          |  14 +-
 net/mptcp/protocol.h                          |  20 +++
 net/mptcp/sched.c                             | 154 ++++++++++++++++++
 tools/testing/selftests/bpf/bpf_tcp_helpers.h |  12 ++
 .../testing/selftests/bpf/prog_tests/mptcp.c  | 114 +++++++++++++
 .../selftests/bpf/progs/mptcp_bpf_first.c     |  30 ++++
 12 files changed, 483 insertions(+), 5 deletions(-)
 create mode 100644 net/mptcp/sched.c
 create mode 100644 tools/testing/selftests/bpf/progs/mptcp_bpf_first.c

-- 
2.34.1

[PATCH mptcp-next v7 1/8] mptcp: add struct mptcp_sched_ops

[Geliang Tang]

This patch defines struct mptcp_sched_ops, which has three struct members,
name, owner and list, and three function pointers, init, release and
get_subflow.

Add the scheduler registering, unregistering and finding functions to add
or delete or find a packet scheduler on the pernet sched_list.

Suggested-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 include/net/mptcp.h  |  13 +++++
 net/mptcp/Makefile   |   2 +-
 net/mptcp/protocol.c |   1 +
 net/mptcp/protocol.h |   7 +++
 net/mptcp/sched.c    | 113 +++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 135 insertions(+), 1 deletion(-)
 create mode 100644 net/mptcp/sched.c

diff --git a/include/net/mptcp.h b/include/net/mptcp.h
index 8b1afd6f5cc4..e3a0baa8dbd7 100644
--- a/include/net/mptcp.h
+++ b/include/net/mptcp.h
@@ -95,6 +95,19 @@ struct mptcp_out_options {
 #endif
 };
 
+#define MPTCP_SCHED_NAME_MAX 16
+
+struct mptcp_sched_ops {
+	struct sock *	(*get_subflow)(struct mptcp_sock *msk);
+
+	char			name[MPTCP_SCHED_NAME_MAX];
+	struct module		*owner;
+	struct list_head	list;
+
+	void (*init)(struct mptcp_sock *msk);
+	void (*release)(struct mptcp_sock *msk);
+} ____cacheline_aligned_in_smp;
+
 #ifdef CONFIG_MPTCP
 extern struct request_sock_ops mptcp_subflow_request_sock_ops;
 
diff --git a/net/mptcp/Makefile b/net/mptcp/Makefile
index 168c55d1c917..a37330760b0c 100644
--- a/net/mptcp/Makefile
+++ b/net/mptcp/Makefile
@@ -2,7 +2,7 @@
 obj-$(CONFIG_MPTCP) += mptcp.o
 
 mptcp-y := protocol.o subflow.o options.o token.o crypto.o ctrl.o pm.o diag.o \
-	   mib.o pm_netlink.o sockopt.o
+	   mib.o pm_netlink.o sockopt.o sched.o
 
 obj-$(CONFIG_SYN_COOKIES) += syncookies.o
 obj-$(CONFIG_INET_MPTCP_DIAG) += mptcp_diag.o
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index d3887f628b54..b1d7c8b0c112 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -3807,6 +3807,7 @@ void __init mptcp_proto_init(void)
 
 	mptcp_subflow_init();
 	mptcp_pm_init();
+	mptcp_sched_init();
 	mptcp_token_init();
 
 	if (proto_register(&mptcp_prot, MPTCP_USE_SLAB) != 0)
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index fd82fd113113..3258b740c8ee 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -608,6 +608,13 @@ int mptcp_subflow_create_socket(struct sock *sk, struct socket **new_sock);
 void mptcp_info2sockaddr(const struct mptcp_addr_info *info,
 			 struct sockaddr_storage *addr,
 			 unsigned short family);
+struct mptcp_sched_ops *mptcp_sched_find(const struct net *net,
+					 const char *name);
+int mptcp_register_scheduler(const struct net *net,
+			     struct mptcp_sched_ops *sched);
+void mptcp_unregister_scheduler(const struct net *net,
+				struct mptcp_sched_ops *sched);
+void mptcp_sched_init(void);
 
 static inline bool __mptcp_subflow_active(struct mptcp_subflow_context *subflow)
 {
diff --git a/net/mptcp/sched.c b/net/mptcp/sched.c
new file mode 100644
index 000000000000..ae1956b6de92
--- /dev/null
+++ b/net/mptcp/sched.c
@@ -0,0 +1,113 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Multipath TCP
+ *
+ * Copyright (c) 2022, SUSE.
+ */
+
+#define pr_fmt(fmt) "MPTCP: " fmt
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/list.h>
+#include <linux/rculist.h>
+#include <linux/spinlock.h>
+#include <net/tcp.h>
+#include <net/netns/generic.h>
+#include "protocol.h"
+
+static int sched_pernet_id;
+
+struct sched_pernet {
+	/* protects pernet updates */
+	spinlock_t		lock;
+	struct list_head	sched_list;
+};
+
+static struct sched_pernet *sched_get_pernet(const struct net *net)
+{
+	return net_generic(net, sched_pernet_id);
+}
+
+struct mptcp_sched_ops *mptcp_sched_find(const struct net *net,
+					 const char *name)
+{
+	struct sched_pernet *pernet = sched_get_pernet(net);
+	struct mptcp_sched_ops *sched, *ret = NULL;
+
+	rcu_read_lock();
+	list_for_each_entry_rcu(sched, &pernet->sched_list, list) {
+		if (!strcmp(sched->name, name)) {
+			ret = sched;
+			break;
+		}
+	}
+	rcu_read_unlock();
+
+	return ret;
+}
+
+int mptcp_register_scheduler(const struct net *net,
+			     struct mptcp_sched_ops *sched)
+{
+	struct sched_pernet *pernet = sched_get_pernet(net);
+
+	if (!sched->get_subflow)
+		return -EINVAL;
+
+	if (mptcp_sched_find(net, sched->name))
+		return -EEXIST;
+
+	spin_lock(&pernet->lock);
+	list_add_tail_rcu(&sched->list, &pernet->sched_list);
+	spin_unlock(&pernet->lock);
+
+	pr_debug("%s registered", sched->name);
+	return 0;
+}
+
+void mptcp_unregister_scheduler(const struct net *net,
+				struct mptcp_sched_ops *sched)
+{
+	struct sched_pernet *pernet = sched_get_pernet(net);
+
+	spin_lock(&pernet->lock);
+	list_del_rcu(&sched->list);
+	spin_unlock(&pernet->lock);
+
+	/* avoid workqueue lockup */
+	synchronize_rcu();
+}
+
+static int __net_init sched_init_net(struct net *net)
+{
+	struct sched_pernet *pernet = sched_get_pernet(net);
+
+	INIT_LIST_HEAD_RCU(&pernet->sched_list);
+	spin_lock_init(&pernet->lock);
+
+	return 0;
+}
+
+static void __net_exit sched_exit_net(struct net *net)
+{
+	struct sched_pernet *pernet = sched_get_pernet(net);
+	struct mptcp_sched_ops *sched;
+
+	spin_lock(&pernet->lock);
+	list_for_each_entry_rcu(sched, &pernet->sched_list, list)
+		list_del_rcu(&sched->list);
+	spin_unlock(&pernet->lock);
+}
+
+static struct pernet_operations mptcp_sched_pernet_ops = {
+	.init = sched_init_net,
+	.exit = sched_exit_net,
+	.id = &sched_pernet_id,
+	.size = sizeof(struct sched_pernet),
+};
+
+void mptcp_sched_init(void)
+{
+	if (register_pernet_subsys(&mptcp_sched_pernet_ops) < 0)
+		panic("Failed to register MPTCP sched pernet subsystem.\n");
+}
-- 
2.34.1

[PATCH mptcp-next v7 2/8] mptcp: register default scheduler

[Geliang Tang]

This patch defines the default packet scheduler mptcp_sched_default,
registers it in mptcp_sched_data_init(). Skip deleting this default
scheduler in mptcp_unregister_scheduler.

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 net/mptcp/protocol.c |  3 ++-
 net/mptcp/protocol.h |  2 ++
 net/mptcp/sched.c    | 14 ++++++++++++++
 3 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index b1d7c8b0c112..2c684034fe7a 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -1424,7 +1424,7 @@ bool mptcp_subflow_active(struct mptcp_subflow_context *subflow)
  * returns the subflow that will transmit the next DSS
  * additionally updates the rtx timeout
  */
-static struct sock *mptcp_subflow_get_send(struct mptcp_sock *msk)
+struct sock *mptcp_subflow_get_send(struct mptcp_sock *msk)
 {
 	struct subflow_send_info send_info[SSK_MODE_MAX];
 	struct mptcp_subflow_context *subflow;
@@ -2614,6 +2614,7 @@ static int __mptcp_init_sock(struct sock *sk)
 	msk->recovery = false;
 
 	mptcp_pm_data_init(msk);
+	mptcp_sched_data_init(sk);
 
 	/* re-use the csk retrans timer for MPTCP-level retrans */
 	timer_setup(&msk->sk.icsk_retransmit_timer, mptcp_retransmit_timer, 0);
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 3258b740c8ee..4c0a3c39e897 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -615,6 +615,8 @@ int mptcp_register_scheduler(const struct net *net,
 void mptcp_unregister_scheduler(const struct net *net,
 				struct mptcp_sched_ops *sched);
 void mptcp_sched_init(void);
+struct sock *mptcp_subflow_get_send(struct mptcp_sock *msk);
+void mptcp_sched_data_init(struct sock *sk);
 
 static inline bool __mptcp_subflow_active(struct mptcp_subflow_context *subflow)
 {
diff --git a/net/mptcp/sched.c b/net/mptcp/sched.c
index ae1956b6de92..1fb3dd24d6ff 100644
--- a/net/mptcp/sched.c
+++ b/net/mptcp/sched.c
@@ -23,6 +23,12 @@ struct sched_pernet {
 	struct list_head	sched_list;
 };
 
+static struct mptcp_sched_ops mptcp_sched_default = {
+	.get_subflow    = mptcp_subflow_get_send,
+	.name           = "default",
+	.owner          = THIS_MODULE,
+};
+
 static struct sched_pernet *sched_get_pernet(const struct net *net)
 {
 	return net_generic(net, sched_pernet_id);
@@ -70,6 +76,9 @@ void mptcp_unregister_scheduler(const struct net *net,
 {
 	struct sched_pernet *pernet = sched_get_pernet(net);
 
+	if (sched == &mptcp_sched_default)
+		return;
+
 	spin_lock(&pernet->lock);
 	list_del_rcu(&sched->list);
 	spin_unlock(&pernet->lock);
@@ -111,3 +120,8 @@ void mptcp_sched_init(void)
 	if (register_pernet_subsys(&mptcp_sched_pernet_ops) < 0)
 		panic("Failed to register MPTCP sched pernet subsystem.\n");
 }
+
+void mptcp_sched_data_init(struct sock *sk)
+{
+	mptcp_register_scheduler(sock_net(sk), &mptcp_sched_default);
+}
-- 
2.34.1

[PATCH mptcp-next v7 3/8] mptcp: add a new sysctl scheduler

[Geliang Tang]

This patch adds a new sysctl, named scheduler, to support for selection
of different schedulers. Export mptcp_get_scheduler helper to get this
sysctl.

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 Documentation/networking/mptcp-sysctl.rst |  8 ++++++++
 net/mptcp/ctrl.c                          | 14 ++++++++++++++
 net/mptcp/protocol.h                      |  1 +
 3 files changed, 23 insertions(+)

diff --git a/Documentation/networking/mptcp-sysctl.rst b/Documentation/networking/mptcp-sysctl.rst
index e263dfcc4b40..d9e69fdc7ea3 100644
--- a/Documentation/networking/mptcp-sysctl.rst
+++ b/Documentation/networking/mptcp-sysctl.rst
@@ -75,3 +75,11 @@ stale_loss_cnt - INTEGER
 	This is a per-namespace sysctl.
 
 	Default: 4
+
+scheduler - STRING
+	Select the scheduler of your choice.
+
+	Support for selection of different schedulers. This is a per-namespace
+	sysctl.
+
+	Default: "default"
diff --git a/net/mptcp/ctrl.c b/net/mptcp/ctrl.c
index ae20b7d92e28..c46c22a84d23 100644
--- a/net/mptcp/ctrl.c
+++ b/net/mptcp/ctrl.c
@@ -32,6 +32,7 @@ struct mptcp_pernet {
 	u8 checksum_enabled;
 	u8 allow_join_initial_addr_port;
 	u8 pm_type;
+	char scheduler[MPTCP_SCHED_NAME_MAX];
 };
 
 static struct mptcp_pernet *mptcp_get_pernet(const struct net *net)
@@ -69,6 +70,11 @@ int mptcp_get_pm_type(const struct net *net)
 	return mptcp_get_pernet(net)->pm_type;
 }
 
+const char *mptcp_get_scheduler(const struct net *net)
+{
+	return mptcp_get_pernet(net)->scheduler;
+}
+
 static void mptcp_pernet_set_defaults(struct mptcp_pernet *pernet)
 {
 	pernet->mptcp_enabled = 1;
@@ -77,6 +83,7 @@ static void mptcp_pernet_set_defaults(struct mptcp_pernet *pernet)
 	pernet->allow_join_initial_addr_port = 1;
 	pernet->stale_loss_cnt = 4;
 	pernet->pm_type = MPTCP_PM_TYPE_KERNEL;
+	strcpy(pernet->scheduler, "default");
 }
 
 #ifdef CONFIG_SYSCTL
@@ -128,6 +135,12 @@ static struct ctl_table mptcp_sysctl_table[] = {
 		.extra1       = SYSCTL_ZERO,
 		.extra2       = &mptcp_pm_type_max
 	},
+	{
+		.procname = "scheduler",
+		.maxlen	= MPTCP_SCHED_NAME_MAX,
+		.mode = 0644,
+		.proc_handler = proc_dostring,
+	},
 	{}
 };
 
@@ -149,6 +162,7 @@ static int mptcp_pernet_new_table(struct net *net, struct mptcp_pernet *pernet)
 	table[3].data = &pernet->allow_join_initial_addr_port;
 	table[4].data = &pernet->stale_loss_cnt;
 	table[5].data = &pernet->pm_type;
+	table[6].data = &pernet->scheduler;
 
 	hdr = register_net_sysctl(net, MPTCP_SYSCTL_PATH, table);
 	if (!hdr)
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 4c0a3c39e897..dd4a2afdb38b 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -586,6 +586,7 @@ int mptcp_is_checksum_enabled(const struct net *net);
 int mptcp_allow_join_id0(const struct net *net);
 unsigned int mptcp_stale_loss_cnt(const struct net *net);
 int mptcp_get_pm_type(const struct net *net);
+const char *mptcp_get_scheduler(const struct net *net);
 void mptcp_subflow_fully_established(struct mptcp_subflow_context *subflow,
 				     struct mptcp_options_received *mp_opt);
 bool __mptcp_retransmit_pending_data(struct sock *sk);
-- 
2.34.1

[PATCH mptcp-next v7 4/8] mptcp: add sched in mptcp_sock

[Geliang Tang]

This patch adds a new struct member sched in struct mptcp_sock.
And two helpers mptcp_init_sched() and mptcp_release_sched() to
init and release it.

Init it with the sysctl scheduler in mptcp_init_sock(), copy the
scheduler from the parent in mptcp_sk_clone(), and release it in
__mptcp_destroy_sock().

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 net/mptcp/protocol.c |  4 ++++
 net/mptcp/protocol.h |  4 ++++
 net/mptcp/sched.c    | 27 +++++++++++++++++++++++++++
 3 files changed, 35 insertions(+)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 2c684034fe7a..82b3846147a6 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -2662,6 +2662,8 @@ static int mptcp_init_sock(struct sock *sk)
 	 * propagate the correct value
 	 */
 	mptcp_ca_reset(sk);
+	mptcp_init_sched(mptcp_sk(sk),
+			 mptcp_sched_find(net, mptcp_get_scheduler(net)));
 
 	sk_sockets_allocated_inc(sk);
 	sk->sk_rcvbuf = sock_net(sk)->ipv4.sysctl_tcp_rmem[1];
@@ -2817,6 +2819,7 @@ static void __mptcp_destroy_sock(struct sock *sk)
 	sk_stop_timer(sk, &sk->sk_timer);
 	mptcp_data_unlock(sk);
 	msk->pm.status = 0;
+	mptcp_release_sched(msk);
 
 	/* clears msk->subflow, allowing the following loop to close
 	 * even the initial subflow
@@ -2994,6 +2997,7 @@ struct sock *mptcp_sk_clone(const struct sock *sk,
 	msk->snd_una = msk->write_seq;
 	msk->wnd_end = msk->snd_nxt + req->rsk_rcv_wnd;
 	msk->setsockopt_seq = mptcp_sk(sk)->setsockopt_seq;
+	mptcp_init_sched(msk, mptcp_sk(sk)->sched);
 
 	if (mp_opt->suboptions & OPTIONS_MPTCP_MPC) {
 		msk->can_ack = true;
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index dd4a2afdb38b..81c35af7ade5 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -288,6 +288,7 @@ struct mptcp_sock {
 	struct socket	*subflow; /* outgoing connect/listener/!mp_capable */
 	struct sock	*first;
 	struct mptcp_pm_data	pm;
+	struct mptcp_sched_ops	*sched;
 	struct {
 		u32	space;	/* bytes copied in last measurement window */
 		u32	copied; /* bytes copied in this measurement window */
@@ -618,6 +619,9 @@ void mptcp_unregister_scheduler(const struct net *net,
 void mptcp_sched_init(void);
 struct sock *mptcp_subflow_get_send(struct mptcp_sock *msk);
 void mptcp_sched_data_init(struct sock *sk);
+int mptcp_init_sched(struct mptcp_sock *msk,
+		     struct mptcp_sched_ops *sched);
+void mptcp_release_sched(struct mptcp_sock *msk);
 
 static inline bool __mptcp_subflow_active(struct mptcp_subflow_context *subflow)
 {
diff --git a/net/mptcp/sched.c b/net/mptcp/sched.c
index 1fb3dd24d6ff..9b3a3157111d 100644
--- a/net/mptcp/sched.c
+++ b/net/mptcp/sched.c
@@ -125,3 +125,30 @@ void mptcp_sched_data_init(struct sock *sk)
 {
 	mptcp_register_scheduler(sock_net(sk), &mptcp_sched_default);
 }
+
+int mptcp_init_sched(struct mptcp_sock *msk,
+		     struct mptcp_sched_ops *sched)
+{
+	if (!sched)
+		msk->sched = &mptcp_sched_default;
+	else
+		msk->sched = sched;
+
+	if (!bpf_try_module_get(msk->sched, msk->sched->owner))
+		return -EBUSY;
+
+	if (msk->sched->init)
+		msk->sched->init(msk);
+
+	pr_debug("sched=%s", msk->sched->name);
+
+	return 0;
+}
+
+void mptcp_release_sched(struct mptcp_sock *msk)
+{
+	if (msk->sched && msk->sched->release)
+		msk->sched->release(msk);
+	bpf_module_put(msk->sched, msk->sched->owner);
+	msk->sched = NULL;
+}
-- 
2.34.1

[PATCH mptcp-next v7 5/8] mptcp: add get_subflow wrapper

[Geliang Tang]

This patch defines a new wrapper mptcp_sched_get_subflow(), invoke
get_subflow of msk->sched in it. Use the wrapper instead of using
mptcp_subflow_get_send() directly.

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 net/mptcp/protocol.c | 6 +++---
 net/mptcp/protocol.h | 6 ++++++
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 82b3846147a6..7bc7768f63e8 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -1567,7 +1567,7 @@ void __mptcp_push_pending(struct sock *sk, unsigned int flags)
 			int ret = 0;
 
 			prev_ssk = ssk;
-			ssk = mptcp_subflow_get_send(msk);
+			ssk = mptcp_sched_get_subflow(msk);
 
 			/* First check. If the ssk has changed since
 			 * the last round, release prev_ssk
@@ -1636,7 +1636,7 @@ static void __mptcp_subflow_push_pending(struct sock *sk, struct sock *ssk)
 			 * check for a different subflow usage only after
 			 * spooling the first chunk of data
 			 */
-			xmit_ssk = first ? ssk : mptcp_subflow_get_send(mptcp_sk(sk));
+			xmit_ssk = first ? ssk : mptcp_sched_get_subflow(mptcp_sk(sk));
 			if (!xmit_ssk)
 				goto out;
 			if (xmit_ssk != ssk) {
@@ -3135,7 +3135,7 @@ void __mptcp_check_push(struct sock *sk, struct sock *ssk)
 		return;
 
 	if (!sock_owned_by_user(sk)) {
-		struct sock *xmit_ssk = mptcp_subflow_get_send(mptcp_sk(sk));
+		struct sock *xmit_ssk = mptcp_sched_get_subflow(mptcp_sk(sk));
 
 		if (xmit_ssk == ssk)
 			__mptcp_subflow_push_pending(sk, ssk);
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 81c35af7ade5..31845b43242f 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -623,6 +623,12 @@ int mptcp_init_sched(struct mptcp_sock *msk,
 		     struct mptcp_sched_ops *sched);
 void mptcp_release_sched(struct mptcp_sock *msk);
 
+static inline struct sock *mptcp_sched_get_subflow(struct mptcp_sock *msk)
+{
+	return INDIRECT_CALL_INET_1(msk->sched->get_subflow,
+				    mptcp_subflow_get_send, msk);
+}
+
 static inline bool __mptcp_subflow_active(struct mptcp_subflow_context *subflow)
 {
 	struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
-- 
2.34.1

[PATCH mptcp-next v7 6/8] mptcp: add bpf_mptcp_sched_ops

[Geliang Tang]

This patch implements a new struct bpf_struct_ops, bpf_mptcp_sched_ops.
Register and unregister the bpf scheduler in .reg and .unreg.

This implementation is similar to BPF TCP CC. And some code in this patch
is from bpf_tcp_ca.c

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 kernel/bpf/bpf_struct_ops_types.h |   4 ++
 net/mptcp/bpf.c                   | 103 ++++++++++++++++++++++++++++++
 2 files changed, 107 insertions(+)

diff --git a/kernel/bpf/bpf_struct_ops_types.h b/kernel/bpf/bpf_struct_ops_types.h
index 5678a9ddf817..5a6b0c0d8d3d 100644
--- a/kernel/bpf/bpf_struct_ops_types.h
+++ b/kernel/bpf/bpf_struct_ops_types.h
@@ -8,5 +8,9 @@ BPF_STRUCT_OPS_TYPE(bpf_dummy_ops)
 #ifdef CONFIG_INET
 #include <net/tcp.h>
 BPF_STRUCT_OPS_TYPE(tcp_congestion_ops)
+#ifdef CONFIG_MPTCP
+#include <net/mptcp.h>
+BPF_STRUCT_OPS_TYPE(mptcp_sched_ops)
+#endif
 #endif
 #endif
diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c
index 535602ba2582..be809438c5d2 100644
--- a/net/mptcp/bpf.c
+++ b/net/mptcp/bpf.c
@@ -10,8 +10,111 @@
 #define pr_fmt(fmt) "MPTCP: " fmt
 
 #include <linux/bpf.h>
+#include <linux/bpf_verifier.h>
+#include <linux/btf.h>
+#include <linux/btf_ids.h>
 #include "protocol.h"
 
+extern struct bpf_struct_ops bpf_mptcp_sched_ops;
+extern struct btf *btf_vmlinux;
+
+static u32 optional_ops[] = {
+	offsetof(struct mptcp_sched_ops, init),
+	offsetof(struct mptcp_sched_ops, release),
+	offsetof(struct mptcp_sched_ops, get_subflow),
+};
+
+static const struct bpf_func_proto *
+bpf_mptcp_sched_get_func_proto(enum bpf_func_id func_id,
+			       const struct bpf_prog *prog)
+{
+	return bpf_base_func_proto(func_id);
+}
+
+static const struct bpf_verifier_ops bpf_mptcp_sched_verifier_ops = {
+	.get_func_proto		= bpf_mptcp_sched_get_func_proto,
+	.is_valid_access	= btf_ctx_access,
+	.btf_struct_access	= btf_struct_access,
+};
+
+static int bpf_mptcp_sched_reg(void *kdata)
+{
+	return mptcp_register_scheduler(current->nsproxy->net_ns, kdata);
+}
+
+static void bpf_mptcp_sched_unreg(void *kdata)
+{
+	mptcp_unregister_scheduler(current->nsproxy->net_ns, kdata);
+}
+
+static int bpf_mptcp_sched_check_member(const struct btf_type *t,
+					const struct btf_member *member)
+{
+	return 0;
+}
+
+static bool is_optional(u32 member_offset)
+{
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(optional_ops); i++) {
+		if (member_offset == optional_ops[i])
+			return true;
+	}
+
+	return false;
+}
+
+static int bpf_mptcp_sched_init_member(const struct btf_type *t,
+				       const struct btf_member *member,
+				       void *kdata, const void *udata)
+{
+	const struct mptcp_sched_ops *usched;
+	struct mptcp_sched_ops *sched;
+	int prog_fd;
+	u32 moff;
+
+	usched = (const struct mptcp_sched_ops *)udata;
+	sched = (struct mptcp_sched_ops *)kdata;
+
+	moff = __btf_member_bit_offset(t, member) / 8;
+	switch (moff) {
+	case offsetof(struct mptcp_sched_ops, name):
+		if (bpf_obj_name_cpy(sched->name, usched->name,
+				     sizeof(sched->name)) <= 0)
+			return -EINVAL;
+		if (mptcp_sched_find(current->nsproxy->net_ns,
+				     usched->name))
+			return -EEXIST;
+		return 1;
+	}
+
+	if (!btf_type_resolve_func_ptr(btf_vmlinux, member->type, NULL))
+		return 0;
+
+	/* Ensure bpf_prog is provided for compulsory func ptr */
+	prog_fd = (int)(*(unsigned long *)(udata + moff));
+	if (!prog_fd && !is_optional(moff))
+		return -EINVAL;
+
+	return 0;
+}
+
+static int bpf_mptcp_sched_init(struct btf *btf)
+{
+	return 0;
+}
+
+struct bpf_struct_ops bpf_mptcp_sched_ops = {
+	.verifier_ops	= &bpf_mptcp_sched_verifier_ops,
+	.reg		= bpf_mptcp_sched_reg,
+	.unreg		= bpf_mptcp_sched_unreg,
+	.check_member	= bpf_mptcp_sched_check_member,
+	.init_member	= bpf_mptcp_sched_init_member,
+	.init		= bpf_mptcp_sched_init,
+	.name		= "mptcp_sched_ops",
+};
+
 struct mptcp_sock *bpf_mptcp_sock_from_subflow(struct sock *sk)
 {
 	if (sk && sk_fullsock(sk) && sk->sk_protocol == IPPROTO_TCP && sk_is_mptcp(sk))
-- 
2.34.1

[PATCH mptcp-next v7 7/8] selftests: bpf: add mptcp_bpf_first scheduler

[Geliang Tang]

This patch implements the simplest MPTCP scheduler, named mptcp_bpf_first,
which always picks the first subflow to send data. It's a sample of MPTCP
BPF scheduler implementation.

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

diff --git a/tools/testing/selftests/bpf/bpf_tcp_helpers.h b/tools/testing/selftests/bpf/bpf_tcp_helpers.h
index f92357597e63..5bb21b24231f 100644
--- a/tools/testing/selftests/bpf/bpf_tcp_helpers.h
+++ b/tools/testing/selftests/bpf/bpf_tcp_helpers.h
@@ -197,6 +197,18 @@ struct tcp_congestion_ops {
 	void *owner;
 };
 
+#define MPTCP_SCHED_NAME_MAX 16
+
+struct mptcp_sched_ops {
+	char name[MPTCP_SCHED_NAME_MAX];
+
+	void (*init)(struct mptcp_sock *msk);
+	void (*release)(struct mptcp_sock *msk);
+
+	struct sock *	(*get_subflow)(struct mptcp_sock *msk);
+	void *owner;
+};
+
 #define min(a, b) ((a) < (b) ? (a) : (b))
 #define max(a, b) ((a) > (b) ? (a) : (b))
 #define min_not_zero(x, y) ({			\
diff --git a/tools/testing/selftests/bpf/progs/mptcp_bpf_first.c b/tools/testing/selftests/bpf/progs/mptcp_bpf_first.c
new file mode 100644
index 000000000000..3ae5c1eab5f5
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/mptcp_bpf_first.c
@@ -0,0 +1,30 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* Copyright (c) 2022, SUSE. */
+
+#include <linux/bpf.h>
+#include "bpf_tcp_helpers.h"
+
+char _license[] SEC("license") = "GPL";
+
+SEC("struct_ops/mptcp_sched_first_init")
+void BPF_PROG(mptcp_sched_first_init, struct mptcp_sock *msk)
+{
+}
+
+SEC("struct_ops/mptcp_sched_first_release")
+void BPF_PROG(mptcp_sched_first_release, struct mptcp_sock *msk)
+{
+}
+
+struct sock *BPF_STRUCT_OPS(bpf_first_get_subflow, struct mptcp_sock *msk)
+{
+	return msk->first;
+}
+
+SEC(".struct_ops")
+struct mptcp_sched_ops first = {
+	.init		= (void *)mptcp_sched_first_init,
+	.release	= (void *)mptcp_sched_first_release,
+	.get_subflow	= (void *)bpf_first_get_subflow,
+	.name		= "bpf_first",
+};
-- 
2.34.1

[PATCH mptcp-next v7 8/8] selftests: bpf: add mptcp_bpf_first test

[Geliang Tang]

This patch expends the MPTCP test base to support MPTCP packet
scheduler tests. Add the mptcp_bpf_first scheduler test in it.
Use sysctl to set net.mptcp.scheduler to use this sched.

Some code in send_data() is from bpf_tcp_ca.c.

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

diff --git a/tools/testing/selftests/bpf/prog_tests/mptcp.c b/tools/testing/selftests/bpf/prog_tests/mptcp.c
index 9b73ef62ee74..e5b6f2f4a52d 100644
--- a/tools/testing/selftests/bpf/prog_tests/mptcp.c
+++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c
@@ -4,6 +4,9 @@
 #include <test_progs.h>
 #include "cgroup_helpers.h"
 #include "network_helpers.h"
+#include "bpf_first.skel.h"
+
+#define min(a, b) ((a) < (b) ? (a) : (b))
 
 #ifndef TCP_CA_NAME_MAX
 #define TCP_CA_NAME_MAX	16
@@ -19,6 +22,8 @@ struct mptcp_storage {
 };
 
 static char monitor_log_path[64];
+static const unsigned int total_bytes = 10 * 1024 * 1024;
+static int stop, duration;
 
 static int verify_tsk(int map_fd, int client_fd)
 {
@@ -251,8 +256,117 @@ void test_base(void)
 	close(cgroup_fd);
 }
 
+static void *server(void *arg)
+{
+	int lfd = (int)(long)arg, err = 0, fd;
+	ssize_t nr_sent = 0, bytes = 0;
+	char batch[1500];
+
+	fd = accept(lfd, NULL, NULL);
+	while (fd == -1) {
+		if (errno == EINTR)
+			continue;
+		err = -errno;
+		goto done;
+	}
+
+	if (settimeo(fd, 0)) {
+		err = -errno;
+		goto done;
+	}
+
+	while (bytes < total_bytes && !READ_ONCE(stop)) {
+		nr_sent = send(fd, &batch,
+			       min(total_bytes - bytes, sizeof(batch)), 0);
+		if (nr_sent == -1 && errno == EINTR)
+			continue;
+		if (nr_sent == -1) {
+			err = -errno;
+			break;
+		}
+		bytes += nr_sent;
+	}
+
+	CHECK(bytes != total_bytes, "send", "%zd != %u nr_sent:%zd errno:%d\n",
+	      bytes, total_bytes, nr_sent, errno);
+
+done:
+	if (fd >= 0)
+		close(fd);
+	if (err) {
+		WRITE_ONCE(stop, 1);
+		return ERR_PTR(err);
+	}
+	return NULL;
+}
+
+static void send_data(int lfd, int fd)
+{
+	ssize_t nr_recv = 0, bytes = 0;
+	pthread_t srv_thread;
+	void *thread_ret;
+	char batch[1500];
+	int err;
+
+	WRITE_ONCE(stop, 0);
+
+	err = pthread_create(&srv_thread, NULL, server, (void *)(long)lfd);
+	if (CHECK(err != 0, "pthread_create", "err:%d errno:%d\n", err, errno))
+		return;
+
+	/* recv total_bytes */
+	while (bytes < total_bytes && !READ_ONCE(stop)) {
+		nr_recv = recv(fd, &batch,
+			       min(total_bytes - bytes, sizeof(batch)), 0);
+		if (nr_recv == -1 && errno == EINTR)
+			continue;
+		if (nr_recv == -1)
+			break;
+		bytes += nr_recv;
+	}
+
+	CHECK(bytes != total_bytes, "recv", "%zd != %u nr_recv:%zd errno:%d\n",
+	      bytes, total_bytes, nr_recv, errno);
+
+	WRITE_ONCE(stop, 1);
+
+	pthread_join(srv_thread, &thread_ret);
+	CHECK(IS_ERR(thread_ret), "pthread_join", "thread_ret:%ld",
+	      PTR_ERR(thread_ret));
+}
+
+static void test_first(void)
+{
+	struct bpf_first *first_skel;
+	int server_fd, client_fd;
+	struct bpf_link *link;
+
+	first_skel = bpf_first__open_and_load();
+	if (CHECK(!first_skel, "bpf_first__open_and_load", "failed\n"))
+		return;
+
+	link = bpf_map__attach_struct_ops(first_skel->maps.first);
+	if (!ASSERT_OK_PTR(link, "bpf_map__attach_struct_ops")) {
+		bpf_first__destroy(first_skel);
+		return;
+	}
+
+	system("sysctl -q net.mptcp.scheduler=mptcp_bpf_first");
+	server_fd = start_mptcp_server(AF_INET, NULL, 0, 0);
+	client_fd = connect_to_mptcp_fd(server_fd, 0);
+
+	send_data(server_fd, client_fd);
+
+	close(client_fd);
+	close(server_fd);
+	bpf_link__destroy(link);
+	bpf_first__destroy(first_skel);
+}
+
 void test_mptcp(void)
 {
 	if (test__start_subtest("base"))
 		test_base();
+	if (test__start_subtest("first"))
+		test_first();
 }
-- 
2.34.1

Re: [PATCH mptcp-next v7 1/8] mptcp: add struct mptcp_sched_ops

[Florian Westphal]

Geliang Tang <geliang.tang@suse.com> wrote:
> This patch defines struct mptcp_sched_ops, which has three struct members,
> name, owner and list, and three function pointers, init, release and
> get_subflow.
> 
> Add the scheduler registering, unregistering and finding functions to add
> or delete or find a packet scheduler on the pernet sched_list.
> 
> Suggested-by: Florian Westphal <fw@strlen.de>

Huh?  I made some comments on earlier patch but I did not suggest this.
In fact, I don't like this at all but I guess I've been vetoed wrt.
pernet design.

> +struct mptcp_sched_ops *mptcp_sched_find(const struct net *net,
> +					 const char *name)
> +{
> +	struct sched_pernet *pernet = sched_get_pernet(net);
> +	struct mptcp_sched_ops *sched, *ret = NULL;
> +
> +	rcu_read_lock();
> +	list_for_each_entry_rcu(sched, &pernet->sched_list, list) {
> +		if (!strcmp(sched->name, name)) {
> +			ret = sched;
> +			break;
> +		}
> +	}
> +	rcu_read_unlock();
> +	return ret;

This is fishy.  Either caller must already hold rcu read lock,
or mptcp_sched_ops can be free'd before function returns.

I suspect its best to force callers to do the rcu_read_lock and
document that this function is called with rcu read lock held.

> +int mptcp_register_scheduler(const struct net *net,
> +			     struct mptcp_sched_ops *sched)
> +{
> +	struct sched_pernet *pernet = sched_get_pernet(net);
> +
> +	if (!sched->get_subflow)
> +		return -EINVAL;
> +
> +	if (mptcp_sched_find(net, sched->name))
> +		return -EEXIST;
> +
> +	spin_lock(&pernet->lock);
> +	list_add_tail_rcu(&sched->list, &pernet->sched_list);
> +	spin_unlock(&pernet->lock);

This is racy.  Lock, test, add, unlock, not test, lock, ...

> +	spin_lock(&pernet->lock);
> +	list_del_rcu(&sched->list);
> +	spin_unlock(&pernet->lock);
> +
> +	/* avoid workqueue lockup */
> +	synchronize_rcu();

Hmm.  You mean 'wait until rcu read sides are done'?
But even that makes little sense to me.

There is no free operation done here so this is safe even
without synchronize_rcu()?

Re: [PATCH mptcp-next v7 4/8] mptcp: add sched in mptcp_sock

[Florian Westphal]

Geliang Tang <geliang.tang@suse.com> wrote:
> +void mptcp_release_sched(struct mptcp_sock *msk)
> +{
> +	if (msk->sched && msk->sched->release)
> +		msk->sched->release(msk);
> +	bpf_module_put(msk->sched, msk->sched->owner);
> +	msk->sched = NULL;

Strange.  First line makes sure we don't null deref on msk->sched ==
NULL, but bpf_module_put() passes msk->sched->owner?

Does bpf_module_put need to live in a 
if (msk->sched) { branch or is the null test redundant?

Re: [PATCH mptcp-next v7 1/8] mptcp: add struct mptcp_sched_ops

[Geliang Tang]

On Mon, Mar 28, 2022 at 11:49:19AM +0200, Florian Westphal wrote:
> Geliang Tang <geliang.tang@suse.com> wrote:
> > This patch defines struct mptcp_sched_ops, which has three struct members,
> > name, owner and list, and three function pointers, init, release and
> > get_subflow.
> > 
> > Add the scheduler registering, unregistering and finding functions to add
> > or delete or find a packet scheduler on the pernet sched_list.
> > 
> > Suggested-by: Florian Westphal <fw@strlen.de>
> 
> Huh?  I made some comments on earlier patch but I did not suggest this.
> In fact, I don't like this at all but I guess I've been vetoed wrt.
> pernet design.

Sorry Florian, I didn't use 'Suggested-by' tag correctly here. I'll drop
it in v8. At the weekly meeting last week, everyone did agree to use the
global list instead of the pernet list to implement sched list. I have no
strong intention of which type of list to use. I just think that if we
used pernet local_addr_list in PM netlink, it makes sense for sched list
to use pernet too.

How about switching to use global sched list in v8? I guess Mat won't
object.

> 
> > +struct mptcp_sched_ops *mptcp_sched_find(const struct net *net,
> > +					 const char *name)
> > +{
> > +	struct sched_pernet *pernet = sched_get_pernet(net);
> > +	struct mptcp_sched_ops *sched, *ret = NULL;
> > +
> > +	rcu_read_lock();
> > +	list_for_each_entry_rcu(sched, &pernet->sched_list, list) {
> > +		if (!strcmp(sched->name, name)) {
> > +			ret = sched;
> > +			break;
> > +		}
> > +	}
> > +	rcu_read_unlock();
> > +	return ret;
> 
> This is fishy.  Either caller must already hold rcu read lock,
> or mptcp_sched_ops can be free'd before function returns.
> 
> I suspect its best to force callers to do the rcu_read_lock and
> document that this function is called with rcu read lock held.

Agree, update in v8.

> 
> > +int mptcp_register_scheduler(const struct net *net,
> > +			     struct mptcp_sched_ops *sched)
> > +{
> > +	struct sched_pernet *pernet = sched_get_pernet(net);
> > +
> > +	if (!sched->get_subflow)
> > +		return -EINVAL;
> > +
> > +	if (mptcp_sched_find(net, sched->name))
> > +		return -EEXIST;
> > +
> > +	spin_lock(&pernet->lock);
> > +	list_add_tail_rcu(&sched->list, &pernet->sched_list);
> > +	spin_unlock(&pernet->lock);
> 
> This is racy.  Lock, test, add, unlock, not test, lock, ...

How should I fix this?

> 
> > +	spin_lock(&pernet->lock);
> > +	list_del_rcu(&sched->list);
> > +	spin_unlock(&pernet->lock);
> > +
> > +	/* avoid workqueue lockup */
> > +	synchronize_rcu();
> 
> Hmm.  You mean 'wait until rcu read sides are done'?
> But even that makes little sense to me.
> 
> There is no free operation done here so this is safe even
> without synchronize_rcu()?

To be honest, I don't know if I should use synchronize_rcu() here. But
I've tested it. If remove this synchronize_rcu(), kernel will crash, with
"BUG: workqueue lockup - pool ...!"

Thanks,
-Geliang

> 

Re: [PATCH mptcp-next v7 1/8] mptcp: add struct mptcp_sched_ops

[Florian Westphal]

Geliang Tang <geliang.tang@suse.com> wrote:
> > > +	if (!sched->get_subflow)
> > > +		return -EINVAL;
> > > +
> > > +	if (mptcp_sched_find(net, sched->name))
> > > +		return -EEXIST;
> > > +
> > > +	spin_lock(&pernet->lock);
> > > +	list_add_tail_rcu(&sched->list, &pernet->sched_list);
> > > +	spin_unlock(&pernet->lock);
> > 
> > This is racy.  Lock, test, add, unlock, not test, lock, ...
> 
> How should I fix this?

spin_lock(&pernet->lock);
if (mptcp_sched_find(net, sched->name)) {
	spin_unlock(&pernet->lock);
	return -EEXIST;
}
list_add_....

Re: [PATCH mptcp-next v7 4/8] mptcp: add sched in mptcp_sock

[Mat Martineau]

On Mon, 28 Mar 2022, Geliang Tang wrote:

> This patch adds a new struct member sched in struct mptcp_sock.
> And two helpers mptcp_init_sched() and mptcp_release_sched() to
> init and release it.
>
> Init it with the sysctl scheduler in mptcp_init_sock(), copy the
> scheduler from the parent in mptcp_sk_clone(), and release it in
> __mptcp_destroy_sock().
>
> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
> net/mptcp/protocol.c |  4 ++++
> net/mptcp/protocol.h |  4 ++++
> net/mptcp/sched.c    | 27 +++++++++++++++++++++++++++
> 3 files changed, 35 insertions(+)
>
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 2c684034fe7a..82b3846147a6 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -2662,6 +2662,8 @@ static int mptcp_init_sock(struct sock *sk)
> 	 * propagate the correct value
> 	 */
> 	mptcp_ca_reset(sk);
> +	mptcp_init_sched(mptcp_sk(sk),
> +			 mptcp_sched_find(net, mptcp_get_scheduler(net)));
>
> 	sk_sockets_allocated_inc(sk);
> 	sk->sk_rcvbuf = sock_net(sk)->ipv4.sysctl_tcp_rmem[1];
> @@ -2817,6 +2819,7 @@ static void __mptcp_destroy_sock(struct sock *sk)
> 	sk_stop_timer(sk, &sk->sk_timer);
> 	mptcp_data_unlock(sk);
> 	msk->pm.status = 0;
> +	mptcp_release_sched(msk);
>
> 	/* clears msk->subflow, allowing the following loop to close
> 	 * even the initial subflow
> @@ -2994,6 +2997,7 @@ struct sock *mptcp_sk_clone(const struct sock *sk,
> 	msk->snd_una = msk->write_seq;
> 	msk->wnd_end = msk->snd_nxt + req->rsk_rcv_wnd;
> 	msk->setsockopt_seq = mptcp_sk(sk)->setsockopt_seq;
> +	mptcp_init_sched(msk, mptcp_sk(sk)->sched);
>
> 	if (mp_opt->suboptions & OPTIONS_MPTCP_MPC) {
> 		msk->can_ack = true;
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index dd4a2afdb38b..81c35af7ade5 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -288,6 +288,7 @@ struct mptcp_sock {
> 	struct socket	*subflow; /* outgoing connect/listener/!mp_capable */
> 	struct sock	*first;
> 	struct mptcp_pm_data	pm;
> +	struct mptcp_sched_ops	*sched;
> 	struct {
> 		u32	space;	/* bytes copied in last measurement window */
> 		u32	copied; /* bytes copied in this measurement window */
> @@ -618,6 +619,9 @@ void mptcp_unregister_scheduler(const struct net *net,
> void mptcp_sched_init(void);
> struct sock *mptcp_subflow_get_send(struct mptcp_sock *msk);
> void mptcp_sched_data_init(struct sock *sk);
> +int mptcp_init_sched(struct mptcp_sock *msk,
> +		     struct mptcp_sched_ops *sched);
> +void mptcp_release_sched(struct mptcp_sock *msk);
>
> static inline bool __mptcp_subflow_active(struct mptcp_subflow_context *subflow)
> {
> diff --git a/net/mptcp/sched.c b/net/mptcp/sched.c
> index 1fb3dd24d6ff..9b3a3157111d 100644
> --- a/net/mptcp/sched.c
> +++ b/net/mptcp/sched.c
> @@ -125,3 +125,30 @@ void mptcp_sched_data_init(struct sock *sk)
> {
> 	mptcp_register_scheduler(sock_net(sk), &mptcp_sched_default);
> }
> +
> +int mptcp_init_sched(struct mptcp_sock *msk,
> +		     struct mptcp_sched_ops *sched)
> +{
> +	if (!sched)
> +		msk->sched = &mptcp_sched_default;
> +	else
> +		msk->sched = sched;
> +
> +	if (!bpf_try_module_get(msk->sched, msk->sched->owner))
> +		return -EBUSY;

The bpf_try_module_get() should be before msk->sched is assigned.

> +
> +	if (msk->sched->init)
> +		msk->sched->init(msk);
> +
> +	pr_debug("sched=%s", msk->sched->name);
> +
> +	return 0;
> +}
> +
> +void mptcp_release_sched(struct mptcp_sock *msk)
> +{
> +	if (msk->sched && msk->sched->release)
> +		msk->sched->release(msk);

Looks like the null check here is already addressed in the squash-to 
patch.

> +	bpf_module_put(msk->sched, msk->sched->owner);

Would be better to store a local pointer to the sched struct and clear 
msk->sched before doing the bpf_module_put() call. With the RCU grace 
period it's not likely to be a problem but might as well do the put last.

> +	msk->sched = NULL;
> +}
> -- 
> 2.34.1
>
>
>

--
Mat Martineau
Intel

Re: [PATCH mptcp-next v7 8/8] selftests: bpf: add mptcp_bpf_first test

[Mat Martineau]

On Mon, 28 Mar 2022, Geliang Tang wrote:

> This patch expends the MPTCP test base to support MPTCP packet
> scheduler tests. Add the mptcp_bpf_first scheduler test in it.
> Use sysctl to set net.mptcp.scheduler to use this sched.
>
> Some code in send_data() is from bpf_tcp_ca.c.
>
> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
> .../testing/selftests/bpf/prog_tests/mptcp.c  | 114 ++++++++++++++++++
> 1 file changed, 114 insertions(+)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/mptcp.c b/tools/testing/selftests/bpf/prog_tests/mptcp.c
> index 9b73ef62ee74..e5b6f2f4a52d 100644
> --- a/tools/testing/selftests/bpf/prog_tests/mptcp.c
> +++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c
> @@ -4,6 +4,9 @@
> #include <test_progs.h>
> #include "cgroup_helpers.h"
> #include "network_helpers.h"
> +#include "bpf_first.skel.h"

This didn't build for me - above needs to be changed to

#include "mptcp_bpf_first.skel.h"

to account for the .c file rename. But after that change, I was able to 
build and run the mptcp bpf test.

- Mat

> +
> +#define min(a, b) ((a) < (b) ? (a) : (b))
>
> #ifndef TCP_CA_NAME_MAX
> #define TCP_CA_NAME_MAX	16
> @@ -19,6 +22,8 @@ struct mptcp_storage {
> };
>
> static char monitor_log_path[64];
> +static const unsigned int total_bytes = 10 * 1024 * 1024;
> +static int stop, duration;
>
> static int verify_tsk(int map_fd, int client_fd)
> {
> @@ -251,8 +256,117 @@ void test_base(void)
> 	close(cgroup_fd);
> }
>
> +static void *server(void *arg)
> +{
> +	int lfd = (int)(long)arg, err = 0, fd;
> +	ssize_t nr_sent = 0, bytes = 0;
> +	char batch[1500];
> +
> +	fd = accept(lfd, NULL, NULL);
> +	while (fd == -1) {
> +		if (errno == EINTR)
> +			continue;
> +		err = -errno;
> +		goto done;
> +	}
> +
> +	if (settimeo(fd, 0)) {
> +		err = -errno;
> +		goto done;
> +	}
> +
> +	while (bytes < total_bytes && !READ_ONCE(stop)) {
> +		nr_sent = send(fd, &batch,
> +			       min(total_bytes - bytes, sizeof(batch)), 0);
> +		if (nr_sent == -1 && errno == EINTR)
> +			continue;
> +		if (nr_sent == -1) {
> +			err = -errno;
> +			break;
> +		}
> +		bytes += nr_sent;
> +	}
> +
> +	CHECK(bytes != total_bytes, "send", "%zd != %u nr_sent:%zd errno:%d\n",
> +	      bytes, total_bytes, nr_sent, errno);
> +
> +done:
> +	if (fd >= 0)
> +		close(fd);
> +	if (err) {
> +		WRITE_ONCE(stop, 1);
> +		return ERR_PTR(err);
> +	}
> +	return NULL;
> +}
> +
> +static void send_data(int lfd, int fd)
> +{
> +	ssize_t nr_recv = 0, bytes = 0;
> +	pthread_t srv_thread;
> +	void *thread_ret;
> +	char batch[1500];
> +	int err;
> +
> +	WRITE_ONCE(stop, 0);
> +
> +	err = pthread_create(&srv_thread, NULL, server, (void *)(long)lfd);
> +	if (CHECK(err != 0, "pthread_create", "err:%d errno:%d\n", err, errno))
> +		return;
> +
> +	/* recv total_bytes */
> +	while (bytes < total_bytes && !READ_ONCE(stop)) {
> +		nr_recv = recv(fd, &batch,
> +			       min(total_bytes - bytes, sizeof(batch)), 0);
> +		if (nr_recv == -1 && errno == EINTR)
> +			continue;
> +		if (nr_recv == -1)
> +			break;
> +		bytes += nr_recv;
> +	}
> +
> +	CHECK(bytes != total_bytes, "recv", "%zd != %u nr_recv:%zd errno:%d\n",
> +	      bytes, total_bytes, nr_recv, errno);
> +
> +	WRITE_ONCE(stop, 1);
> +
> +	pthread_join(srv_thread, &thread_ret);
> +	CHECK(IS_ERR(thread_ret), "pthread_join", "thread_ret:%ld",
> +	      PTR_ERR(thread_ret));
> +}
> +
> +static void test_first(void)
> +{
> +	struct bpf_first *first_skel;
> +	int server_fd, client_fd;
> +	struct bpf_link *link;
> +
> +	first_skel = bpf_first__open_and_load();
> +	if (CHECK(!first_skel, "bpf_first__open_and_load", "failed\n"))
> +		return;
> +
> +	link = bpf_map__attach_struct_ops(first_skel->maps.first);
> +	if (!ASSERT_OK_PTR(link, "bpf_map__attach_struct_ops")) {
> +		bpf_first__destroy(first_skel);
> +		return;
> +	}
> +
> +	system("sysctl -q net.mptcp.scheduler=mptcp_bpf_first");
> +	server_fd = start_mptcp_server(AF_INET, NULL, 0, 0);
> +	client_fd = connect_to_mptcp_fd(server_fd, 0);
> +
> +	send_data(server_fd, client_fd);
> +
> +	close(client_fd);
> +	close(server_fd);
> +	bpf_link__destroy(link);
> +	bpf_first__destroy(first_skel);
> +}
> +
> void test_mptcp(void)
> {
> 	if (test__start_subtest("base"))
> 		test_base();
> +	if (test__start_subtest("first"))
> +		test_first();
> }
> -- 
> 2.34.1
>
>
>

--
Mat Martineau
Intel

Re: selftests: bpf: add mptcp_bpf_first test: Build Failure

[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://patchwork.kernel.org/project/mptcp/patch/50798e945994585b9c9c5443ce991ceb3dbc662f.1648459866.git.geliang.tang@suse.com/
  https://github.com/multipath-tcp/mptcp_net-next/actions/runs/2066170068

Status: failure
Initiator: MPTCPimporter
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/ea9e0f5c2dfd

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 (Tessares)

Re: selftests: bpf: add mptcp_bpf_first test: Tests Results

[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:
  - Critical: 1 Call Trace(s) - Critical: Global Timeout ❌:
  - Task: https://cirrus-ci.com/task/5323883229216768
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5323883229216768/summary/summary.txt

- {"code":404,"message":
  - "HTTP 404 Not Found"}:
  - Task: https://cirrus-ci.com/task/6238759346765824
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6238759346765824/summary/summary.txt

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/ea9e0f5c2dfd

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 (Tessares)