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

[Geliang Tang <geliang@kernel.org>]

On Mon, 2024-10-14 at 18:06 +0200, Matthieu Baerts wrote:
> 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.

This set can be upstream as is, no need to wait for the new scheduler
API.

If no scheduler helpers are used in this test, no need to add this
mptcp_subflow bpf_iter at all. mptcp_for_each_subflow() helper in
progs/mptcp_bpf.h can do that.

mptcp_for_each_subflow(msk, subflow) {
	subflow = bpf_core_cast(subflow, struct
mptcp_subflow_context);
	subflows += subflow->subflow_id;
}

No need to use this:

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

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

But "subflow" in mptcp_for_each_subflow() loop can't be passed to a
kernel function:

mptcp_for_each_subflow(msk, subflow) {
	subflow = bpf_core_cast(subflow, struct
mptcp_subflow_context);
	mptcp_subflow_active(subflows);
}

This is not allowed by BPF.

So we add this iter to do this:

bpf_for_each(mptcp_subflow, subflow, msk)
	kfunc(subflow);

In this test, I must pick some kfuncs accepted "subflow" argument to do
this test, so mptcp_subflow_active and mptcp_subflow_set_scheduled are
picked.

Also, This iter is for the BPF packet scheduler, why not test the
actual usage of scheduler helpers in this test?

> 
> > 
> > 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?

Yes, in the first call, the subflows are not been added yet, so it is
called twice. I will add this check to skip the first call:

       if (msk->pm.server_side || !msk->pm.subflows)
               return 0;

> 
> > +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?

I'll drop it.

> 
> > +
> > +		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()?

Yes, almost every BPF scheduler use these helpers, so test them here.

> 
> > +	}
> > +	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.

Yes, "subflows += subflow->subflow_id" is much better.

> 
> 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?

Sorry, I don't fully understand your last paragraph. But I stored the
dport number in the code below. Is it the same as what you thought? If
so, I'll send a squash-to patch for it.

int pid;
int ids;

SEC("fentry/mptcp_sched_get_send")
int BPF_PROG(trace_mptcp_sched_get_send, struct mptcp_sock *msk)
{
        struct mptcp_subflow_context *subflow;
        struct sock *sk = (struct sock *)msk;
        struct sock *ssk = NULL;
        int subflows = 0;
        __be16 dport;

        if (bpf_get_current_pid_tgid() >> 32 != pid)
                return 0;

        if (msk->pm.server_side || !msk->pm.subflows)
                return 0;

        msk = bpf_mptcp_sock_acquire(msk);
        if (!msk)
                return 0;
        bpf_for_each(mptcp_subflow, subflow, msk) {
                if (!mptcp_subflow_active(subflow))
                        continue;

                subflows += subflow->subflow_id;
                ssk = bpf_mptcp_subflow_tcp_sock(subflow);
                dport = ssk->sk_dport;
        }

        if (!ssk)
                goto out;
        subflow = bpf_mptcp_subflow_ctx(ssk);
        if (dport != ssk->sk_dport || dport != sk->sk_dport)
                goto out;
        mptcp_subflow_set_scheduled(subflow, true);
        ids = subflows;

out:
        bpf_mptcp_sock_release(msk);
        return 0;
}

Thanks,
-Geliang

> 
> > +
> > +	return 0;
> > +}
> 
> Cheers,
> Matt