On Sun, Oct 18, 2026 at 5:15 PM <[email protected]> wrote: > > From: Ilpo Järvinen <[email protected]>
Thanks for the patch. I agree this will be useful for congestion control modules using L4S ECN. For example, I see this field is used by Prague in https://github.com/L4STeam/linux/blob/testing/net/ipv4/tcp_prague.c .... is that the current URL for the code? > Include echoed CE count into rate_sample. Replace local ecn_count > variable with it. I'd suggest a clearer commit message, like: Include in rate_sample a count of the number of segments that were newly marked, on this ACK, as having been CE (congestion experienced). Replace local ecn_count variable with it. This signal will be useful, in upcoming patches, for congestion control modules, like Prague, which want to process AccECN-style ECN signals. > > Co-developed-by: Olivier Tilmans <[email protected]> > Signed-off-by: Olivier Tilmans <[email protected]> > Signed-off-by: Ilpo Järvinen <[email protected]> > Signed-off-by: Chia-Yu Chang <[email protected]> > --- > include/net/tcp.h | 1 + > net/ipv4/tcp_input.c | 41 +++++++++++++++++++---------------------- > 2 files changed, 20 insertions(+), 22 deletions(-) > > diff --git a/include/net/tcp.h b/include/net/tcp.h > index 0265c27a5fd7..1838389a9d8a 100644 > --- a/include/net/tcp.h > +++ b/include/net/tcp.h > @@ -1346,6 +1346,7 @@ struct rate_sample { > int losses; /* number of packets marked lost upon ACK */ > u32 acked_sacked; /* number of packets newly (S)ACKed upon ACK > */ > u32 prior_in_flight; /* in flight before this ACK */ > + u32 ece_delta; /* is this ACK echoing some received CE? */ This comment seems incorrect. It implies the value is a boolean, but it's a count. I'd suggest renaming the field for clarity, to something like acked_sacked_ece, since AFAIK it is the subset of the packets in acked_sacked that were marked as CE. So, for clarity/readability, I'd suggest renaming it, and placing the field immediately below acked_sacked, and reworking the comment to be like: u32 acked_sacked; /* number of packets newly (S)ACKed upon ACK */ u32 acked_sacked_ece; /* number of packets ECE-marked upon ACK */ > u32 last_end_seq; /* end_seq of most recently ACKed packet */ > bool is_app_limited; /* is sample from packet with bubble in pipe? > */ > bool is_retrans; /* is sample from retransmission? */ > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c > index 02ec0b0116d1..a5a7302050c4 100644 > --- a/net/ipv4/tcp_input.c > +++ b/net/ipv4/tcp_input.c > @@ -588,9 +588,10 @@ static u32 __tcp_accecn_process(struct sock *sk, const > struct sk_buff *skb, > return safe_delta; > } > > -static u32 tcp_accecn_process(struct sock *sk, const struct sk_buff *skb, > - u32 delivered_pkts, u32 delivered_bytes, > - u64 prior_bytes_acked, int *flag) > +static void tcp_accecn_process(struct sock *sk, struct rate_sample *rs, > + const struct sk_buff *skb, u32 delivered_pkts, > + u32 delivered_bytes, u64 prior_bytes_acked, > + int *flag) > { > struct tcp_sock *tp = tcp_sk(sk); > u32 delta; > @@ -600,11 +601,11 @@ static u32 tcp_accecn_process(struct sock *sk, const > struct sk_buff *skb, > if (delta > 0) { > tcp_count_delivered_ce(tp, delta); > *flag |= FLAG_ECE; > + rs->ece_delta = delta; > /* Recalculate header predictor */ > if (tp->pred_flags) > tcp_fast_path_on(tp); > } > - return delta; > } Does this compile? Did you try to compile this patch? This removes the only "return" statement from a function that returns a value. Looks like invalid C to me. :-) > > /* Buffer size and advertised window tuning. > @@ -4220,8 +4221,8 @@ static void tcp_xmit_recovery(struct sock *sk, int > rexmit) > } > > /* Returns the number of packets newly acked or sacked by the current ACK */ > -static u32 tcp_newly_delivered(struct sock *sk, u32 prior_delivered, > - u32 ecn_count, int flag) > +static u32 tcp_newly_delivered(struct sock *sk, struct rate_sample *rs, > + u32 prior_delivered, int flag) > { > const struct net *net = sock_net(sk); > struct tcp_sock *tp = tcp_sk(sk); > @@ -4232,8 +4233,8 @@ static u32 tcp_newly_delivered(struct sock *sk, u32 > prior_delivered, > > if (flag & FLAG_ECE) { > if (tcp_ecn_mode_rfc3168(tp)) This (tcp_ecn_mode_rfc3168(tp)) expression seems to suggest that the value will not be available for AccECN connections. How will Prague use this rs->ece_delta field if it's only available for rfc3168 ECN? I suggest addressing this so that the rs->ece_delta is updated if tcp_ecn_mode_any(), and putting that change in a new commit, placed before this commit in this patch series. > > - ecn_count = delivered; > - NET_ADD_STATS(net, LINUX_MIB_TCPDELIVEREDCE, ecn_count); > + rs->ece_delta = delivered; > + NET_ADD_STATS(net, LINUX_MIB_TCPDELIVEREDCE, rs->ece_delta); > } > > return delivered; > @@ -4288,7 +4289,7 @@ static int tcp_ack(struct sock *sk, const struct > sk_buff *skb, int flag) > struct inet_connection_sock *icsk = inet_csk(sk); > struct tcp_sock *tp = tcp_sk(sk); > struct tcp_sacktag_state sack_state; > - struct rate_sample rs = { .prior_delivered = 0 }; > + struct rate_sample rs = { .prior_delivered = 0, .ece_delta = 0 }; Adding the ece_delta field here in the initializer is redundant. According to the C99 standard (ISO/IEC 9899:1999, Section 6.7.9, paragraph 21), if an aggregate or union contains fewer initializers than there are members, the remaining fields are implicitly initialized the same as objects that have static storage duration. When you explicitly initialize even a single scalar field (whether to zero or any other value) using a designated initializer, you trigger the C language rule for partial initialization. Arithmetic/Scalar types (integers, floats) are initialized to 0. Thanks again for the patch. Best, neal
