Re: [PATCH mptcp-next v18 5/7] mptcp: add bpf_mptcp_sched_ops

[Mat Martineau <mathew.j.martineau@linux.intel.com>]

On Mon, 2 May 2022, Paolo Abeni wrote:

> Hello,
>
> On Sun, 2022-05-01 at 21:48 +0800, Geliang Tang wrote:
>> This patch implements a new struct bpf_struct_ops, bpf_mptcp_sched_ops.
>> Register and unregister the bpf scheduler in .reg and .unreg.
>>
>> This MPTCP BPF scheduler implementation is similar to BPF TCP CC. And
>> net/ipv4/bpf_tcp_ca.c is a frame of reference for this patch.
>>
>> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
>> ---
>>  kernel/bpf/bpf_struct_ops_types.h |   4 +
>>  net/mptcp/bpf.c                   | 154 ++++++++++++++++++++++++++++++
>>  2 files changed, 158 insertions(+)
>>
>> diff --git a/kernel/bpf/bpf_struct_ops_types.h b/kernel/bpf/bpf_struct_ops_types.h
>> index 5678a9ddf817..5a6b0c0d8d3d 100644
>> --- a/kernel/bpf/bpf_struct_ops_types.h
>> +++ b/kernel/bpf/bpf_struct_ops_types.h
>> @@ -8,5 +8,9 @@ BPF_STRUCT_OPS_TYPE(bpf_dummy_ops)
>>  #ifdef CONFIG_INET
>>  #include <net/tcp.h>
>>  BPF_STRUCT_OPS_TYPE(tcp_congestion_ops)
>> +#ifdef CONFIG_MPTCP
>> +#include <net/mptcp.h>
>> +BPF_STRUCT_OPS_TYPE(mptcp_sched_ops)
>> +#endif
>>  #endif
>>  #endif
>> diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c
>> index 535602ba2582..ad770deb5a72 100644
>> --- a/net/mptcp/bpf.c
>> +++ b/net/mptcp/bpf.c
>> @@ -10,8 +10,162 @@
>>  #define pr_fmt(fmt) "MPTCP: " fmt
>>
>>  #include <linux/bpf.h>
>> +#include <linux/bpf_verifier.h>
>> +#include <linux/btf.h>
>> +#include <linux/btf_ids.h>
>>  #include "protocol.h"
>>
>> +#ifdef CONFIG_BPF_JIT
>> +extern struct bpf_struct_ops bpf_mptcp_sched_ops;
>> +extern struct btf *btf_vmlinux;
>> +static const struct btf_type *mptcp_sched_type __read_mostly;
>> +static u32 mptcp_sched_id;
>> +
>> +static u32 optional_ops[] = {
>> +	offsetof(struct mptcp_sched_ops, init),
>> +	offsetof(struct mptcp_sched_ops, release),
>> +	offsetof(struct mptcp_sched_ops, get_subflow),
>
> I think that the 'get_subflow' member should not be options. Otherwise
> the scheduler will useless, right ?!?
>
> Otherwise LGTM.
>

I agree - for this series (with the squashed get_subflow fix), looks good:

Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>

--
Mat Martineau
Intel