Re: [PATCH net-next v2 3/3] Add test for the use of new args in cong_control

[Martin KaFai Lau <martin.lau@linux.dev>]

On 5/1/24 12:43 AM, Miao Xu wrote:
> This patch adds a selftest to show the usage of the new arguments in
> cong_control. For simplicity's sake, the testing example reuses cubic's
> kernel functions.

Jakub, is it ok to target the set for the bpf-next?

The bpf_tcp_ca test failed (Jakub also mentioned). The progs/tcp_ca_kfunc.c 
requires changes. The func signature of bbr_main and the BPF_PROG(cong_control, 
...) has to be adjusted.

Since it needs a respin, a few nits.

Please add "selftests/bpf:" to the subject line of this patch 3. I think Patch 1 
can use "tcp:" and patch 2 can use "bpf: tcp:" also.

Please also add a cover letter, git format-patch --cover-letter ...

[ ... ]

> +void BPF_STRUCT_OPS(bpf_cubic_cong_control, struct sock *sk, __u32 ack, int flag,
> +		const struct rate_sample *rs)
> +{
> +	struct tcp_sock *tp = tcp_sk(sk);
> +
> +	if (((1<<TCP_CA_CWR) | (1<<TCP_CA_Recovery)) &
> +			(1 << inet_csk(sk)->icsk_ca_state)) {
> +		/* Reduce cwnd if state mandates */
> +		tcp_cwnd_reduction(sk, rs->acked_sacked, rs->losses, flag);
> +
> +		if (!before(tp->snd_una, tp->high_seq)) {
> +			/* Reset cwnd to ssthresh in CWR or Recovery (unless it's undone) */
> +			if (tp->snd_ssthresh < TCP_INFINITE_SSTHRESH &&
> +					inet_csk(sk)->icsk_ca_state == TCP_CA_CWR) {
> +				tp->snd_cwnd = tp->snd_ssthresh;
> +				tp->snd_cwnd_stamp = tcp_jiffies32;
> +			}
> +			// __cwnd_event(sk, CA_EVENT_COMPLETE_CWR);

Remove the commented out code.

> +		}
> +	} else if (tcp_may_raise_cwnd(sk, flag)) {
> +		/* Advance cwnd if state allows */
> +		cubictcp_cong_avoid(sk, ack, rs->acked_sacked);
> +		tp->snd_cwnd_stamp = tcp_jiffies32;
> +	}
> +
> +	tcp_update_pacing_rate(sk);
> +}
> +
> +__u32 BPF_STRUCT_OPS(bpf_cubic_recalc_ssthresh, struct sock *sk)
> +{
> +	return cubictcp_recalc_ssthresh(sk);
> +}
> +
> +void BPF_STRUCT_OPS(bpf_cubic_state, struct sock *sk, __u8 new_state)
> +{
> +	cubictcp_state(sk, new_state);
> +}
> +
> +void BPF_STRUCT_OPS(bpf_cubic_acked, struct sock *sk,
> +		const struct ack_sample *sample)
> +{
> +	cubictcp_acked(sk, sample);
> +}
> +
> +__u32 BPF_STRUCT_OPS(bpf_cubic_undo_cwnd, struct sock *sk)
> +{
> +	return tcp_reno_undo_cwnd(sk);
> +}
> +
> +
> +SEC(".struct_ops")
> +struct tcp_congestion_ops cubic = {
> +	.init		= (void *)bpf_cubic_init,
> +	.ssthresh	= (void *)bpf_cubic_recalc_ssthresh,
> +	.cong_control	= (void *)bpf_cubic_cong_control,
> +	.set_state	= (void *)bpf_cubic_state,
> +	.undo_cwnd	= (void *)bpf_cubic_undo_cwnd,
> +	.cwnd_event	= (void *)bpf_cubic_cwnd_event,
> +	.pkts_acked     = (void *)bpf_cubic_acked,
> +	.name		= "bpf_cubic",

nit. It has the same name as the tcp-cc in bpf_cubic.c. Rename it to 
"bpf_cc_cubic" ?