Re: [PATCH mptcp-next 2/8] Squash to "selftests/bpf: Add bpf_first scheduler & test"

[Geliang Tang <geliang@kernel.org>]

Hi Mat,

Thanks for your review.

On Fri, 2024-12-20 at 17:28 -0800, Mat Martineau wrote:
> On Thu, 21 Nov 2024, Geliang Tang wrote:
> 
> > From: Geliang Tang <tanggeliang@kylinos.cn>
> > 
> > Split get_subflow() interface into two: get_send() and
> > get_retrans().
> > 
> > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> > ---
> > .../selftests/bpf/progs/mptcp_bpf_first.c     | 24 +++++++++++++++-
> > ---
> > 1 file changed, 19 insertions(+), 5 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/bpf/progs/mptcp_bpf_first.c
> > b/tools/testing/selftests/bpf/progs/mptcp_bpf_first.c
> > index d57399b407a7..36bf9adffb2c 100644
> > --- a/tools/testing/selftests/bpf/progs/mptcp_bpf_first.c
> > +++ b/tools/testing/selftests/bpf/progs/mptcp_bpf_first.c
> > @@ -16,18 +16,32 @@ void BPF_PROG(mptcp_sched_first_release, struct
> > mptcp_sock *msk)
> > {
> > }
> > 
> > -SEC("struct_ops")
> > -int BPF_PROG(bpf_first_get_subflow, struct mptcp_sock *msk,
> > -	     struct mptcp_sched_data *data)
> > +static int bpf_first_get_subflow(struct mptcp_sock *msk,
> > +				 struct mptcp_sched_data *data)
> > {
> > 	mptcp_subflow_set_scheduled(bpf_mptcp_subflow_ctx_by_pos(d
> > ata, 0), true);
> > 	return 0;
> > }
> > 
> > -SEC(".struct_ops")
> > +SEC("struct_ops")
> > +int BPF_PROG(bpf_first_get_send, struct mptcp_sock *msk,
> > +	     struct mptcp_sched_data *data)
> > +{
> > +	return bpf_first_get_subflow(msk, data);
> > +}
> > +
> > +SEC("struct_ops")
> > +int BPF_PROG(bpf_first_get_retrans, struct mptcp_sock *msk,
> > +	     struct mptcp_sched_data *data)
> > +{
> > +	return bpf_first_get_subflow(msk, data);
> > +}
> 
> Hi Geliang -
> 
> If my suggestion in patch 1 (use get_send for retransmit scheduling
> if 
> get_retrans is NULL) doesn't seem like clear enough behavior for BPF

The suggestion in patch 1 works well, I updated it in v2.

>  
> scheduler authors, this code and the following patches could be
> simplified 
> by removing the duplicate _retrans function above...
> 
> > +
> > +SEC(".struct_ops.link")
> > struct mptcp_sched_ops first = {
> > 	.init		= (void *)mptcp_sched_first_init,
> > 	.release	= (void *)mptcp_sched_first_release,
> > -	.get_subflow	= (void *)bpf_first_get_subflow,
> > +	.get_send	= (void *)bpf_first_get_send,
> > +	.get_retrans	= (void *)bpf_first_get_retrans,
> 
> ... and setting get_retrans like this:
>  	.get_retrans	= (void *)bpf_first_get_send,

If we do this, libbpf will report the following error:

# libbpf: struct_ops init_kern first func ptr get_retrans: invalid
reuse of prog bpf_first_get_send in sec struct_ops with type 27:
expected_attach_type 0 != kern_member_idx 1
# libbpf: failed to load object 'mptcp_bpf_first'
# libbpf: failed to load BPF skeleton 'mptcp_bpf_first': -EINVAL
# test_first:FAIL:open_and_load: first unexpected error: -22

So in v2, I didn't set this get_retrans ptr, but let it be NULL.

Thanks,
-Geliang

> 
> Then there would be example code for BPF scheduler programmers to see
> that 
> it's ok to use the same function pointer in both fields.
> 
> - Mat
> 
> > 	.name		= "bpf_first",
> > };
> > -- 
> > 2.43.0
> > 
> > 
> >