There's a bug in bpf_burst. snd_burst stored in mptcp_burst_storage in
BPF context is not used. msk->snd_burst is still used in kernel space.
To fix this, add two new interfaces in mptcp_sched_ops to get and set
scheduler's paramters from BPF context to kernel space.

Geliang Tang (11):
  Squash to "mptcp: add struct mptcp_sched_ops"
  Squash to "mptcp: add scheduler wrappers"
  mptcp: use snd_burst wrappers
  Squash to "mptcp: register default scheduler"
  Squash to "bpf: Add bpf_mptcp_sched_ops"
  Squash to "selftests/bpf: Add mptcp sched structs"
  Squash to "selftests/bpf: Add bpf_bkup scheduler"
  Squash to "selftests/bpf: Add bpf_rr scheduler"
  Squash to "selftests/bpf: Add bpf_burst scheduler"
  selftests/bpf: Add bpf_stale scheduler
  selftests/bpf: Add bpf_stale test

 include/net/mptcp.h                           |  10 ++
 net/mptcp/bpf.c                               |  27 ++-
 net/mptcp/protocol.c                          |   8 +-
 net/mptcp/protocol.h                          |   2 +
 net/mptcp/sched.c                             |  49 ++++++
 tools/testing/selftests/bpf/bpf_tcp_helpers.h |  11 ++
 .../testing/selftests/bpf/prog_tests/mptcp.c  |  38 ++++
 .../selftests/bpf/progs/mptcp_bpf_bkup.c      |   4 +-
 .../selftests/bpf/progs/mptcp_bpf_burst.c     |  43 ++++-
 .../selftests/bpf/progs/mptcp_bpf_rr.c        |   2 +
 .../selftests/bpf/progs/mptcp_bpf_stale.c     | 163 ++++++++++++++++++
 11 files changed, 339 insertions(+), 18 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/progs/mptcp_bpf_stale.c

-- 
2.35.3

Hi Geliang,

Thank you for these patches!

On 10/08/2023 16:09, Geliang Tang wrote:
> There's a bug in bpf_burst. snd_burst stored in mptcp_burst_storage in
> BPF context is not used. msk->snd_burst is still used in kernel space.
> To fix this, add two new interfaces in mptcp_sched_ops to get and set
> scheduler's paramters from BPF context to kernel space.

I didn't fully look at this series in details (and I don't have the full
picture in mind) but I'm a bit worry about these parameters. I see two
ways to extend the BPF packet schedulers:

(1) either we keep the API as it is and we parametrise actions like here

(2) or the new schedulers have more control on what is being done
replacing what the core is doing to select what can be sent and where

I think that if we take the first option, we would limit the extensions:
when a new scheduler will want to change what the core is doing, a new
parameter will need to be added with more indirections, complexity, etc.

The second option requires some "big" changes in the scheduler API but
it is probably the right time to do that and probably worth it. (but
before modifying the code, we should probably discuss high level
architecture). With this new API, BPF scheduler should be able to only
modify the parts they want, leaving the rest like it is when the default
scheduler is running (or use helpers to keep the behaviour of the core).

This is also somehow linked to issues #343 and #344 on GitHub.

> Geliang Tang (11):
>   Squash to "mptcp: add struct mptcp_sched_ops"
>   Squash to "mptcp: add scheduler wrappers"
>   mptcp: use snd_burst wrappers
>   Squash to "mptcp: register default scheduler"
In other words, I think we should not modify the patches above and send
them as their are to netdev. Then think about the strategy we want  to
allow "any kind" of packet schedulers implemented in BPF.

WDYT?

>   Squash to "bpf: Add bpf_mptcp_sched_ops"
>   Squash to "selftests/bpf: Add mptcp sched structs"
>   Squash to "selftests/bpf: Add bpf_bkup scheduler"
>   Squash to "selftests/bpf: Add bpf_rr scheduler"
>   Squash to "selftests/bpf: Add bpf_burst scheduler"
>   selftests/bpf: Add bpf_stale scheduler
>   selftests/bpf: Add bpf_stale test

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

On Thu, Aug 10, 2023 at 05:50:05PM +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> Thank you for these patches!
> 
> On 10/08/2023 16:09, Geliang Tang wrote:
> > There's a bug in bpf_burst. snd_burst stored in mptcp_burst_storage in
> > BPF context is not used. msk->snd_burst is still used in kernel space.
> > To fix this, add two new interfaces in mptcp_sched_ops to get and set
> > scheduler's paramters from BPF context to kernel space.
> 
> I didn't fully look at this series in details (and I don't have the full
> picture in mind) but I'm a bit worry about these parameters. I see two
> ways to extend the BPF packet schedulers:
> 
> (1) either we keep the API as it is and we parametrise actions like here
> 
> (2) or the new schedulers have more control on what is being done
> replacing what the core is doing to select what can be sent and where
> 
> I think that if we take the first option, we would limit the extensions:
> when a new scheduler will want to change what the core is doing, a new
> parameter will need to be added with more indirections, complexity, etc.
> 
> The second option requires some "big" changes in the scheduler API but
> it is probably the right time to do that and probably worth it. (but
> before modifying the code, we should probably discuss high level
> architecture). With this new API, BPF scheduler should be able to only
> modify the parts they want, leaving the rest like it is when the default
> scheduler is running (or use helpers to keep the behaviour of the core).
> 
> This is also somehow linked to issues #343 and #344 on GitHub.
> 
> > Geliang Tang (11):
> >   Squash to "mptcp: add struct mptcp_sched_ops"
> >   Squash to "mptcp: add scheduler wrappers"
> >   mptcp: use snd_burst wrappers
> >   Squash to "mptcp: register default scheduler"
> In other words, I think we should not modify the patches above and send
> them as their are to netdev. Then think about the strategy we want  to
> allow "any kind" of packet schedulers implemented in BPF.
> 
> WDYT?

I agree, let's deal with them after the scheduler refactor patches are
merged. Some patches below are still valid. They address to Mat's
comments in the series "add bpf_stale scheduler" v2. I'll repost them
as a v3 this week.

Thanks,
-Geliang

> 
> >   Squash to "bpf: Add bpf_mptcp_sched_ops"
> >   Squash to "selftests/bpf: Add mptcp sched structs"
> >   Squash to "selftests/bpf: Add bpf_bkup scheduler"
> >   Squash to "selftests/bpf: Add bpf_rr scheduler"
> >   Squash to "selftests/bpf: Add bpf_burst scheduler"
> >   selftests/bpf: Add bpf_stale scheduler
> >   selftests/bpf: Add bpf_stale test
> 
> Cheers,
> Matt
> -- 
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net