Re: [PATCH mptcp-next v11 2/3] selftests: bpf: add bpf_rr scheduler

[Mat Martineau <mathew.j.martineau@linux.intel.com>]

On Tue, 3 May 2022, Geliang Tang wrote:

> This patch implements the round-robin BPF MPTCP scheduler, named bpf_rr,
> which always picks the next available subflow to send data. If no such
> next subflow available, picks the first one.
>
> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
> .../testing/selftests/bpf/bpf_mptcp_helpers.h |  6 +++
> .../selftests/bpf/progs/mptcp_bpf_rr.c        | 48 +++++++++++++++++++
> 2 files changed, 54 insertions(+)
> create mode 100644 tools/testing/selftests/bpf/progs/mptcp_bpf_rr.c
>
> diff --git a/tools/testing/selftests/bpf/bpf_mptcp_helpers.h b/tools/testing/selftests/bpf/bpf_mptcp_helpers.h
> index 7ecd0b14666a..2d5109f459b4 100644
> --- a/tools/testing/selftests/bpf/bpf_mptcp_helpers.h
> +++ b/tools/testing/selftests/bpf/bpf_mptcp_helpers.h
> @@ -29,10 +29,16 @@ struct mptcp_sched_ops {
> struct mptcp_sock {
> 	struct inet_connection_sock	sk;
>
> +	struct sock	*last_snd;
> 	__u32		token;
> 	struct sock	*first;
> 	struct mptcp_sched_ops *sched;
> 	char		ca_name[TCP_CA_NAME_MAX];
> } __attribute__((preserve_access_index));
>
> +struct mptcp_subflow_context {
> +	__u32	token;
> +	struct	sock *tcp_sock;	    /* tcp sk backpointer */
> +} __attribute__((preserve_access_index));
> +
> #endif
> diff --git a/tools/testing/selftests/bpf/progs/mptcp_bpf_rr.c b/tools/testing/selftests/bpf/progs/mptcp_bpf_rr.c
> new file mode 100644
> index 000000000000..ab0bb2a38891
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/mptcp_bpf_rr.c
> @@ -0,0 +1,48 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2022, SUSE. */
> +
> +#include <linux/bpf.h>
> +#include <linux/stddef.h>
> +#include <linux/tcp.h>
> +#include "bpf_mptcp_helpers.h"
> +
> +char _license[] SEC("license") = "GPL";
> +
> +SEC("struct_ops/mptcp_sched_rr_init")
> +void BPF_PROG(mptcp_sched_rr_init, const struct mptcp_sock *msk)
> +{
> +}
> +
> +SEC("struct_ops/mptcp_sched_rr_release")
> +void BPF_PROG(mptcp_sched_rr_release, const struct mptcp_sock *msk)
> +{
> +}
> +
> +void BPF_STRUCT_OPS(bpf_rr_get_subflow, const struct mptcp_sock *msk,
> +		    bool reinject, struct mptcp_sched_data *data)
> +{
> +	struct mptcp_subflow_context *subflow;
> +	struct sock *ssk = msk->first;

Hi Geliang -

I suggest initializing this from data->contexts[0] to make it more obvious 
what is happening in the single-subflow case (or if last_ssk isn't found).

Now that we pass in contexts[] every time, is access to msk->first still 
needed?

> +
> +	for (int i = 0; i < MPTCP_SUBFLOWS_MAX; i++) {
> +		subflow = data->contexts[i];
> +		if (!subflow)
> +			break;
> +
> +		if (subflow->tcp_sock != msk->last_snd) {
> +			ssk = subflow->tcp_sock;
> +			break;

This will only alternate between the first two subflows in the list.

last_ssk could be any one of the values in contexts[], and round-robin 
should cycle through all the subflows (including wrapping around).

- Mat

> +		}
> +	}
> +
> +	data->sock = ssk;
> +	data->call_again = 0;
> +}
> +
> +SEC(".struct_ops")
> +struct mptcp_sched_ops rr = {
> +	.init		= (void *)mptcp_sched_rr_init,
> +	.release	= (void *)mptcp_sched_rr_release,
> +	.get_subflow	= (void *)bpf_rr_get_subflow,
> +	.name		= "bpf_rr",
> +};
> -- 
> 2.34.1
>
>
>

--
Mat Martineau
Intel