[PATCH mptcp-next v2 08/13] mptcp: add subflow_set_stale helper

Geliang Tang posted 13 patches 2 years, 6 months ago
Maintainers: Matthieu Baerts <matthieu.baerts@tessares.net>, Mat Martineau <martineau@kernel.org>, "David S. Miller" <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>, Andrii Nakryiko <andrii@kernel.org>, Mykola Lysenko <mykolal@fb.com>, Alexei Starovoitov <ast@kernel.org>, Daniel Borkmann <daniel@iogearbox.net>, Martin KaFai Lau <martin.lau@linux.dev>, Song Liu <song@kernel.org>, Yonghong Song <yhs@fb.com>, John Fastabend <john.fastabend@gmail.com>, KP Singh <kpsingh@kernel.org>, Stanislav Fomichev <sdf@google.com>, Hao Luo <haoluo@google.com>, Jiri Olsa <jolsa@kernel.org>, Shuah Khan <shuah@kernel.org>
[PATCH mptcp-next v2 08/13] mptcp: add subflow_set_stale helper
Posted by Geliang Tang 2 years, 6 months ago
Add mptcp_subflow_set_stale() helper.

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

diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c
index f388baf08d49..e6f94d5e22fb 100644
--- a/net/mptcp/bpf.c
+++ b/net/mptcp/bpf.c
@@ -146,6 +146,7 @@ struct bpf_struct_ops bpf_mptcp_sched_ops = {
 
 BTF_SET8_START(bpf_mptcp_sched_kfunc_ids)
 BTF_ID_FLAGS(func, mptcp_subflow_set_scheduled)
+BTF_ID_FLAGS(func, mptcp_subflow_set_stale)
 BTF_ID_FLAGS(func, mptcp_sched_data_set_contexts)
 BTF_ID_FLAGS(func, mptcp_subflow_ctx_by_pos)
 BTF_SET8_END(bpf_mptcp_sched_kfunc_ids)
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 1b4457c44fe8..78b3beeb7d7a 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -662,6 +662,8 @@ int mptcp_init_sched(struct mptcp_sock *msk,
 void mptcp_release_sched(struct mptcp_sock *msk);
 void mptcp_subflow_set_scheduled(struct mptcp_subflow_context *subflow,
 				 bool scheduled);
+void mptcp_subflow_set_stale(struct mptcp_subflow_context *subflow,
+			     int stale);
 void mptcp_sched_data_set_contexts(const struct mptcp_sock *msk,
 				   struct mptcp_sched_data *data);
 struct mptcp_subflow_context *
diff --git a/net/mptcp/sched.c b/net/mptcp/sched.c
index a80cf0481edf..3f361e75e38c 100644
--- a/net/mptcp/sched.c
+++ b/net/mptcp/sched.c
@@ -127,6 +127,12 @@ void mptcp_subflow_set_scheduled(struct mptcp_subflow_context *subflow,
 	WRITE_ONCE(subflow->scheduled, scheduled);
 }
 
+void mptcp_subflow_set_stale(struct mptcp_subflow_context *subflow,
+			     int stale)
+{
+	subflow->stale = stale;
+}
+
 void mptcp_sched_data_set_contexts(const struct mptcp_sock *msk,
 				   struct mptcp_sched_data *data)
 {
-- 
2.35.3
Re: [PATCH mptcp-next v2 08/13] mptcp: add subflow_set_stale helper
Posted by Mat Martineau 2 years, 6 months ago
On Tue, 18 Jul 2023, Geliang Tang wrote:

> Add mptcp_subflow_set_stale() helper.
>

I was trying to remember the use case for this. The closest thing I can 
find is this:

     => the scheduler should be able to interact with the "stale" logic
        we have in the upstream kernel: mark a subflow as stale / back
        active.

from 
https://lore.kernel.org/mptcp/dd4363e7-d057-97e8-0b5f-8570f39aa538@tessares.net/

I think that need is met by the change in patch 11 to export 
mptcp_pm_subflow_chk_stale - if a particular scheduler wants to track some 
stale-like metric, it can do so with its own sk_storage values. There's 
also the matter of coordinating the values of stale_count and 
stale_rcv_tstamp.

My suggestion is to drop patches 8, 9, and 10 unless there's a compelling 
reason to allow a BPF scheduler to control this bit. If there's a need for 
it please speak up!


- Mat



> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
> net/mptcp/bpf.c      | 1 +
> net/mptcp/protocol.h | 2 ++
> net/mptcp/sched.c    | 6 ++++++
> 3 files changed, 9 insertions(+)
>
> diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c
> index f388baf08d49..e6f94d5e22fb 100644
> --- a/net/mptcp/bpf.c
> +++ b/net/mptcp/bpf.c
> @@ -146,6 +146,7 @@ struct bpf_struct_ops bpf_mptcp_sched_ops = {
>
> BTF_SET8_START(bpf_mptcp_sched_kfunc_ids)
> BTF_ID_FLAGS(func, mptcp_subflow_set_scheduled)
> +BTF_ID_FLAGS(func, mptcp_subflow_set_stale)
> BTF_ID_FLAGS(func, mptcp_sched_data_set_contexts)
> BTF_ID_FLAGS(func, mptcp_subflow_ctx_by_pos)
> BTF_SET8_END(bpf_mptcp_sched_kfunc_ids)
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 1b4457c44fe8..78b3beeb7d7a 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -662,6 +662,8 @@ int mptcp_init_sched(struct mptcp_sock *msk,
> void mptcp_release_sched(struct mptcp_sock *msk);
> void mptcp_subflow_set_scheduled(struct mptcp_subflow_context *subflow,
> 				 bool scheduled);
> +void mptcp_subflow_set_stale(struct mptcp_subflow_context *subflow,
> +			     int stale);
> void mptcp_sched_data_set_contexts(const struct mptcp_sock *msk,
> 				   struct mptcp_sched_data *data);
> struct mptcp_subflow_context *
> diff --git a/net/mptcp/sched.c b/net/mptcp/sched.c
> index a80cf0481edf..3f361e75e38c 100644
> --- a/net/mptcp/sched.c
> +++ b/net/mptcp/sched.c
> @@ -127,6 +127,12 @@ void mptcp_subflow_set_scheduled(struct mptcp_subflow_context *subflow,
> 	WRITE_ONCE(subflow->scheduled, scheduled);
> }
>
> +void mptcp_subflow_set_stale(struct mptcp_subflow_context *subflow,
> +			     int stale)
> +{
> +	subflow->stale = stale;
> +}
> +
> void mptcp_sched_data_set_contexts(const struct mptcp_sock *msk,
> 				   struct mptcp_sched_data *data)
> {
> -- 
> 2.35.3
>
>
>
Re: [PATCH mptcp-next v2 08/13] mptcp: add subflow_set_stale helper
Posted by Matthieu Baerts 2 years, 6 months ago
Hi Mat,

On 28/07/2023 03:30, Mat Martineau wrote:
> On Tue, 18 Jul 2023, Geliang Tang wrote:
> 
>> Add mptcp_subflow_set_stale() helper.
>>
> 
> I was trying to remember the use case for this. The closest thing I can
> find is this:
> 
>     => the scheduler should be able to interact with the "stale" logic
>        we have in the upstream kernel: mark a subflow as stale / back
>        active.
> 
> from
> https://lore.kernel.org/mptcp/dd4363e7-d057-97e8-0b5f-8570f39aa538@tessares.net/
> 
> I think that need is met by the change in patch 11 to export
> mptcp_pm_subflow_chk_stale - if a particular scheduler wants to track
> some stale-like metric, it can do so with its own sk_storage values.
> There's also the matter of coordinating the values of stale_count and
> stale_rcv_tstamp.
> 
> My suggestion is to drop patches 8, 9, and 10 unless there's a
> compelling reason to allow a BPF scheduler to control this bit. If
> there's a need for it please speak up!

If I'm not mistaken, the 'stale' logic is currently used by the core not
to use a subflow (when calling mptcp_subflow_active()). A packet
scheduler might want to proactively mark a subflow as stall, e.g. if the
latency is too high, too many retransmissions, etc. and just set the
subflow as stale.

But maybe we cannot do that because the core will reset stale in some
conditions? Maybe we need something similar but not reusing "stale"?

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net
Re: [PATCH mptcp-next v2 08/13] mptcp: add subflow_set_stale helper
Posted by Mat Martineau 2 years, 6 months ago
On Fri, 28 Jul 2023, Matthieu Baerts wrote:

> Hi Mat,
>
> On 28/07/2023 03:30, Mat Martineau wrote:
>> On Tue, 18 Jul 2023, Geliang Tang wrote:
>>
>>> Add mptcp_subflow_set_stale() helper.
>>>
>>
>> I was trying to remember the use case for this. The closest thing I can
>> find is this:
>>
>>     => the scheduler should be able to interact with the "stale" logic
>>        we have in the upstream kernel: mark a subflow as stale / back
>>        active.
>>
>> from
>> https://lore.kernel.org/mptcp/dd4363e7-d057-97e8-0b5f-8570f39aa538@tessares.net/
>>
>> I think that need is met by the change in patch 11 to export
>> mptcp_pm_subflow_chk_stale - if a particular scheduler wants to track
>> some stale-like metric, it can do so with its own sk_storage values.
>> There's also the matter of coordinating the values of stale_count and
>> stale_rcv_tstamp.
>>
>> My suggestion is to drop patches 8, 9, and 10 unless there's a
>> compelling reason to allow a BPF scheduler to control this bit. If
>> there's a need for it please speak up!
>
> If I'm not mistaken, the 'stale' logic is currently used by the core not
> to use a subflow (when calling mptcp_subflow_active()). A packet
> scheduler might want to proactively mark a subflow as stall, e.g. if the
> latency is too high, too many retransmissions, etc. and just set the
> subflow as stale.
>
> But maybe we cannot do that because the core will reset stale in some
> conditions? Maybe we need something similar but not reusing "stale"?
>

That's what I'm thinking, yes. Individual BPF schedulers could create 
their own "skip this subflow if possible" variables in their custom 
sk_storage structs, and those would not have the complex interactions with 
stale_count and stale_rcv_tstamp.


- Mat