> -----Original Message----- > From: Neal Cardwell <[email protected]> > Sent: Monday, October 19, 2026 3:54 PM > To: Chia-Yu Chang (Nokia) <[email protected]> > Cc: [email protected]; [email protected]; [email protected]; > [email protected]; [email protected]; [email protected]; > [email protected]; [email protected]; [email protected]; [email protected]; > [email protected]; [email protected]; [email protected]; > [email protected]; [email protected]; [email protected]; > [email protected]; [email protected]; [email protected]; [email protected]; > [email protected]; [email protected]; [email protected]; > [email protected]; [email protected]; [email protected]; > [email protected]; [email protected]; Koen De Schepper (Nokia) > <[email protected]>; [email protected]; > [email protected]; [email protected]; > [email protected]; [email protected]; [email protected]; > [email protected]; [email protected]; [email protected]; > [email protected]; Olivier Tilmans (Nokia) <[email protected]> > Subject: Re: [PATCH net-next v2 1/3] tcp: accecn: Add ece_delta to rate_sample > > > CAUTION: This is an external email. Please be very careful when clicking > links or opening attachments. See the URL nok.it/ext for additional > information. > > > > 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?
Hi Neal, The current code is https://github.com/L4STeam/linux-net-next/blob/upstream_l4steam/net/ipv4/tcp_prague.c > > > 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. > I will update in the next version (after the submission is open again). > > --- 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 */ > Thanks, I will update in the next version. > > 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. :-) It compiles, because the function call is also changed to return void. > > > > > /* 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. > This part is for RFC3168 ECN mode only, the acked_sacked_ce equals delivered when FLAG_ECE is present. For AccECN mode, it carries the derived CE-marked packets from AccECN options or ACE field based on RFC9768 in tcp_accecn_process(). > > > > - 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 Thanks, you are right and I will remove this redundant behavior here. Chia-Yu
