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

[Mat Martineau <martineau@kernel.org>]

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
>
>
>