From: Geliang Tang <tanggeliang@kylinos.cn>

get_retrans() interface of the burst packet scheduler invokes a sleeping
function mptcp_pm_subflow_chk_stale(), which calls __lock_sock_fast().
So get_retrans() interface should be set with BPF_F_SLEEPABLE flag in
BPF. But get_send() interface of this scheduler can't be set with
BPF_F_SLEEPABLE flag since it's invoked in ack_update_msk() under mptcp
data lock.

So this patch has to split get_subflow() interface of packet scheduer into
two interfaces: get_send() and get_retrans(). Then we can set get_retrans()
interface alone with BPF_F_SLEEPABLE flag.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 include/net/mptcp.h |  5 +++--
 net/mptcp/bpf.c     | 11 +++++++++--
 net/mptcp/sched.c   | 33 ++++++++++++++++++++++-----------
 3 files changed, 34 insertions(+), 15 deletions(-)

diff --git a/include/net/mptcp.h b/include/net/mptcp.h
index 814b5f2e3ed5..2c85ca92bb1c 100644
--- a/include/net/mptcp.h
+++ b/include/net/mptcp.h
@@ -103,13 +103,14 @@ struct mptcp_out_options {
 #define MPTCP_SUBFLOWS_MAX	8
 
 struct mptcp_sched_data {
-	bool	reinject;
 	u8	subflows;
 	struct mptcp_subflow_context *contexts[MPTCP_SUBFLOWS_MAX];
 };
 
 struct mptcp_sched_ops {
-	int (*get_subflow)(struct mptcp_sock *msk,
+	int (*get_send)(struct mptcp_sock *msk,
+			struct mptcp_sched_data *data);
+	int (*get_retrans)(struct mptcp_sock *msk,
 			   struct mptcp_sched_data *data);
 
 	char			name[MPTCP_SCHED_NAME_MAX];
diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c
index e9db856972cb..ab1de189ba4b 100644
--- a/net/mptcp/bpf.c
+++ b/net/mptcp/bpf.c
@@ -156,7 +156,13 @@ static int bpf_mptcp_sched_init(struct btf *btf)
 	return 0;
 }
 
-static int __bpf_mptcp_sched_get_subflow(struct mptcp_sock *msk,
+static int __bpf_mptcp_sched_get_send(struct mptcp_sock *msk,
+				      struct mptcp_sched_data *data)
+{
+	return 0;
+}
+
+static int __bpf_mptcp_sched_get_retrans(struct mptcp_sock *msk,
 					 struct mptcp_sched_data *data)
 {
 	return 0;
@@ -171,7 +177,8 @@ static void __bpf_mptcp_sched_release(struct mptcp_sock *msk)
 }
 
 static struct mptcp_sched_ops __bpf_mptcp_sched_ops = {
-	.get_subflow	= __bpf_mptcp_sched_get_subflow,
+	.get_send	= __bpf_mptcp_sched_get_send,
+	.get_retrans	= __bpf_mptcp_sched_get_retrans,
 	.init		= __bpf_mptcp_sched_init,
 	.release	= __bpf_mptcp_sched_release,
 };
diff --git a/net/mptcp/sched.c b/net/mptcp/sched.c
index 6da60e35932f..7560693e5b7a 100644
--- a/net/mptcp/sched.c
+++ b/net/mptcp/sched.c
@@ -16,13 +16,25 @@
 static DEFINE_SPINLOCK(mptcp_sched_list_lock);
 static LIST_HEAD(mptcp_sched_list);
 
-static int mptcp_sched_default_get_subflow(struct mptcp_sock *msk,
+static int mptcp_sched_default_get_send(struct mptcp_sock *msk,
+					struct mptcp_sched_data *data)
+{
+	struct sock *ssk;
+
+	ssk = mptcp_subflow_get_send(msk);
+	if (!ssk)
+		return -EINVAL;
+
+	mptcp_subflow_set_scheduled(mptcp_subflow_ctx(ssk), true);
+	return 0;
+}
+
+static int mptcp_sched_default_get_retrans(struct mptcp_sock *msk,
 					   struct mptcp_sched_data *data)
 {
 	struct sock *ssk;
 
-	ssk = data->reinject ? mptcp_subflow_get_retrans(msk) :
-			       mptcp_subflow_get_send(msk);
+	ssk = mptcp_subflow_get_retrans(msk);
 	if (!ssk)
 		return -EINVAL;
 
@@ -31,7 +43,8 @@ static int mptcp_sched_default_get_subflow(struct mptcp_sock *msk,
 }
 
 static struct mptcp_sched_ops mptcp_sched_default = {
-	.get_subflow	= mptcp_sched_default_get_subflow,
+	.get_send	= mptcp_sched_default_get_send,
+	.get_retrans	= mptcp_sched_default_get_retrans,
 	.name		= "default",
 	.owner		= THIS_MODULE,
 };
@@ -73,7 +86,7 @@ void mptcp_get_available_schedulers(char *buf, size_t maxlen)
 
 int mptcp_register_scheduler(struct mptcp_sched_ops *sched)
 {
-	if (!sched->get_subflow)
+	if (!sched->get_send || !sched->get_retrans)
 		return -EINVAL;
 
 	spin_lock(&mptcp_sched_list_lock);
@@ -184,11 +197,10 @@ int mptcp_sched_get_send(struct mptcp_sock *msk)
 			return 0;
 	}
 
-	data.reinject = false;
 	if (msk->sched == &mptcp_sched_default || !msk->sched)
-		return mptcp_sched_default_get_subflow(msk, &data);
+		return mptcp_sched_default_get_send(msk, &data);
 	mptcp_sched_data_set_contexts(msk, &data);
-	return msk->sched->get_subflow(msk, &data);
+	return msk->sched->get_send(msk, &data);
 }
 
 int mptcp_sched_get_retrans(struct mptcp_sock *msk)
@@ -207,9 +219,8 @@ int mptcp_sched_get_retrans(struct mptcp_sock *msk)
 			return 0;
 	}
 
-	data.reinject = true;
 	if (msk->sched == &mptcp_sched_default || !msk->sched)
-		return mptcp_sched_default_get_subflow(msk, &data);
+		return mptcp_sched_default_get_retrans(msk, &data);
 	mptcp_sched_data_set_contexts(msk, &data);
-	return msk->sched->get_subflow(msk, &data);
+	return msk->sched->get_retrans(msk, &data);
 }
-- 
2.43.0

On Thu, 21 Nov 2024, Geliang Tang wrote:

> From: Geliang Tang <tanggeliang@kylinos.cn>
>
> get_retrans() interface of the burst packet scheduler invokes a sleeping
> function mptcp_pm_subflow_chk_stale(), which calls __lock_sock_fast().
> So get_retrans() interface should be set with BPF_F_SLEEPABLE flag in
> BPF. But get_send() interface of this scheduler can't be set with
> BPF_F_SLEEPABLE flag since it's invoked in ack_update_msk() under mptcp
> data lock.
>
> So this patch has to split get_subflow() interface of packet scheduer into
> two interfaces: get_send() and get_retrans(). Then we can set get_retrans()
> interface alone with BPF_F_SLEEPABLE flag.
>
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
> include/net/mptcp.h |  5 +++--
> net/mptcp/bpf.c     | 11 +++++++++--
> net/mptcp/sched.c   | 33 ++++++++++++++++++++++-----------
> 3 files changed, 34 insertions(+), 15 deletions(-)
>
> diff --git a/include/net/mptcp.h b/include/net/mptcp.h
> index 814b5f2e3ed5..2c85ca92bb1c 100644
> --- a/include/net/mptcp.h
> +++ b/include/net/mptcp.h
> @@ -103,13 +103,14 @@ struct mptcp_out_options {
> #define MPTCP_SUBFLOWS_MAX	8
>
> struct mptcp_sched_data {
> -	bool	reinject;
> 	u8	subflows;
> 	struct mptcp_subflow_context *contexts[MPTCP_SUBFLOWS_MAX];
> };
>
> struct mptcp_sched_ops {
> -	int (*get_subflow)(struct mptcp_sock *msk,
> +	int (*get_send)(struct mptcp_sock *msk,
> +			struct mptcp_sched_data *data);
> +	int (*get_retrans)(struct mptcp_sock *msk,
> 			   struct mptcp_sched_data *data);
>
> 	char			name[MPTCP_SCHED_NAME_MAX];
> diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c
> index e9db856972cb..ab1de189ba4b 100644
> --- a/net/mptcp/bpf.c
> +++ b/net/mptcp/bpf.c
> @@ -156,7 +156,13 @@ static int bpf_mptcp_sched_init(struct btf *btf)
> 	return 0;
> }
>
> -static int __bpf_mptcp_sched_get_subflow(struct mptcp_sock *msk,
> +static int __bpf_mptcp_sched_get_send(struct mptcp_sock *msk,
> +				      struct mptcp_sched_data *data)
> +{
> +	return 0;
> +}
> +
> +static int __bpf_mptcp_sched_get_retrans(struct mptcp_sock *msk,
> 					 struct mptcp_sched_data *data)
> {
> 	return 0;
> @@ -171,7 +177,8 @@ static void __bpf_mptcp_sched_release(struct mptcp_sock *msk)
> }
>
> static struct mptcp_sched_ops __bpf_mptcp_sched_ops = {
> -	.get_subflow	= __bpf_mptcp_sched_get_subflow,
> +	.get_send	= __bpf_mptcp_sched_get_send,
> +	.get_retrans	= __bpf_mptcp_sched_get_retrans,
> 	.init		= __bpf_mptcp_sched_init,
> 	.release	= __bpf_mptcp_sched_release,
> };
> diff --git a/net/mptcp/sched.c b/net/mptcp/sched.c
> index 6da60e35932f..7560693e5b7a 100644
> --- a/net/mptcp/sched.c
> +++ b/net/mptcp/sched.c
> @@ -16,13 +16,25 @@
> static DEFINE_SPINLOCK(mptcp_sched_list_lock);
> static LIST_HEAD(mptcp_sched_list);
>
> -static int mptcp_sched_default_get_subflow(struct mptcp_sock *msk,
> +static int mptcp_sched_default_get_send(struct mptcp_sock *msk,
> +					struct mptcp_sched_data *data)
> +{
> +	struct sock *ssk;
> +
> +	ssk = mptcp_subflow_get_send(msk);
> +	if (!ssk)
> +		return -EINVAL;
> +
> +	mptcp_subflow_set_scheduled(mptcp_subflow_ctx(ssk), true);
> +	return 0;
> +}
> +
> +static int mptcp_sched_default_get_retrans(struct mptcp_sock *msk,
> 					   struct mptcp_sched_data *data)
> {
> 	struct sock *ssk;
>
> -	ssk = data->reinject ? mptcp_subflow_get_retrans(msk) :
> -			       mptcp_subflow_get_send(msk);
> +	ssk = mptcp_subflow_get_retrans(msk);
> 	if (!ssk)
> 		return -EINVAL;
>
> @@ -31,7 +43,8 @@ static int mptcp_sched_default_get_subflow(struct mptcp_sock *msk,
> }
>
> static struct mptcp_sched_ops mptcp_sched_default = {
> -	.get_subflow	= mptcp_sched_default_get_subflow,
> +	.get_send	= mptcp_sched_default_get_send,
> +	.get_retrans	= mptcp_sched_default_get_retrans,
> 	.name		= "default",
> 	.owner		= THIS_MODULE,
> };
> @@ -73,7 +86,7 @@ void mptcp_get_available_schedulers(char *buf, size_t maxlen)
>
> int mptcp_register_scheduler(struct mptcp_sched_ops *sched)
> {
> -	if (!sched->get_subflow)
> +	if (!sched->get_send || !sched->get_retrans)

Hi Geliang -

Looking at patches 2-5, they all implement identical code for get_send and 
get_retrans. It might be helpful for simpler schedulers to not have to 
define two handlers if they do the same thing. My suggestion is to only 
require get_send, and if get_retrans is NULL in mptcp_sched_ops then use 
get_send for retransmits too.

> 		return -EINVAL;
>
> 	spin_lock(&mptcp_sched_list_lock);
> @@ -184,11 +197,10 @@ int mptcp_sched_get_send(struct mptcp_sock *msk)
> 			return 0;
> 	}
>
> -	data.reinject = false;
> 	if (msk->sched == &mptcp_sched_default || !msk->sched)
> -		return mptcp_sched_default_get_subflow(msk, &data);
> +		return mptcp_sched_default_get_send(msk, &data);
> 	mptcp_sched_data_set_contexts(msk, &data);
> -	return msk->sched->get_subflow(msk, &data);
> +	return msk->sched->get_send(msk, &data);
> }
>
> int mptcp_sched_get_retrans(struct mptcp_sock *msk)
> @@ -207,9 +219,8 @@ int mptcp_sched_get_retrans(struct mptcp_sock *msk)
> 			return 0;
> 	}
>
> -	data.reinject = true;
> 	if (msk->sched == &mptcp_sched_default || !msk->sched)
> -		return mptcp_sched_default_get_subflow(msk, &data);
> +		return mptcp_sched_default_get_retrans(msk, &data);
> 	mptcp_sched_data_set_contexts(msk, &data);
> -	return msk->sched->get_subflow(msk, &data);
> +	return msk->sched->get_retrans(msk, &data);

As I mentioned above, this could revert to get_send() if get_retrans is 
NULL.

- Mat