netdev - Re: [PATCH net-next] tcp: clarify tcp_congestion

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CADVnQy=-UP9jJ5-bv=aRYL5fEtpjscDEAC1G=_cCM4gF10W8ew@mail.gmail.com>
Date: Thu, 18 Dec 2025 08:37:08 -0500
From: Neal Cardwell <ncardwell@...gle.com>
To: Daniel Sedlak <daniel.sedlak@...77.com>
Cc: Eric Dumazet <edumazet@...gle.com>, Kuniyuki Iwashima <kuniyu@...gle.com>, 
	"David S. Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	Simon Horman <horms@...nel.org>, netdev@...r.kernel.org
Subject: Re: [PATCH net-next] tcp: clarify tcp_congestion_ops functions comments

On Thu, Dec 18, 2025 at 5:58 AM Daniel Sedlak <daniel.sedlak@...77.com> wrote:
>
> The optional/required hints in the tcp_congestion_ops are information
> for the user of this interface to signalize its importance when
> implementing these functions.
>
> However, cong_avoid comment incorrectly tells that it is required.
> For example the BBR does not implement this function, thus mark it as
> an optional.
>
> In addition, min_tso_segs has not had any comment optional/required
> hints. So mark it as optional since it is used only in BBR.
>
> Signed-off-by: Daniel Sedlak <daniel.sedlak@...77.com>
> ---
>  include/net/tcp.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index 0deb5e9dd911..a94722e4de8c 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -1246,7 +1246,7 @@ struct tcp_congestion_ops {
>         /* return slow start threshold (required) */
>         u32 (*ssthresh)(struct sock *sk);
>
> -       /* do new cwnd calculation (required) */
> +       /* do new cwnd calculation (optional) */
>         void (*cong_avoid)(struct sock *sk, u32 ack, u32 acked);

Thanks for proposing a patch to update this incorrect comment!

I agree that some kind of update here is worthwhile, since indeed
cong_avoid is not strictly required.

However, IMHO "optional" does not quite capture all the important
information/constraints here. If we're going to update this comment,
which I agree is worthwhile, then I'd suggest that we have the
comments clarify that a CC module must implement either cong_avoid or
cong_control.

Perhaps we can move cong_avoid and cong_control to the top of the
module so that they are next to each other, and provide a single
comment to explain their relationship, and when to implement one vs
implementing the other.

Perhaps something like the following.

diff --git a/include/net/tcp.h b/include/net/tcp.h
index 10706a1753e96..d35908bc977db 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -1326,12 +1326,28 @@ struct rate_sample {
 struct tcp_congestion_ops {
 /* fast path fields are put first to fill one cache line */

+       /* A congestion control (CC) must provide one of either:
+        *
+        * (a) a cong_avoid function, if the CC wants to use the core TCP
+        *     stack's default functionality to implement a "classic"
+        *     (Reno/CUBIC-style) response to packet loss, RFC3168 ECN,
+        *     idle periods, pacing rate computations, etc.
+        *
+        * (b) a cong_control function, if the CC wants custom behavior and
+        *      complete control of all congestion control behaviors
+        */
+       /* (a) "classic" response: calculate new cwnd.
+        */
+       void (*cong_avoid)(struct sock *sk, u32 ack, u32 acked);
+       /* (b) "custom" response: call when packets are delivered to update
+        * cwnd and pacing rate, after all the ca_state processing.
+        */
+       void (*cong_control)(struct sock *sk, u32 ack, int flag,
+                            const struct rate_sample *rs);
+
        /* return slow start threshold (required) */
        u32 (*ssthresh)(struct sock *sk);

-       /* do new cwnd calculation (required) */
-       void (*cong_avoid)(struct sock *sk, u32 ack, u32 acked);
-
        /* call before changing ca_state (optional) */
        void (*set_state)(struct sock *sk, u8 new_state);

@@ -1347,12 +1363,6 @@ struct tcp_congestion_ops {
        /* pick target number of segments per TSO/GSO skb (optional): */
        u32 (*tso_segs)(struct sock *sk, unsigned int mss_now);

-       /* call when packets are delivered to update cwnd and pacing rate,
-        * after all the ca_state processing. (optional)
-        */
-       void (*cong_control)(struct sock *sk, u32 ack, int flag, const
struct rate_sample *rs);
-
-
        /* new value of cwnd after loss (required) */
        u32  (*undo_cwnd)(struct sock *sk);
        /* returns the multiplier used in tcp_sndbuf_expand (optional) */

How does that sound?

>
>         /* call before changing ca_state (optional) */
> @@ -1261,7 +1261,7 @@ struct tcp_congestion_ops {
>         /* hook for packet ack accounting (optional) */
>         void (*pkts_acked)(struct sock *sk, const struct ack_sample *sample);
>
> -       /* override sysctl_tcp_min_tso_segs */
> +       /* override sysctl_tcp_min_tso_segs (optional) */

This part looks good to me as-is.

Thanks!
neal