[RFC mptcp-next v2 0/2] BPF packet scheduler updates part 4

[RFC mptcp-next v2 0/2] BPF packet scheduler updates part 4

[Geliang Tang]

v2:
 - keep sched API unchanged.
 - use bpf_sk_storage_lookup to get snd_burst from BPF context.
 - applied after "add bpf_stale scheduler v3" serise.

v1:

There's a bug in bpf_burst. snd_burst stored in mptcp_burst_storage in
BPF context is not used. msk->snd_burst is still used in kernel space.
To fix this, add two new interfaces in mptcp_sched_ops to get and set
scheduler's paramters from BPF context to kernel space.

Geliang Tang (2):
  mptcp: add set/get params wrappers
  mptcp: add bpf_burst set/get params

 include/net/bpf_sk_storage.h |  7 +++++
 net/core/bpf_sk_storage.c    |  2 +-
 net/mptcp/protocol.c         |  8 ++---
 net/mptcp/protocol.h         |  2 ++
 net/mptcp/sched.c            | 57 ++++++++++++++++++++++++++++++++++++
 5 files changed, 71 insertions(+), 5 deletions(-)

-- 
2.35.3

[RFC mptcp-next v2 1/2] mptcp: add set/get params wrappers

[Geliang Tang]

Add two sched params wrappers: mptcp_sched_set_params() and
mptcp_sched_set_params(). Use these wrappers instead of get and set
msk->snd_burst directly.

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

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index ab3aaf6cacc5..7c1e72bc5576 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -1436,7 +1436,7 @@ struct sock *mptcp_subflow_get_send(struct mptcp_sock *msk)
 	subflow->avg_pacing_rate = div_u64((u64)subflow->avg_pacing_rate * wmem +
 					   READ_ONCE(ssk->sk_pacing_rate) * burst,
 					   burst + wmem);
-	msk->snd_burst = burst;
+	mptcp_sched_set_params(msk, burst);
 	return ssk;
 }
 
@@ -1454,7 +1454,7 @@ static void mptcp_update_post_push(struct mptcp_sock *msk,
 
 	dfrag->already_sent += sent;
 
-	msk->snd_burst -= sent;
+	mptcp_sched_set_params(msk, mptcp_sched_get_params(msk) - sent);
 
 	snd_nxt_new += dfrag->already_sent;
 
@@ -1507,7 +1507,7 @@ static int __subflow_push_pending(struct sock *sk, struct sock *ssk,
 		}
 		WRITE_ONCE(msk->first_pending, mptcp_send_next(sk));
 
-		if (msk->snd_burst <= 0 ||
+		if (mptcp_sched_get_params(msk) <= 0 ||
 		    !sk_stream_memory_free(ssk) ||
 		    !mptcp_subflow_active(mptcp_subflow_ctx(ssk))) {
 			err = copied;
@@ -2291,7 +2291,7 @@ bool __mptcp_retransmit_pending_data(struct sock *sk)
 	mptcp_data_unlock(sk);
 
 	msk->first_pending = rtx_head;
-	msk->snd_burst = 0;
+	mptcp_sched_set_params(msk, 0);
 
 	/* be sure to clear the "sent status" on all re-injected fragments */
 	list_for_each_entry(cur, &msk->rtx_queue, list) {
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 12d70ce24843..dbd1881f8f28 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -678,6 +678,8 @@ void mptcp_sched_data_set_contexts(const struct mptcp_sock *msk,
 				   struct mptcp_sched_data *data);
 struct mptcp_subflow_context *
 mptcp_subflow_ctx_by_pos(const struct mptcp_sched_data *data, unsigned int pos);
+int mptcp_sched_get_params(struct mptcp_sock *msk);
+int mptcp_sched_set_params(struct mptcp_sock *msk, int burst);
 
 static inline bool __tcp_can_send(const struct sock *ssk)
 {
diff --git a/net/mptcp/sched.c b/net/mptcp/sched.c
index 2ae3f33bd244..c78ed61290d2 100644
--- a/net/mptcp/sched.c
+++ b/net/mptcp/sched.c
@@ -199,3 +199,21 @@ mptcp_subflow_ctx_by_pos(const struct mptcp_sched_data *data, unsigned int pos)
 		return NULL;
 	return data->contexts[pos];
 }
+
+int mptcp_sched_get_params(struct mptcp_sock *msk)
+{
+	if (msk->sched == &mptcp_sched_default)
+		return msk->snd_burst;
+
+	return 0;
+}
+
+int mptcp_sched_set_params(struct mptcp_sock *msk, int burst)
+{
+	if (msk->sched == &mptcp_sched_default) {
+		msk->snd_burst = burst;
+		return 0;
+	}
+
+	return 0;
+}
-- 
2.35.3

[RFC mptcp-next v2 2/2] mptcp: add bpf_burst set/get params

[Geliang Tang]

This patch implements the set/get params interfaces for bpf_burst
scheduler.

Export bpf_sk_storage_lookup(), use it to get bpf_local_storage_data
from sk->sk_bpf_storage, then get or set its params ptr->snd_burst.

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 include/net/bpf_sk_storage.h |  7 +++++++
 net/core/bpf_sk_storage.c    |  2 +-
 net/mptcp/sched.c            | 39 ++++++++++++++++++++++++++++++++++++
 3 files changed, 47 insertions(+), 1 deletion(-)

diff --git a/include/net/bpf_sk_storage.h b/include/net/bpf_sk_storage.h
index 2926f1f00d65..805142d59d02 100644
--- a/include/net/bpf_sk_storage.h
+++ b/include/net/bpf_sk_storage.h
@@ -37,6 +37,8 @@ int bpf_sk_storage_diag_put(struct bpf_sk_storage_diag *diag,
 			    struct sock *sk, struct sk_buff *skb,
 			    int stg_array_type,
 			    unsigned int *res_diag_size);
+struct bpf_local_storage_data *
+bpf_sk_storage_lookup(struct sock *sk, struct bpf_map *map, bool cacheit_lockit);
 #else
 static inline int bpf_sk_storage_clone(const struct sock *sk,
 				       struct sock *newsk)
@@ -58,6 +60,11 @@ static inline int bpf_sk_storage_diag_put(struct bpf_sk_storage_diag *diag,
 {
 	return 0;
 }
+struct bpf_local_storage_data *
+bpf_sk_storage_lookup(struct sock *sk, struct bpf_map *map, bool cacheit_lockit)
+{
+	return NULL;
+}
 #endif
 
 #endif /* _BPF_SK_STORAGE_H */
diff --git a/net/core/bpf_sk_storage.c b/net/core/bpf_sk_storage.c
index cca7594be92e..1c023f65017f 100644
--- a/net/core/bpf_sk_storage.c
+++ b/net/core/bpf_sk_storage.c
@@ -17,7 +17,7 @@
 
 DEFINE_BPF_STORAGE_CACHE(sk_cache);
 
-static struct bpf_local_storage_data *
+struct bpf_local_storage_data *
 bpf_sk_storage_lookup(struct sock *sk, struct bpf_map *map, bool cacheit_lockit)
 {
 	struct bpf_local_storage *sk_storage;
diff --git a/net/mptcp/sched.c b/net/mptcp/sched.c
index c78ed61290d2..067df28899a6 100644
--- a/net/mptcp/sched.c
+++ b/net/mptcp/sched.c
@@ -11,6 +11,7 @@
 #include <linux/list.h>
 #include <linux/rculist.h>
 #include <linux/spinlock.h>
+#include <net/bpf_sk_storage.h>
 #include "protocol.h"
 
 static DEFINE_SPINLOCK(mptcp_sched_list_lock);
@@ -202,18 +203,56 @@ mptcp_subflow_ctx_by_pos(const struct mptcp_sched_data *data, unsigned int pos)
 
 int mptcp_sched_get_params(struct mptcp_sock *msk)
 {
+	struct sock *sk = (struct sock *)msk;
+
 	if (msk->sched == &mptcp_sched_default)
 		return msk->snd_burst;
 
+	if (sk->sk_bpf_storage && sk->sk_bpf_storage->smap &&
+	    !strcmp(sk->sk_bpf_storage->smap->map.name, "mptcp_burst_map")) {
+		struct bpf_local_storage_data *sdata;
+
+		sdata = bpf_sk_storage_lookup(sk, &sk->sk_bpf_storage->smap->map, true);
+		if (sdata) {
+			struct mptcp_burst_storage {
+				int snd_burst;
+			} *ptr;
+
+			ptr = (struct mptcp_burst_storage *)sdata->data;
+			if (ptr)
+				return ptr->snd_burst;
+		}
+	}
+
 	return 0;
 }
 
 int mptcp_sched_set_params(struct mptcp_sock *msk, int burst)
 {
+	struct sock *sk = (struct sock *)msk;
+
 	if (msk->sched == &mptcp_sched_default) {
 		msk->snd_burst = burst;
 		return 0;
 	}
 
+	if (sk->sk_bpf_storage && sk->sk_bpf_storage->smap &&
+	    !strcmp(sk->sk_bpf_storage->smap->map.name, "mptcp_burst_map")) {
+		struct bpf_local_storage_data *sdata;
+
+		sdata = bpf_sk_storage_lookup(sk, &sk->sk_bpf_storage->smap->map, true);
+		if (sdata) {
+			struct mptcp_burst_storage {
+				int snd_burst;
+			} *ptr;
+
+			ptr = (struct mptcp_burst_storage *)sdata->data;
+			if (ptr) {
+				ptr->snd_burst = burst;
+				return 0;
+			}
+		}
+	}
+
 	return 0;
 }
-- 
2.35.3

Re: mptcp: add bpf_burst set/get params: 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/486891cb494ba4e9eeb02b6e220ca670dbc80866.1692788531.git.geliang.tang@suse.com/
  https://github.com/multipath-tcp/mptcp_net-next/actions/runs/5950638723

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

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: mptcp: add bpf_burst set/get params: 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 (except selftest_mptcp_join):
  - Unstable: 1 failed test(s): selftest_simult_flows 🔴:
  - Task: https://cirrus-ci.com/task/5518608940400640
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5518608940400640/summary/summary.txt

- KVM Validation: debug (only selftest_mptcp_join):
  - Unstable: 1 failed test(s): selftest_mptcp_join 🔴:
  - Task: https://cirrus-ci.com/task/5729715172933632
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5729715172933632/summary/summary.txt

- KVM Validation: normal (only selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/6644508847243264
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6644508847243264/summary/summary.txt

- KVM Validation: debug (except selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/4603815266091008
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/4603815266091008/summary/summary.txt

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


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-debug

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


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)

Re: [RFC mptcp-next v2 2/2] mptcp: add bpf_burst set/get params

[Mat Martineau]

On Wed, 23 Aug 2023, Geliang Tang wrote:

> This patch implements the set/get params interfaces for bpf_burst
> scheduler.
>
> Export bpf_sk_storage_lookup(), use it to get bpf_local_storage_data
> from sk->sk_bpf_storage, then get or set its params ptr->snd_burst.
>
> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
> include/net/bpf_sk_storage.h |  7 +++++++
> net/core/bpf_sk_storage.c    |  2 +-
> net/mptcp/sched.c            | 39 ++++++++++++++++++++++++++++++++++++
> 3 files changed, 47 insertions(+), 1 deletion(-)
>
> diff --git a/include/net/bpf_sk_storage.h b/include/net/bpf_sk_storage.h
> index 2926f1f00d65..805142d59d02 100644
> --- a/include/net/bpf_sk_storage.h
> +++ b/include/net/bpf_sk_storage.h
> @@ -37,6 +37,8 @@ int bpf_sk_storage_diag_put(struct bpf_sk_storage_diag *diag,
> 			    struct sock *sk, struct sk_buff *skb,
> 			    int stg_array_type,
> 			    unsigned int *res_diag_size);
> +struct bpf_local_storage_data *
> +bpf_sk_storage_lookup(struct sock *sk, struct bpf_map *map, bool cacheit_lockit);
> #else
> static inline int bpf_sk_storage_clone(const struct sock *sk,
> 				       struct sock *newsk)
> @@ -58,6 +60,11 @@ static inline int bpf_sk_storage_diag_put(struct bpf_sk_storage_diag *diag,
> {
> 	return 0;
> }
> +struct bpf_local_storage_data *
> +bpf_sk_storage_lookup(struct sock *sk, struct bpf_map *map, bool cacheit_lockit)
> +{
> +	return NULL;
> +}
> #endif
>
> #endif /* _BPF_SK_STORAGE_H */
> diff --git a/net/core/bpf_sk_storage.c b/net/core/bpf_sk_storage.c
> index cca7594be92e..1c023f65017f 100644
> --- a/net/core/bpf_sk_storage.c
> +++ b/net/core/bpf_sk_storage.c
> @@ -17,7 +17,7 @@
>
> DEFINE_BPF_STORAGE_CACHE(sk_cache);
>
> -static struct bpf_local_storage_data *
> +struct bpf_local_storage_data *
> bpf_sk_storage_lookup(struct sock *sk, struct bpf_map *map, bool cacheit_lockit)
> {
> 	struct bpf_local_storage *sk_storage;
> diff --git a/net/mptcp/sched.c b/net/mptcp/sched.c
> index c78ed61290d2..067df28899a6 100644
> --- a/net/mptcp/sched.c
> +++ b/net/mptcp/sched.c
> @@ -11,6 +11,7 @@
> #include <linux/list.h>
> #include <linux/rculist.h>
> #include <linux/spinlock.h>
> +#include <net/bpf_sk_storage.h>
> #include "protocol.h"
>
> static DEFINE_SPINLOCK(mptcp_sched_list_lock);
> @@ -202,18 +203,56 @@ mptcp_subflow_ctx_by_pos(const struct mptcp_sched_data *data, unsigned int pos)
>
> int mptcp_sched_get_params(struct mptcp_sock *msk)
> {
> +	struct sock *sk = (struct sock *)msk;
> +
> 	if (msk->sched == &mptcp_sched_default)
> 		return msk->snd_burst;
>
> +	if (sk->sk_bpf_storage && sk->sk_bpf_storage->smap &&
> +	    !strcmp(sk->sk_bpf_storage->smap->map.name, "mptcp_burst_map")) {
> +		struct bpf_local_storage_data *sdata;
> +
> +		sdata = bpf_sk_storage_lookup(sk, &sk->sk_bpf_storage->smap->map, true);
> +		if (sdata) {
> +			struct mptcp_burst_storage {
> +				int snd_burst;
> +			} *ptr;
> +
> +			ptr = (struct mptcp_burst_storage *)sdata->data;
> +			if (ptr)
> +				return ptr->snd_burst;
> +		}
> +	}
> +
> 	return 0;
> }

This seems very complex. Why implement it this way versus adding snd_burst 
to 'struct mptcp_sock' in bpf_tcp_helpers.h?

- Mat


>
> int mptcp_sched_set_params(struct mptcp_sock *msk, int burst)
> {
> +	struct sock *sk = (struct sock *)msk;
> +
> 	if (msk->sched == &mptcp_sched_default) {
> 		msk->snd_burst = burst;
> 		return 0;
> 	}
>
> +	if (sk->sk_bpf_storage && sk->sk_bpf_storage->smap &&
> +	    !strcmp(sk->sk_bpf_storage->smap->map.name, "mptcp_burst_map")) {
> +		struct bpf_local_storage_data *sdata;
> +
> +		sdata = bpf_sk_storage_lookup(sk, &sk->sk_bpf_storage->smap->map, true);
> +		if (sdata) {
> +			struct mptcp_burst_storage {
> +				int snd_burst;
> +			} *ptr;
> +
> +			ptr = (struct mptcp_burst_storage *)sdata->data;
> +			if (ptr) {
> +				ptr->snd_burst = burst;
> +				return 0;
> +			}
> +		}
> +	}
> +
> 	return 0;
> }
> -- 
> 2.35.3
>
>
>