On Sun, Oct 18, 2026 at 5:15 PM <[email protected]> wrote: > > From: Chia-Yu Chang <[email protected]> > > When connection is initialized after ECN negotiation, this patch > enables CA to control whether ECT(0) or ECT(1) should be used.
AFAICT this should be "When congestion control is initialized" rather than 'When connection is initialized". > A new identiifer (TCP_CONG_ECT_1_ESTABLISH) defines the expected > ECT value in the IP header. Using "expected" here is not as clear as it could be. It sounds like the expected incoming value rather than a value that the CC algorithm is specifying should be sent. Also, the name TCP_CONG_ECT_1_ESTABLISH is quite verbose, and not 100% accurate AFAICT, since it is used in tcp_ecn_send(), which is called when the SYN bit is not set, which can be in a number of TCP states other than ESTABLISHED. I would suggest calling this something shorter and more clear and correct, like TCP_ECN_SEND_ECT_1. Putting that all together, how about something like: """ This patch introduces the new TCP_ECN_SEND_ECT_1 bit, which enables a congestion control module to control whether the connection transmits using ECT(1) (if the bit is set) or ECT(0) (otherwise), when congestion control is initialized after ECN negotiation. A congestion control module, such as the upcoming TCP Prague using L4S ECN, can set this new TCP_ECN_SEND_ECT_1 bit if it wants to send the ECT(1) code point in outgoing IP headers. """ > Signed-off-by: Chia-Yu Chang <[email protected]> > --- > include/net/tcp.h | 1 + > include/net/tcp_ecn.h | 6 ++++++ > net/ipv4/tcp_cong.c | 4 +++- > net/ipv4/tcp_output.c | 2 +- > 4 files changed, 11 insertions(+), 2 deletions(-) > > diff --git a/include/net/tcp.h b/include/net/tcp.h > index 1838389a9d8a..b9def462479d 100644 > --- a/include/net/tcp.h > +++ b/include/net/tcp.h > @@ -428,6 +428,7 @@ static inline void tcp_dec_quickack_mode(struct sock *sk) > #define TCP_ECN_DEMAND_CWR BIT(2) > #define TCP_ECN_SEEN BIT(3) > #define TCP_ECN_MODE_ACCECN BIT(4) > +#define TCP_ECN_ECT_1_ESTABLISH BIT(5) > > #define TCP_ECN_DISABLED 0 > #define TCP_ECN_MODE_PENDING (TCP_ECN_MODE_RFC3168 | > TCP_ECN_MODE_ACCECN) > diff --git a/include/net/tcp_ecn.h b/include/net/tcp_ecn.h > index 865d5c5a7718..210ec63538f8 100644 > --- a/include/net/tcp_ecn.h > +++ b/include/net/tcp_ecn.h > @@ -38,6 +38,12 @@ static inline void INET_ECN_xmit_ect_1_negotiation(struct > sock *sk) > __INET_ECN_xmit(sk, tcp_ca_ect_1_negotiation(sk)); > } > > +/* Apply either ECT(0) or ECT(1) based on TCP_CONG_ECT_1_ESTABLISH flag */ > +static inline void INET_ECN_xmit_ect_1_establish(struct sock *sk) > +{ > + __INET_ECN_xmit(sk, tcp_sk(sk)->ecn_flags & TCP_ECN_ECT_1_ESTABLISH); > +} This function name is quite verbose, and a little confusing, since it may or may not set the ECT code point to ECT(1). I realize the name is chosen to be consistent with INET_ECN_xmit_ect_1_negotiation(). But that one is confusing in the same way. :-) Also, using "establish" in this way sounds like a verb, making it sound like this function is establishing something. How about something like: (1) a new preceding patch in the series can rename INET_ECN_xmit_ect_1_negotiation() to INET_ECN_xmit_negotiation_ect() (2) this patch can rename its INET_ECN_xmit_ect_1_establish to INET_ECN_xmit_established_ect() Thanks for the patch! neal
