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

[Matthieu Baerts <matttbe@kernel.org>]

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? */
  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;

  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
-- 
Sponsored by the NGI0 Core fund.