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

[Geliang Tang <geliang@kernel.org>]

Hi Matt,

On Tue, 2024-10-15 at 12:47 +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 15/10/2024 09:59, Geliang Tang wrote:
> > 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;
> 
> I agree, but for me, I see this as a preparation step, and then it is
> fine if this test is not doing anything useful for the moment. I
> think
> it would be enough to add a comment in the code like:
> 
>   /* Here MPTCP-specific kfunc can be called */
> 
> And add in the commit message that these kfunc will be added later
> one,
> as a next step.
> 
> > > 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.
> 
> I think that can be done later on, the first step is to have these
> MPTCP
> bpf_iter upstreamed I think.
> 
> > Also, This iter is for the BPF packet scheduler, why not test the
> > actual usage of scheduler helpers in this test?
> 
> I understand that, but I think it will be easier to avoid these
> helpers
> for the moment, and make the test as simpler as possible.
> 
> (...)
> 
> > > > 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;
> 
> Good idea! Please add a comment above to explain you added that to
> only
> do the rest once.
> 
> If we want to have a simpler test without the scheduler helpers, we
> can
> also use another hook, e.g. just before closing the different
> subflows?
> Up to you.
> 
> Also, just to be sure: is this BPF program only used (loaded,
> running)
> by the corresponding test?
> Can you remind me why is there a PID check? Is it because all BPF
> programs are always loaded for all tests?
> 
> > > > +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.
> 
> Just to be clear: I'm not asking to drop it, but to understand why it
> is
> needed. The main reason is to have other people using it in future
> programs, because they saw it was used here. If it is here as an
> "assert", something that cannot be wrong, that's fine, as long as it
> is
> marked (by a comment) as is.
> 
> > > > +
> > > > +		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.
> 
> For me, that's fine to keep using these helpers, even if their usage
> is
> limited in the test. You can keep it here, and keep
> bpf_mptcp_subflow_ctx(ssk) below, then use 'subflow' below to compare
> the token with the one of the msk for example. Feel free to add a
> comment mentioning that it is just to show these helpers can be used
> in
> the 'bpf_for_each()' and outside.
> 
> > > > +	}
> > > > +	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.
> 
> I was just thinking about something you could do to keep using these
> new
> helpers: keep the ref to the last subflow, and compare with data from
> the msk, e.g.
> 
>   struct mptcp_subflow_context *subflow;
>   struct sock *sk = (struct sock *)msk;
>   struct sock *ssk = NULL;
>   int all_ids = 0;
> 
>   /* TODO: why is it needed? */

This test program is a ftrace hook, it may affect other programs, so
the pid limitation is needed. v10 has changed the test program as
"cgroup/getsockopt", no need to add this limitation any more.

>   if (bpf_get_current_pid_tgid() >> 32 != pid)
>       return 0;
> 
>   /* to do the test only once: on the client side, at the 2nd send */
>   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) {
>       /* Here MPTCP-specific kfunc can be called: this test is not
> doing
>        * anything really useful, only to verify the iteration works.
>        */
> 
>       /* assert: if not OK, something wrong on the kernel side */
>       if (subflow->token != msk->token)
>           break;
> 
>       /* to check we iterate over all subflows */
>       local_ids += subflow->subflow_id;
> 
>       /* only to check the following kfunc works */
>       ssk = bpf_mptcp_subflow_tcp_sock(subflow);
>   }
> 
>   if (!ssk)
>       goto out;
> 
>   /* assert: if not OK, something wrong on the kernel side */
>   if (ssk->sk_dport != sk->sk_dport)
>       goto out;
> 
>   /* only to check the following kfunc works */
>   subflow = bpf_mptcp_subflow_ctx(ssk);
>   if (subflow->token != msk->token)
>       goto out;

token is checked twice. I dropped the first one in v10.

Thanks,
-Geliang

> 
>   ids = local_ids;
> 
> 
> (not tested)
> 
> → So not using anything linked to the packet scheduler, just very
> simple
> check to make sure 'bpf_iter' + bpf_mptcp_subflow_tcp_sock() and
> bpf_mptcp_subflow_ctx() work as expected. Like that, it sounds easier
> to
> upstream as it is.
> 
> WDYT?
> 
> Cheers,
> Matt