Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:
Critical: 0 · High: 0 · Medium: 0 · Low: 1
- [Low] New kfunc `tcp_tso_autosize()` is registered in the TCP
congestion-control kfunc set (net/ipv4/bpf_tcp_ca.c:197) without any…
--
bpf: make tcp_tso_autosize() available to BPF congestion controls
This exposes tcp_tso_autosize() as a BPF kfunc and registers it in the TCP
congestion-control kfunc set. It also sanitizes min_tso_segs to at least 1
and returns that sanitized minimum when mss_now is 0.
> diff --git a/net/ipv4/bpf_tcp_ca.c b/net/ipv4/bpf_tcp_ca.c
> index ed4fea98dfde1..9deed2244c2de 100644
> --- a/net/ipv4/bpf_tcp_ca.c
> +++ b/net/ipv4/bpf_tcp_ca.c
> @@ -194,6 +194,7 @@ BTF_ID_FLAGS(func, tcp_reno_cong_avoid)
> BTF_ID_FLAGS(func, tcp_reno_undo_cwnd)
> BTF_ID_FLAGS(func, tcp_slow_start)
> BTF_ID_FLAGS(func, tcp_cong_avoid_ai)
> +BTF_ID_FLAGS(func, tcp_tso_autosize)
> BTF_KFUNCS_END(bpf_tcp_ca_check_kfunc_ids)
[Severity: Low]
Is there a selftest planned for this new kfunc? A grep for
tcp_tso_autosize at this commit matches only:
Documentation/networking/net_cachelines/netns_ipv4_sysctl.rst
include/net/tcp.h
net/ipv4/bpf_tcp_ca.c
net/ipv4/tcp.c
net/ipv4/tcp_bbr.c
net/ipv4/tcp_output.c
so nothing under tools/testing/selftests/bpf references it.
The other members of bpf_tcp_ca_check_kfunc_ids do have a consumer, for
example in tools/testing/selftests/bpf/progs/bpf_cubic.c:
extern __u32 tcp_slow_start(struct tcp_sock *tp, __u32 acked) __ksym;
extern void tcp_cong_avoid_ai(struct tcp_sock *tp, __u32 w, __u32 acked)
__ksym;
and the tso_segs struct_ops program in
tools/testing/selftests/bpf/progs/tcp_ca_kfunc.c still calls
bbr_tso_segs(sk, mss_now) rather than the newly exposed kfunc.
Would adding a struct_ops program that declares
extern __u32 tcp_tso_autosize(struct sock *sk, __u32 mss_now, int
min_tso_segs) __ksym;
and calls it be worthwhile, so that the load-time BTF/argument match for
the promoted kfunc is checked?
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index b1b493d46b9cf..b7f2ee5e7a6b1 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -2252,13 +2252,21 @@ static bool tcp_nagle_check(bool partial, const
> struct tcp_sock *tp,
> * in bigger TSO bursts. We we cut the RTT-based allowance in half
> * for every 2^9 usec (aka 512 us) of RTT, so that the RTT-based allowance
> * is below 1500 bytes after 6 * ~500 usec = 3ms.
> + *
> + * The min_tso_segs is floored to 1 to avoid surprising conversion. Also,
> + * BPF callers may pass mss_now == 0. In that case the function returns the
> + * sanitized min_tso_segs value and skips autosizing.
> */
> -u32 tcp_tso_autosize(const struct sock *sk, unsigned int mss_now,
> - int min_tso_segs)
> +__bpf_kfunc u32 tcp_tso_autosize(const struct sock *sk, unsigned int mss_now,
> + int min_tso_segs)
> {
> + u32 min_tso = max(min_tso_segs, 1);
> unsigned long bytes;
> u32 r;
>
> + if (unlikely(!mss_now))
> + return min_tso;
> +
[Severity: Low]
Can either of these two new branches be reached by anything in the tree
today?
tcp_tso_segs() passes sysctl_tcp_min_tso_segs, whose minimum is 1, together
with a non-zero tcp_current_mss() value, and bbr_tso_segs() already filters
out !mss_now before passing 1 or 2:
net/ipv4/tcp_bbr.c:bbr_tso_segs() {
...
if (mss_now)
segs = tcp_tso_autosize(sk, mss_now, min_tso_segs);
...
}
That leaves a BPF congestion control as the only caller that can pass
mss_now == 0 or min_tso_segs < 1, and no such program exists in
tools/testing/selftests/bpf. Would a test that exercises these two paths
be worth adding alongside the kfunc registration?
> bytes = READ_ONCE(sk->sk_pacing_rate) >> READ_ONCE(sk->sk_pacing_shift);
>
> r = tcp_min_rtt(tcp_sk(sk)) >>
> READ_ONCE(sock_net(sk)->ipv4.sysctl_tcp_tso_rtt_log);
[ ... ]
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831165230.2696893-1-chia-yu.chang%40nokia-bell-labs.com
