RE: [PATCH v8 net-next 09/15] tcp: accecn: AccECN option

-----Original Message----- From: Paolo Abeni pabeni@redhat.com Sent: Tuesday, June 17, 2025 11:27 AM To: Chia-Yu Chang (Nokia) chia-yu.chang@nokia-bell-labs.com; edumazet@google.com; linux-doc@vger.kernel.org; corbet@lwn.net; horms@kernel.org; dsahern@kernel.org; kuniyu@amazon.com; bpf@vger.kernel.org; netdev@vger.kernel.org; dave.taht@gmail.com; jhs@mojatatu.com; kuba@kernel.org; stephen@networkplumber.org; xiyou.wangcong@gmail.com; jiri@resnulli.us; davem@davemloft.net; andrew+netdev@lunn.ch; donald.hunter@gmail.com; ast@fiberby.net; liuhangbin@gmail.com; shuah@kernel.org; linux-kselftest@vger.kernel.org; ij@kernel.org; ncardwell@google.com; Koen De Schepper (Nokia) koen.de_schepper@nokia-bell-labs.com; g.white@cablelabs.com; ingemar.s.johansson@ericsson.com; mirja.kuehlewind@ericsson.com; cheshire@apple.com; rs.ietf@gmx.at; Jason_Livingood@comcast.com; vidhi_goel@apple.com Subject: Re: [PATCH v8 net-next 09/15] tcp: accecn: AccECN option

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 6/10/25 2:53 PM, chia-yu.chang@nokia-bell-labs.com wrote:

...

@@ -294,6 +295,9 @@ struct tcp_sock { rate_app_limited:1; /* rate_{delivered,interval_us} limited? */ u8 received_ce_pending:4, /* Not yet transmit cnt of received_ce */ unused2:4;

• u8      accecn_minlen:2,/* Minimum length of AccECN option sent */
  

•         est_ecnfield:2,/* ECN field for AccECN delivered 
  

• estimates */

It's unclear to me why you didn't use the 4 bits avail in 'unused2', instead of adding more fragmented bitfields.

Hi Paolo, This is becuase some bits of unused2 will be used in latter patches.

[...]

...

+/* Returns true if the byte counters can be used */ static bool +tcp_accecn_process_option(struct tcp_sock *tp,

•                               const struct sk_buff *skb,
  

•                               u32 delivered_bytes, int flag) {
  

• u8 estimate_ecnfield = tp->est_ecnfield;
  

• bool ambiguous_ecn_bytes_incr = false;
  

• bool first_changed = false;
  

• unsigned int optlen;
  

• bool order1, res;
  

• unsigned int i;
  

• u8 *ptr;
  

• if (!(flag & FLAG_SLOWPATH) || !tp->rx_opt.accecn) {
  

•         if (estimate_ecnfield) {
  

•                 u8 ecnfield = estimate_ecnfield - 1;
  

•                 tp->delivered_ecn_bytes[ecnfield] += delivered_bytes;
  

•                 return true;
  

•         }
  

•         return false;
  

• }
  

• ptr = skb_transport_header(skb) + tp->rx_opt.accecn;
  

• optlen = ptr[1] - 2;
  

• WARN_ON_ONCE(ptr[0] != TCPOPT_ACCECN0 && ptr[0] != TCPOPT_ACCECN1);
  

• order1 = (ptr[0] == TCPOPT_ACCECN1);
  

• ptr += 2;
  

• res = !!estimate_ecnfield;
  

• for (i = 0; i < 3; i++) {
  

•         if (optlen < TCPOLEN_ACCECN_PERFIELD)
  

•                 continue;
  

Why continuing the loop if nothing could be done? possbly just 'break;'

...

•         u32 init_offset;
  

•         u8 ecnfield;
  

•         s32 delta;
  

•         u32 *cnt;
  

Please don't mix variables definition and code. Move the definition at the beginning of the block.

[...]

...

@@ -4236,6 +4375,7 @@ static int tcp_ack(struct sock *sk, const struct sk_buff *skb, int flag) if (tcp_ecn_mode_accecn(tp)) ecn_count = tcp_accecn_process(sk, skb, tp->delivered - delivered,

• sack_state.delivered_bytes, &flag);

  tcp_in_ack_event(sk, flag);
  

@@ -4275,6 +4415,7 @@ static int tcp_ack(struct sock *sk, const struct sk_buff *skb, int flag) if (tcp_ecn_mode_accecn(tp)) ecn_count = tcp_accecn_process(sk, skb, tp->delivered - delivered,

• sack_state.delivered_bytes, &flag);

The two above chunks suggest you could move more code into tcp_accecn_process()

I do not get your point here.

These two chunks reflect a new argument is added to tcp_accecn_process().

But the value of this argument is computed by other fnuctions already, so not sure how to move code into tcp_accecn_process().

...

  tcp_in_ack_event(sk, flag);
  /* If data was DSACKed, see if we can undo a cwnd reduction. */ 

@@ -4402,6 +4543,7 @@ void tcp_parse_options(const struct net *net,

  ptr = (const unsigned char *)(th + 1);
  opt_rx->saw_tstamp = 0;

• opt_rx->accecn = 0;
  opt_rx->saw_unknown = 0;
  
  while (length > 0) {
  

@@ -4493,6 +4635,12 @@ void tcp_parse_options(const struct net *net, ptr, th->syn, foc, false); break;

•                 case TCPOPT_ACCECN0:
  

•                 case TCPOPT_ACCECN1:
  

•                         /* Save offset of AccECN option in TCP header */
  

•                         opt_rx->accecn = (ptr - 2) - (__u8 *)th;
  

•                         break;
  

•                 case TCPOPT_EXP:
                          /* Fast Open option shares code 254 using a
                           * 16 bits magic number.
  

@@ -4553,11 +4701,14 @@ static bool tcp_fast_parse_options(const struct net *net, */ if (th->doff == (sizeof(*th) / 4)) { tp->rx_opt.saw_tstamp = 0;

•         tp->rx_opt.accecn = 0;
          return false;
  } else if (tp->rx_opt.tstamp_ok &&
             th->doff == ((sizeof(*th) + TCPOLEN_TSTAMP_ALIGNED) / 4)) {
  

•         if (tcp_parse_aligned_timestamp(tp, th))
  

•         if (tcp_parse_aligned_timestamp(tp, th)) {
  

•                 tp->rx_opt.accecn = 0;
                  return true;
  

•         }
  }
  
  tcp_parse_options(net, skb, &tp->rx_opt, 1, NULL); @@ -6166,8 
  

+6317,12 @@ void tcp_ecn_received_counters(struct sock *sk, const struct sk_buff *skb, tp->received_ce_pending = min(tp->received_ce_pending + pcount, 0xfU);

•         if (payload_len > 0)
  

•         if (payload_len > 0) {
  

•                 u8 minlen = 
  

• tcp_ecnfield_to_accecn_optfield(ecnfield); tp->received_ecn_bytes[ecnfield - 1] +=

payload_len;

•                 tp->accecn_minlen = max_t(u8, tp->accecn_minlen,
  

•                                           minlen);
  

•         }
  }
  

}

@@ -6387,6 +6542,7 @@ void tcp_rcv_established(struct sock *sk, struct sk_buff *skb) */

  tp->rx_opt.saw_tstamp = 0;

• tp->rx_opt.accecn = 0;
  
  /*      pred_flags is 0xS?10 << 16 + snd_wnd
   *      if header_prediction is to be made
  

diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c index c5b93802d7c0..387cf8994202 100644 --- a/net/ipv4/tcp_ipv4.c +++ b/net/ipv4/tcp_ipv4.c @@ -3450,6 +3450,7 @@ static void __net_init tcp_set_hashinfo(struct net *net) static int __net_init tcp_sk_init(struct net *net) { net->ipv4.sysctl_tcp_ecn = TCP_ECN_IN_ECN_OUT_NOECN;

• net->ipv4.sysctl_tcp_ecn_option = TCP_ECN_OPTION_FULL;
  net->ipv4.sysctl_tcp_ecn_fallback = 1;
  
  net->ipv4.sysctl_tcp_base_mss = TCP_BASE_MSS; diff --git 
  

a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c index 78e78bca0db9..8869e49c07ec 100644 --- a/net/ipv4/tcp_output.c +++ b/net/ipv4/tcp_output.c @@ -492,6 +492,7 @@ static inline bool tcp_urg_mode(const struct tcp_sock *tp) #define OPTION_SMC BIT(9) #define OPTION_MPTCP BIT(10) #define OPTION_AO BIT(11) +#define OPTION_ACCECN BIT(12)

static void smc_options_write(__be32 *ptr, u16 *options) { @@ -513,12 +514,14 @@ struct tcp_out_options { u16 mss; /* 0 to disable */ u8 ws; /* window scale, 0 to disable */ u8 num_sack_blocks; /* number of SACK blocks to include */

• u8 num_accecn_fields;   /* number of AccECN fields needed */
  u8 hash_size;           /* bytes in hash_location */
  u8 bpf_opt_len;         /* length of BPF hdr option */
  __u8 *hash_location;    /* temporary pointer, overloaded */
  __u32 tsval, tsecr;     /* need to include OPTION_TS */
  struct tcp_fastopen_cookie *fastopen_cookie;    /* Fast open cookie */
  struct mptcp_out_options mptcp;
  

• u32 *ecn_bytes;         /* AccECN ECT/CE byte counters */
  

This uncoditionally adds 16 bytes to out_options, bringing it to a 64 bytes size with MPTCP disabled. I guess that adding another field will make the zeroing of out_options visible in perf report.

You can probably avoid adding `ecn_bytes` including instead an additional bit telling tcp_options_write to fetch the bytes from tp->received_ecn_bytes or zeroing them.

...

};

static void mptcp_options_write(struct tcphdr *th, __be32 *ptr, @@ -710,6 +713,8 @@ static __be32 *process_tcp_ao_options(struct tcp_sock *tp, return ptr; }

+#define NOP_LEFTOVER ((TCPOPT_NOP << 8) | TCPOPT_NOP)

/* Write previously computed TCP options to the packet.

• Beware: Something in the Internet is very sensitive to the

ordering of @@ -728,8 +733,10 @@ static void tcp_options_write(struct tcphdr *th, struct tcp_sock *tp, struct tcp_out_options *opts, struct tcp_key *key) {

• u16 leftover_bytes = NOP_LEFTOVER;      /* replace next NOPs if avail */
  __be32 *ptr = (__be32 *)(th + 1);
  u16 options = opts->options;    /* mungable copy */
  

• int leftover_size = 2;
  
  if (tcp_key_is_md5(key)) {
          *ptr++ = htonl((TCPOPT_NOP << 24) | (TCPOPT_NOP << 16) | 
  

@@ -763,18 +770,65 @@ static void tcp_options_write(struct tcphdr *th, struct tcp_sock *tp, *ptr++ = htonl(opts->tsecr); }

• if (OPTION_ACCECN & options) {
  

•         const u8 ect0_idx = INET_ECN_ECT_0 - 1;
  

•         const u8 ect1_idx = INET_ECN_ECT_1 - 1;
  

•         const u8 ce_idx = INET_ECN_CE - 1;
  

•         u32 e0b;
  

•         u32 e1b;
  

•         u32 ceb;
  

•         u8 len;
  

•         e0b = opts->ecn_bytes[ect0_idx] + TCP_ACCECN_E0B_INIT_OFFSET;
  

•         e1b = opts->ecn_bytes[ect1_idx] + TCP_ACCECN_E1B_INIT_OFFSET;
  

•         ceb = opts->ecn_bytes[ce_idx] + TCP_ACCECN_CEB_INIT_OFFSET;
  

•         len = TCPOLEN_ACCECN_BASE +
  

•               opts->num_accecn_fields * TCPOLEN_ACCECN_PERFIELD;
  

•         if (opts->num_accecn_fields == 2) {
  

•                 *ptr++ = htonl((TCPOPT_ACCECN1 << 24) | (len << 16) |
  

•                                ((e1b >> 8) & 0xffff));
  

•                 *ptr++ = htonl(((e1b & 0xff) << 24) |
  

•                                (ceb & 0xffffff));
  

•         } else if (opts->num_accecn_fields == 1) {
  

•                 *ptr++ = htonl((TCPOPT_ACCECN1 << 24) | (len << 16) |
  

•                                ((e1b >> 8) & 0xffff));
  

•                 leftover_bytes = ((e1b & 0xff) << 8) |
  

•                                  TCPOPT_NOP;
  

•                 leftover_size = 1;
  

•         } else if (opts->num_accecn_fields == 0) {
  

•                 leftover_bytes = (TCPOPT_ACCECN1 << 8) | len;
  

•                 leftover_size = 2;
  

•         } else if (opts->num_accecn_fields == 3) {
  

•                 *ptr++ = htonl((TCPOPT_ACCECN1 << 24) | (len << 16) |
  

•                                ((e1b >> 8) & 0xffff));
  

•                 *ptr++ = htonl(((e1b & 0xff) << 24) |
  

•                                (ceb & 0xffffff));
  

•                 *ptr++ = htonl(((e0b & 0xffffff) << 8) |
  

•                                TCPOPT_NOP);
  

•         }
  

•         if (tp)
  

•                 tp->accecn_minlen = 0;
  

• }
  

• if (unlikely(OPTION_SACK_ADVERTISE & options)) {
  

•         *ptr++ = htonl((TCPOPT_NOP << 24) |
  

•                        (TCPOPT_NOP << 16) |
  

•         *ptr++ = htonl((leftover_bytes << 16) |
                         (TCPOPT_SACK_PERM << 8) |
                         TCPOLEN_SACK_PERM);
  

•         leftover_bytes = NOP_LEFTOVER;
  

•         leftover_size = 2;
  }
  
  if (unlikely(OPTION_WSCALE & options)) {
  

•         *ptr++ = htonl((TCPOPT_NOP << 24) |
  

•         u8 highbyte = TCPOPT_NOP;
  

•         if (unlikely(leftover_size == 1))
  

•                 highbyte = leftover_bytes >> 8;
  

•         *ptr++ = htonl((highbyte << 24) |
                         (TCPOPT_WINDOW << 16) |
                         (TCPOLEN_WINDOW << 8) |
                         opts->ws);
  

•         leftover_bytes = NOP_LEFTOVER;
  

This bloks looks inconsistent. AFAICS if leftover_size == 1, such a byte will be consumed, but the leftover size will be left unmodified.

...

  }

  if (unlikely(opts->num_sack_blocks)) { @@ -782,8 +836,7 @@ 

static void tcp_options_write(struct tcphdr *th, struct tcp_sock *tp, tp->duplicate_sack : tp->selective_acks; int this_sack;

•         *ptr++ = htonl((TCPOPT_NOP  << 24) |
  

•                        (TCPOPT_NOP  << 16) |
  

•         *ptr++ = htonl((leftover_bytes << 16) |
                         (TCPOPT_SACK <<  8) |
                         (TCPOLEN_SACK_BASE + (opts->num_sack_blocks *
                                               
  

TCPOLEN_SACK_PERBLOCK)));

Here leftover_size/bytes are consumed and not updated, which should be safe as they will not be used later in this function, but looks inconsistent.

The whole options handling looks very fragile to me. I really would prefer something simpler (i.e. just use the avail space if any) if that would work.

I would still use leftover_size/bytes, but make it more consistent.

As this part of code already pass AccECN packetdrill tests.

...

@@ -957,6 +1068,17 @@ static unsigned int tcp_syn_options(struct sock *sk, struct sk_buff *skb, } }

• /* Simultaneous open SYN/ACK needs AccECN option but not SYN */
  

• if (unlikely((TCP_SKB_CB(skb)->tcp_flags & TCPHDR_ACK) &&
  

•              tcp_ecn_mode_accecn(tp) &&
  

•              sock_net(sk)->ipv4.sysctl_tcp_ecn_option &&
  

•              remaining >= TCPOLEN_ACCECN_BASE)) {
  

•         u32 saving = tcp_synack_options_combine_saving(opts);
  

•         opts->ecn_bytes = synack_ecn_bytes;
  

•         remaining -= tcp_options_fit_accecn(opts, 0, remaining, saving);
  

• }
  

• bpf_skops_hdr_opt_len(sk, skb, NULL, NULL, 0, opts, &remaining);
  
  return MAX_TCP_OPTION_SPACE - remaining;
  

[...]

...

@@ -1036,6 +1159,14 @@ static unsigned int tcp_synack_options(const struct sock *sk,

  smc_set_option_cond(tcp_sk(sk), ireq, opts, &remaining);

• if (treq->accecn_ok && sock_net(sk)->ipv4.sysctl_tcp_ecn_option &&
  

•     remaining >= TCPOLEN_ACCECN_BASE) {
  

•         u32 saving = tcp_synack_options_combine_saving(opts);
  

•         opts->ecn_bytes = synack_ecn_bytes;
  

•         remaining -= tcp_options_fit_accecn(opts, 0, remaining, saving);
  

• }
  

• bpf_skops_hdr_opt_len((struct sock *)sk, skb, req, syn_skb,
                        synack_type, opts, &remaining);
  
  

The similarities of the above 2 chuncks hints you could move more code into tcp_options_fit_accecn().

/P

I also do not get it, because tcp_options_fit_accecn() will be called with different argument values.

So, I would prefer to keep as it is.

Thanks.

BRs, Chai-Yu