From: Mat Martineau <martineau@kernel.org>
To: Geliang Tang <geliang@kernel.org>
Cc: mptcp@lists.linux.dev, baohua@kernel.org,
Geliang Tang <tanggeliang@kylinos.cn>
Subject: Re: [PATCH mptcp-next 1/8] mptcp: split get_subflow interface into two
Date: Fri, 20 Dec 2024 17:23:58 -0800 (PST) [thread overview]
Message-ID: <8395b8e6-fe26-ec0d-4fb9-2aad0bfefb3b@kernel.org> (raw)
In-Reply-To: <5887e174fc52db76e9ce6a4683a694a693f8567a.1732153672.git.tanggeliang@kylinos.cn>
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
next prev parent reply other threads:[~2024-12-21 1:24 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-11-21 2:45 [PATCH mptcp-next 0/8] split get_subflow interface into two Geliang Tang
2024-11-21 2:45 ` [PATCH mptcp-next 1/8] mptcp: " Geliang Tang
2024-12-21 1:23 ` Mat Martineau [this message]
2024-11-21 2:45 ` [PATCH mptcp-next 2/8] Squash to "selftests/bpf: Add bpf_first scheduler & test" Geliang Tang
2024-12-21 1:28 ` Mat Martineau
2024-12-23 9:49 ` Geliang Tang
2024-11-21 2:45 ` [PATCH mptcp-next 3/8] Squash to "selftests/bpf: Add bpf_bkup " Geliang Tang
2024-11-21 2:45 ` [PATCH mptcp-next 4/8] Squash to "selftests/bpf: Add bpf_rr " Geliang Tang
2024-11-21 2:45 ` [PATCH mptcp-next 5/8] Squash to "selftests/bpf: Add bpf_red " Geliang Tang
2024-11-21 2:45 ` [PATCH mptcp-next 6/8] Squash to "selftests/bpf: Add bpf_burst " Geliang Tang
2024-11-21 2:45 ` [PATCH mptcp-next 7/8] Squash to "bpf: Export mptcp packet scheduler helpers" Geliang Tang
2024-11-21 2:45 ` [PATCH mptcp-next 8/8] Squash to "selftests/bpf: Add bpf_burst scheduler & test" Geliang Tang
2024-11-21 3:55 ` [PATCH mptcp-next 0/8] split get_subflow interface into two MPTCP CI
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8395b8e6-fe26-ec0d-4fb9-2aad0bfefb3b@kernel.org \
--to=martineau@kernel.org \
--cc=baohua@kernel.org \
--cc=geliang@kernel.org \
--cc=mptcp@lists.linux.dev \
--cc=tanggeliang@kylinos.cn \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox