> -----Original Message-----
> From: Ilpo Järvinen <[email protected]>
> Sent: Tuesday, May 6, 2025 1:27 AM
> To: Chia-Yu Chang (Nokia) <[email protected]>
> Cc: Paolo Abeni <[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]>; g.white
> <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; vidhi_goel <[email protected]>
> Subject: RE: [PATCH v5 net-next 10/15] tcp: accecn: AccECN option send control
>
>
> 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 Mon, 5 May 2025, Chia-Yu Chang (Nokia) wrote:
>
> > > -----Original Message-----
> > > From: Paolo Abeni <[email protected]>
> > > Sent: Tuesday, April 29, 2025 2:10 PM
> > > To: Chia-Yu Chang (Nokia) <[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]>; g.white
> > > <[email protected]>; [email protected];
> > > [email protected]; [email protected]; [email protected];
> > > [email protected]; vidhi_goel <[email protected]>
> > > Subject: Re: [PATCH v5 net-next 10/15] tcp: accecn: AccECN option
> > > send control
> > >
> > >
> > > 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 4/22/25 5:35 PM, [email protected] wrote:
> > > > From: Ilpo Järvinen <[email protected]>
> > > >
> > > > Instead of sending the option in every ACK, limit sending to those
> > > > ACKs where the option is necessary:
> > > > - Handshake
> > > > - "Change-triggered ACK" + the ACK following it. The
> > > > 2nd ACK is necessary to unambiguously indicate which
> > > > of the ECN byte counters in increasing. The first
> > > > ACK has two counters increasing due to the ecnfield
> > > > edge.
> > > > - ACKs with CE to allow CEP delta validations to take
> > > > advantage of the option.
> > > > - Force option to be sent every at least once per 2^22
> > > > bytes. The check is done using the bit edges of the
> > > > byte counters (avoids need for extra variables).
> > > > - AccECN option beacon to send a few times per RTT even if
> > > > nothing in the ECN state requires that. The default is 3
> > > > times per RTT, and its period can be set via
> > > > sysctl_tcp_ecn_option_beacon.
> > > >
> > > > Signed-off-by: Ilpo Järvinen <[email protected]>
> > > > Co-developed-by: Chia-Yu Chang <[email protected]>
> > > > Signed-off-by: Chia-Yu Chang <[email protected]>
> > > > ---
> > > > include/linux/tcp.h | 3 +++
> > > > include/net/netns/ipv4.h | 1 +
> > > > include/net/tcp.h | 1 +
> > > > net/ipv4/sysctl_net_ipv4.c | 9 ++++++++
> > > > net/ipv4/tcp.c | 5 ++++-
> > > > net/ipv4/tcp_input.c | 36 +++++++++++++++++++++++++++++++-
> > > > net/ipv4/tcp_ipv4.c | 1 +
> > > > net/ipv4/tcp_minisocks.c | 2 ++
> > > > net/ipv4/tcp_output.c | 42 ++++++++++++++++++++++++++++++--------
> > > > 9 files changed, 90 insertions(+), 10 deletions(-)
>
> > > > diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c index
> > > > 3f3e285fc973..2e95dad66fe3 100644
> > > > --- a/net/ipv4/tcp_ipv4.c
> > > > +++ b/net/ipv4/tcp_ipv4.c
> > > > @@ -3451,6 +3451,7 @@ static int __net_init tcp_sk_init(struct net
> > > > *net) {
> > > > net->ipv4.sysctl_tcp_ecn = 2;
> > > > net->ipv4.sysctl_tcp_ecn_option = 2;
> > > > + net->ipv4.sysctl_tcp_ecn_option_beacon = 3;
> > > > net->ipv4.sysctl_tcp_ecn_fallback = 1;
> > >
> > > Human readable macros instead of magic numbers could help.
> >
> > OK, commments will be added here.
>
> Hi,
>
> Using named defines to replace literals would be more useful than comments
> (names can be grepped for, do not fall out-of-sync with code, etc.).
>
> > > > @@ -1237,13 +1253,18 @@ static unsigned int
> > > > tcp_established_options(struct sock *sk, struct sk_buff *skb
> > > >
> > > > if (tcp_ecn_mode_accecn(tp) &&
> > > > sock_net(sk)->ipv4.sysctl_tcp_ecn_option) {
> > > > - int saving = opts->num_sack_blocks > 0 ? 2 : 0;
> > > > - int remaining = MAX_TCP_OPTION_SPACE - size;
> > > > -
> > > > - opts->ecn_bytes = tp->received_ecn_bytes;
> > > > - size += tcp_options_fit_accecn(opts, tp->accecn_minlen,
> > > > - remaining,
> > > > - saving);
> > > > + if (sock_net(sk)->ipv4.sysctl_tcp_ecn_option >= 2 ||
> > > > + tp->accecn_opt_demand ||
> > > > + tcp_accecn_option_beacon_check(sk)) {
> > >
> > > Why a nested if here and just not expanding the existing one?
> >
> > Sure, will merge them.
>
> While I don't remember everything that well anymore, this might have been to
> reduce code churn in some later patch, so it might be worth to check it first
> (that patch might even fall outside of this series now that these are split
> into multiple chunks).
>
> --
> i.
Hi Ilpo,
Thanks for this raised point.
And I've checked that the condition is changed at the latter patches ("tcp:
accecn: AccECN option failure handling"), but with the similar nested if.
So, I would try to merge it at the next version, and will check packetdrill for
sure.
Chia-Yu
