Re: [PATCH mptcp-next v9 4/7] selftests/bpf: Add mptcp_subflow bpf_iter test prog

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 09/10/2024 11:45, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> This patch adds a ftrace hook for mptcp_sched_get_send() to test the newly
> added mptcp_subflow bpf_iter. This test simulates a typical mptcp packet
> scheduler, which selects a subflow from multiple subflows of an mptcp
> socket to send data.
> 
> Export mptcp_subflow helpers bpf_iter_mptcp_subflow_new/_next/_destroy,
> bpf_mptcp_sock_acquire/_release and other helpers into bpf_experimental.h.
> 
> Use _acquire() to acquire the msk, then use bpf_for_each(mptcp_subflow) to
> walk the subflow list of this msk. Invoke kfuncs mptcp_subflow_active() and
> bpf_mptcp_subflow_tcp_sock() in the loop to pick a subsocket. Finally use
> bpf_mptcp_subflow_ctx() to get the subflow context of this subsocket and
> use mptcp_subflow_set_scheduled() to set it as being scheduled.

Do you think we could have a test not depending on scheduler helpers?
Without this dependence, we could already upstream this series, and get
feedback without having to wait for the new scheduler API.

If you remove the use of mptcp_subflow_active() and
mptcp_subflow_set_scheduled(), that's enough, no?

> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  .../testing/selftests/bpf/bpf_experimental.h  |  7 ++++
>  tools/testing/selftests/bpf/progs/mptcp_bpf.h |  9 ++++
>  .../bpf/progs/mptcp_bpf_iters_subflow.c       | 42 +++++++++++++++++++
>  3 files changed, 58 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/progs/mptcp_bpf_iters_subflow.c
> 
> diff --git a/tools/testing/selftests/bpf/bpf_experimental.h b/tools/testing/selftests/bpf/bpf_experimental.h
> index b0668f29f7b3..d43690b17468 100644
> --- a/tools/testing/selftests/bpf/bpf_experimental.h
> +++ b/tools/testing/selftests/bpf/bpf_experimental.h
> @@ -575,6 +575,13 @@ extern int bpf_iter_css_new(struct bpf_iter_css *it,
>  extern struct cgroup_subsys_state *bpf_iter_css_next(struct bpf_iter_css *it) __weak __ksym;
>  extern void bpf_iter_css_destroy(struct bpf_iter_css *it) __weak __ksym;
>  
> +struct bpf_iter_mptcp_subflow;
> +extern int bpf_iter_mptcp_subflow_new(struct bpf_iter_mptcp_subflow *it,
> +				      struct mptcp_sock *msk) __weak __ksym;
> +extern struct mptcp_subflow_context *
> +bpf_iter_mptcp_subflow_next(struct bpf_iter_mptcp_subflow *it) __weak __ksym;
> +extern void bpf_iter_mptcp_subflow_destroy(struct bpf_iter_mptcp_subflow *it) __weak __ksym;
> +
>  extern int bpf_wq_init(struct bpf_wq *wq, void *p__map, unsigned int flags) __weak __ksym;
>  extern int bpf_wq_start(struct bpf_wq *wq, unsigned int flags) __weak __ksym;
>  extern int bpf_wq_set_callback_impl(struct bpf_wq *wq,
> diff --git a/tools/testing/selftests/bpf/progs/mptcp_bpf.h b/tools/testing/selftests/bpf/progs/mptcp_bpf.h
> index c3800f986ae1..e18796361394 100644
> --- a/tools/testing/selftests/bpf/progs/mptcp_bpf.h
> +++ b/tools/testing/selftests/bpf/progs/mptcp_bpf.h
> @@ -43,9 +43,18 @@ mptcp_subflow_tcp_sock(const struct mptcp_subflow_context *subflow)
>  }
>  
>  /* ksym */
> +extern bool mptcp_subflow_active(struct mptcp_subflow_context *subflow) __ksym;
>  extern void mptcp_subflow_set_scheduled(struct mptcp_subflow_context *subflow,
>  					bool scheduled) __ksym;
>  
> +extern struct mptcp_sock *bpf_mptcp_sock_acquire(struct mptcp_sock *msk) __ksym;
> +extern void bpf_mptcp_sock_release(struct mptcp_sock *msk) __ksym;
> +
> +extern struct mptcp_subflow_context *
> +bpf_mptcp_subflow_ctx(const struct sock *sk) __ksym;
> +extern struct sock *
> +bpf_mptcp_subflow_tcp_sock(const struct mptcp_subflow_context *subflow) __ksym;
> +
>  extern struct mptcp_subflow_context *
>  bpf_mptcp_subflow_ctx_by_pos(const struct mptcp_sched_data *data, unsigned int pos) __ksym;
>  
> diff --git a/tools/testing/selftests/bpf/progs/mptcp_bpf_iters_subflow.c b/tools/testing/selftests/bpf/progs/mptcp_bpf_iters_subflow.c
> new file mode 100644
> index 000000000000..4268e4604c5a
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/mptcp_bpf_iters_subflow.c
> @@ -0,0 +1,42 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2024, Kylin Software */
> +
> +/* vmlinux.h, bpf_helpers.h and other 'define' */
> +#include "bpf_tracing_net.h"
> +#include "mptcp_bpf.h"
> +
> +char _license[] SEC("license") = "GPL";
> +int subflows;
> +int pid;
> +
> +SEC("fentry/mptcp_sched_get_send")

If I understand correctly, this hook will be called twice on the client
connection for this test because the client is sending two times one
byte, right?

> +int BPF_PROG(trace_mptcp_sched_get_send, struct mptcp_sock *msk)
> +{
> +	struct mptcp_subflow_context *subflow;
> +	struct sock *ssk = NULL;
> +
> +	if (bpf_get_current_pid_tgid() >> 32 != pid)
> +		return 0;
> +
> +	msk = bpf_mptcp_sock_acquire(msk);
> +	if (!msk)
> +		return 0;
> +	bpf_for_each(mptcp_subflow, subflow, msk) {
> +		if (subflow->token != msk->token)
> +			break;

Out of curiosity, why is this needed? Is it needed for the verifier? Or
just an extra check?
Can you add a comment here explaining why this is there please?

> +
> +		if (!mptcp_subflow_active(subflow))
> +			continue;
> +
> +		ssk = bpf_mptcp_subflow_tcp_sock(subflow);

Do you get 'ssk' just to use bpf_mptcp_subflow_tcp_sock() and
bpf_mptcp_subflow_ctx()?

> +	}
> +	bpf_mptcp_sock_release(msk);
> +
> +	if (!ssk)
> +		return 0;
> +	subflow = bpf_mptcp_subflow_ctx(ssk);
> +	mptcp_subflow_set_scheduled(subflow, true);
> +	subflows = subflow->subflow_id;

Here, it looks like you only check if the last subflow is in the list.

Would it not be better to count the number of subflows in the list? Or
if you want to read something from 'subflow', you could add all subflow_id?

  subflows = 0;

  (...)

  bpf_for_each(mptcp_subflow, subflow, msk)
      subflows += subflow->subflow_id;

By doing that, we will catch if one is missing, and the order is not
important.

If still want to use bpf_mptcp_subflow_tcp_sock() and
bpf_mptcp_subflow_ctx(), maybe you can save other fields from the last
subflow: the token to compare it with the one in the msk? Or the port
number or the address?

> +
> +	return 0;
> +}

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.