Re: [RFC mptcp-next 00/11] BPF packet scheduler updates part 4

[Geliang Tang <geliang.tang@suse.com>]

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