1. Patchew
  2. MPTCP
  3. [PATCH mptcp-next 0/8] split get_subflow interface into two
  4. View patch

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

Geliang Tang posted 8 patches 1 year, 2 months ago
There is a newer version of this series
[PATCH mptcp-next 2/8] Squash to "selftests/bpf: Add bpf_first scheduler & test"
Posted by Geliang Tang 1 year, 2 months ago 
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(data, 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);
+}
+
+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,
 	.name		= "bpf_first",
 };
-- 
2.43.0
Re: [PATCH mptcp-next 2/8] Squash to "selftests/bpf: Add bpf_first scheduler & test"
Posted by Mat Martineau 1 year, 1 month ago 
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(data, 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 
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,

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
>
>
>
Re: [PATCH mptcp-next 2/8] Squash to "selftests/bpf: Add bpf_first scheduler & test"
Posted by Geliang Tang 1 year, 1 month ago 
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
> > 
> > 
> >

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.