Re: [PATCH mptcp-next v7 2/5] Squash to "selftests/bpf: Add bpf_rr scheduler & test"

[Geliang Tang <geliang@kernel.org>]

Hi Mat,

Thanks for your review.

On Tue, 2024-10-22 at 16:52 -0700, Mat Martineau wrote:
> On Tue, 22 Oct 2024, Geliang Tang wrote:
> 
> > From: Geliang Tang <tanggeliang@kylinos.cn>
> > 
> > Use the newly added bpf_for_each() helper to walk the conn_list.
> > 
> > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> > ---
> > .../selftests/bpf/progs/mptcp_bpf_rr.c        | 26 ++++++++--------
> > ---
> > 1 file changed, 11 insertions(+), 15 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/bpf/progs/mptcp_bpf_rr.c
> > b/tools/testing/selftests/bpf/progs/mptcp_bpf_rr.c
> > index 638ea6aa63b7..e02b8cfb262b 100644
> > --- a/tools/testing/selftests/bpf/progs/mptcp_bpf_rr.c
> > +++ b/tools/testing/selftests/bpf/progs/mptcp_bpf_rr.c
> > @@ -34,10 +34,9 @@ SEC("struct_ops")
> > int BPF_PROG(bpf_rr_get_subflow, struct mptcp_sock *msk,
> > 	     struct mptcp_sched_data *data)
> > {
> > -	struct mptcp_subflow_context *subflow;
> > +	struct mptcp_subflow_context *subflow, *next;
> > 	struct mptcp_rr_storage *ptr;
> > 	struct sock *last_snd = NULL;
> > -	int nr = 0;
> > 
> > 	ptr = bpf_sk_storage_get(&mptcp_rr_map, msk, 0,
> > 				 BPF_LOCAL_STORAGE_GET_F_CREATE);
> 
> Hi Geliang -
> 
> Thanks for the series. The bpf_iter code simplifies these scheduler 
> patches significantly!
> 
> > @@ -45,31 +44,28 @@ int BPF_PROG(bpf_rr_get_subflow, struct
> > mptcp_sock *msk,
> > 		return -1;
> > 
> > 	last_snd = ptr->last_snd;
> > +	next = bpf_mptcp_subflow_ctx(msk->first);
> > 
> > -	for (int i = 0; i < data->subflows && i <
> > MPTCP_SUBFLOWS_MAX; i++) {
> > -		subflow = bpf_mptcp_subflow_ctx_by_pos(data, i);
> > -		if (!last_snd || !subflow)
> > +	bpf_for_each(mptcp_subflow, subflow, msk) {
> > +		if (!last_snd)
> 
> last_snd is never changed inside the loop, it's more efficient to
> check it 
> before bpf_for_each().

This is indeed better, updated in v8.

> 
> - Mat
> 
> > 			break;
> > 
> > -		if (mptcp_subflow_tcp_sock(subflow) == last_snd) {
> > -			if (i + 1 == MPTCP_SUBFLOWS_MAX ||
> > -			    !bpf_mptcp_subflow_ctx_by_pos(data, i
> > + 1))
> > +		if (bpf_mptcp_subflow_tcp_sock(subflow) ==
> > last_snd) {
> > +			subflow =
> > bpf_iter_mptcp_subflow_next(&___it);
> > +			if (!subflow)
> > 				break;
> > 
> > -			nr = i + 1;
> > +			next = subflow;
> > 			break;
> > 		}
> > 	}
> > 
> > -	subflow = bpf_mptcp_subflow_ctx_by_pos(data, nr);
> > -	if (!subflow)
> > -		return -1;
> > -	mptcp_subflow_set_scheduled(subflow, true);
> > -	ptr->last_snd = mptcp_subflow_tcp_sock(subflow);
> > +	mptcp_subflow_set_scheduled(next, true);
> > +	ptr->last_snd = bpf_mptcp_subflow_tcp_sock(next);
> > 	return 0;
> > }
> > 
> > -SEC(".struct_ops")
> > +SEC(".struct_ops.link")
> > struct mptcp_sched_ops rr = {
> > 	.init		= (void *)mptcp_sched_rr_init,
> > 	.release	= (void *)mptcp_sched_rr_release,
> > -- 
> > 2.45.2
> > 
> > 
> >