[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <a9aa6df0-b6ee-4512-acbe-7e30c98bba25@linux.dev>
Date: Wed, 1 May 2024 13:19:38 -0700
From: Martin KaFai Lau <martin.lau@...ux.dev>
To: Miao Xu <miaxu@...a.com>
Cc: Eric Dumazet <edumazet@...gle.com>, "David S . Miller"
<davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>, David Ahern <dsahern@...nel.org>,
Martin Lau <kafai@...a.com>, netdev@...r.kernel.org, bpf@...r.kernel.org
Subject: Re: [PATCH net-next v2 3/3] Add test for the use of new args in
cong_control
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" ?
Powered by blists - more mailing lists