Re: [PATCH v12 net-next 11/15] tcp: accecn: AccECN option

[Paolo Abeni <pabeni@redhat.com>]

On 7/4/25 10:53 AM, chia-yu.chang@nokia-bell-labs.com wrote:
> +/* Maps AccECN option field #nr to IP ECN field ECT/CE bits */
> +static inline unsigned int tcp_accecn_optfield_to_ecnfield(unsigned int option,
> +							   bool order)
> +{
> +	u8 tmp;
> +
> +	/* Below is modified from Table 5 of the AccECN spec to explain
> +	 * how the ECN fields are mapped baed on two input arguemnts:
> +	 * +=======================================+=======================+
> +	 * |                                       |  order=0  |  order=1  |
> +	 * ++======================================+===========+===========+
> +	 * | 1st field in AccECN option (option=0) |   ECT(0)  |   ECT(1)  |
> +	 * | 2nd field in AccECN option (option=1) |    CE     |    CE     |
> +	 * | 3rd field in AccECN option (option=2) |   ECT(1)  |   ECT(0)  |
> +	 * +=======================================+===========+===========+
> +	 */
> +
> +	option = order ? 2 - option : option;
> +	tmp = option + 2;
> +
> +	return (tmp + (tmp >> 2)) & INET_ECN_MASK;

Possibly/likely computing the above with an explicit static array lookup
would be simpler and more clear.

> +}
> +
> +/* Handles AccECN option ECT and CE 24-bit byte counters update into
> + * the u32 value in tcp_sock. As we're processing TCP options, it is
> + * safe to access from - 1.
> + */
> +static inline s32 tcp_update_ecn_bytes(u32 *cnt, const char *from,
> +				       u32 init_offset)
> +{
> +	u32 truncated = (get_unaligned_be32(from - 1) - init_offset) &
> +			0xFFFFFFU;
> +	u32 delta = (truncated - *cnt) & 0xFFFFFFU;
> +
> +	/* If delta has the highest bit set (24th bit) indicating
> +	 * negative, sign extend to correct an estimation using
> +	 * sign_extend32(delta, 24 - 1)
> +	 */
> +	delta = sign_extend32(delta, 23);

I'm under the impression that delta could be simply:

	delta = (truncated - *cnt)

What am I missing?

> +/* 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);

Likely/possibly:

	if (WARN_ON_ONCE(ptr[0] != TCPOPT_ACCECN0 && ptr[0] != TCPOPT_ACCECN1))
		return false;

> +	order1 = (ptr[0] == TCPOPT_ACCECN1);
> +	ptr += 2;
> +
> +	res = !!estimate_ecnfield;
> +	for (i = 0; i < 3; i++) {
> +		u32 init_offset;
> +		u8 ecnfield;
> +		s32 delta;
> +		u32 *cnt;
> +
> +		if (optlen < TCPOLEN_ACCECN_PERFIELD)
> +			break;
> +
> +		ecnfield = tcp_accecn_optfield_to_ecnfield(i, order1);
> +		init_offset = tcp_accecn_field_init_offset(ecnfield);
> +		cnt = &tp->delivered_ecn_bytes[ecnfield - 1];
> +		delta = tcp_update_ecn_bytes(cnt, ptr, init_offset);
> +		if (delta && delta < 0) {
> +			res = false;
> +			ambiguous_ecn_bytes_incr = true;
> +		}
> +		if (delta && ecnfield != estimate_ecnfield) {
> +			if (!first_changed) {
> +				tp->est_ecnfield = ecnfield;
> +				first_changed = true;
> +			} else {
> +				res = false;
> +				ambiguous_ecn_bytes_incr = true;
> +			}
> +		}
> +
> +		optlen -= TCPOLEN_ACCECN_PERFIELD;
> +		ptr += TCPOLEN_ACCECN_PERFIELD;
> +	}
> +	if (ambiguous_ecn_bytes_incr)
> +		tp->est_ecnfield = 0;
> +
> +	return res;
> +}
> +
>  static void tcp_count_delivered_ce(struct tcp_sock *tp, u32 ecn_count)
>  {
>  	tp->delivered_ce += ecn_count;
> @@ -400,7 +467,8 @@ static void tcp_count_delivered(struct tcp_sock *tp, u32 delivered,
>  
>  /* Returns the ECN CE delta */
>  static u32 __tcp_accecn_process(struct sock *sk, const struct sk_buff *skb,
> -				u32 delivered_pkts, int flag)
> +				u32 delivered_pkts, u32 delivered_bytes,
> +				int flag)
>  {
>  	const struct tcphdr *th = tcp_hdr(skb);
>  	struct tcp_sock *tp = tcp_sk(sk);
> @@ -411,6 +479,8 @@ static u32 __tcp_accecn_process(struct sock *sk, const struct sk_buff *skb,
>  	if (!(flag & (FLAG_FORWARD_PROGRESS | FLAG_TS_PROGRESS)))
>  		return 0;
>  
> +	tcp_accecn_process_option(tp, skb, delivered_bytes, flag);
> +
>  	if (!(flag & FLAG_SLOWPATH)) {
>  		/* AccECN counter might overflow on large ACKs */
>  		if (delivered_pkts <= TCP_ACCECN_CEP_ACE_MASK)
> @@ -436,12 +506,14 @@ static u32 __tcp_accecn_process(struct sock *sk, const struct sk_buff *skb,
>  }
>  
>  static u32 tcp_accecn_process(struct sock *sk, const struct sk_buff *skb,
> -			      u32 delivered_pkts, int *flag)
> +			      u32 delivered_pkts, u32 delivered_bytes,
> +			      int *flag)
>  {
>  	struct tcp_sock *tp = tcp_sk(sk);
>  	u32 delta;
>  
> -	delta = __tcp_accecn_process(sk, skb, delivered_pkts, *flag);
> +	delta = __tcp_accecn_process(sk, skb, delivered_pkts,
> +				     delivered_bytes, *flag);
>  	if (delta > 0) {
>  		tcp_count_delivered_ce(tp, delta);
>  		*flag |= FLAG_ECE;
> @@ -3973,6 +4045,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);
> @@ -4012,6 +4085,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);
>  	/* If data was DSACKed, see if we can undo a cwnd reduction. */
> @@ -4139,6 +4213,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) {
> @@ -4230,6 +4305,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.
> @@ -4290,11 +4371,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);
> @@ -6094,6 +6178,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 b95e4ed227cb..1da7c5e4da32 100644
> --- a/net/ipv4/tcp_ipv4.c
> +++ b/net/ipv4/tcp_ipv4.c
> @@ -3451,6 +3451,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_ACCECN_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 d98a1a17eb52..2169fd28594e 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -385,6 +385,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)
>  {
> @@ -406,6 +407,8 @@ 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:7,	/* number of AccECN fields needed */
> +	   use_synack_ecn_bytes:1; /* Use synack_ecn_bytes or not */
>  	u8 hash_size;		/* bytes in hash_location */
>  	u8 bpf_opt_len;		/* length of BPF hdr option */
>  	__u8 *hash_location;	/* temporary pointer, overloaded */
> @@ -621,6 +624,8 @@ static void tcp_options_write(struct tcphdr *th, struct tcp_sock *tp,
>  			      struct tcp_out_options *opts,
>  			      struct tcp_key *key)
>  {
> +	u8 leftover_highbyte = TCPOPT_NOP; /* replace 1st NOP if avail */
> +	u8 leftover_lowbyte = TCPOPT_NOP;  /* replace 2nd NOP in succession */
>  	__be32 *ptr = (__be32 *)(th + 1);
>  	u16 options = opts->options;	/* mungable copy */
>  
> @@ -656,15 +661,79 @@ static void tcp_options_write(struct tcphdr *th, struct tcp_sock *tp,
>  		*ptr++ = htonl(opts->tsecr);
>  	}
>  
> +	if (OPTION_ACCECN & options) {
> +		/* Initial values for AccECN option, ordered is based on ECN field bits
> +		 * similar to received_ecn_bytes. Used for SYN/ACK AccECN option.
> +		 */
> +		static u32 synack_ecn_bytes[3] = { 0, 0, 0 };

I think this does not address Eric's concern on v9 WRT global variable,
as every CPU will still touch the same memory while accessing the above
array.

> +		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;
> +
> +		if (opts->use_synack_ecn_bytes) {
> +			e0b = synack_ecn_bytes[ect0_idx] + TCP_ACCECN_E0B_INIT_OFFSET;
> +			e1b = synack_ecn_bytes[ect1_idx] + TCP_ACCECN_E1B_INIT_OFFSET;
> +			ceb = synack_ecn_bytes[ce_idx] + TCP_ACCECN_CEB_INIT_OFFSET;

On the flip side I don't see such array modified here, not in later
patches?!? If so you could make it const and a global variable would be ok.



> +		} else {
> +			e0b = tp->received_ecn_bytes[ect0_idx] + TCP_ACCECN_E0B_INIT_OFFSET;
> +			e1b = tp->received_ecn_bytes[ect1_idx] + TCP_ACCECN_E1B_INIT_OFFSET;
> +			ceb = tp->received_ecn_bytes[ce_idx] + TCP_ACCECN_CEB_INIT_OFFSET;
> +		}

Also it looks like the above two blocks could be condensed to something
alike:

		const u32 *ecn_bytes = opts->use_synack_ecn_bytes ?
					synack_ecn_bytes :
					tp->received_ecn_bytes;

		e0b = ecn_bytes[ect0_idx] + TCP_ACCECN_E0B_INIT_OFFSET;
		e1b = ecn_bytes[ect1_idx] + TCP_ACCECN_E1B_INIT_OFFSET;
		ceb = 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_highbyte = e1b & 0xff;
> +			leftover_lowbyte = TCPOPT_NOP;
> +		} else if (opts->num_accecn_fields == 0) {
> +			leftover_highbyte = TCPOPT_ACCECN1;
> +			leftover_lowbyte = len;
> +		} 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_highbyte << 24) |
> +			       (leftover_lowbyte << 16) |
>  			       (TCPOPT_SACK_PERM << 8) |
>  			       TCPOLEN_SACK_PERM);
> +		leftover_highbyte = TCPOPT_NOP;
> +		leftover_lowbyte = TCPOPT_NOP;
>  	}
>  
>  	if (unlikely(OPTION_WSCALE & options)) {
> -		*ptr++ = htonl((TCPOPT_NOP << 24) |
> +		u8 highbyte = TCPOPT_NOP;
> +
> +		/* Do not split the leftover 2-byte to fit into a single
> +		 * NOP, i.e., replace this NOP only when 1 byte is leftover
> +		 * within leftover_highbyte.
> +		 */
> +		if (unlikely(leftover_highbyte != TCPOPT_NOP &&
> +			     leftover_lowbyte == TCPOPT_NOP)) {
> +			highbyte = leftover_highbyte;
> +			leftover_highbyte = TCPOPT_NOP;
> +		}
> +		*ptr++ = htonl((highbyte << 24) |
>  			       (TCPOPT_WINDOW << 16) |
>  			       (TCPOLEN_WINDOW << 8) |
>  			       opts->ws);
> @@ -675,11 +744,13 @@ 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_highbyte << 24) |
> +			       (leftover_lowbyte << 16) |
>  			       (TCPOPT_SACK <<  8) |
>  			       (TCPOLEN_SACK_BASE + (opts->num_sack_blocks *
>  						     TCPOLEN_SACK_PERBLOCK)));
> +		leftover_highbyte = TCPOPT_NOP;
> +		leftover_lowbyte = TCPOPT_NOP;
>  
>  		for (this_sack = 0; this_sack < opts->num_sack_blocks;
>  		     ++this_sack) {
> @@ -688,6 +759,14 @@ static void tcp_options_write(struct tcphdr *th, struct tcp_sock *tp,
>  		}
>  
>  		tp->rx_opt.dsack = 0;
> +	} else if (unlikely(leftover_highbyte != TCPOPT_NOP ||
> +			    leftover_lowbyte != TCPOPT_NOP)) {
> +		*ptr++ = htonl((leftover_highbyte << 24) |
> +			       (leftover_lowbyte << 16) |
> +			       (TCPOPT_NOP << 8) |
> +			       TCPOPT_NOP);
> +		leftover_highbyte = TCPOPT_NOP;
> +		leftover_lowbyte = TCPOPT_NOP;
>  	}
>  
>  	if (unlikely(OPTION_FAST_OPEN_COOKIE & options)) {
> @@ -768,6 +847,59 @@ static void mptcp_set_option_cond(const struct request_sock *req,
>  	}
>  }
>  
> +static u32 tcp_synack_options_combine_saving(struct tcp_out_options *opts)
> +{
> +	/* How much there's room for combining with the alignment padding? */
> +	if ((opts->options & (OPTION_SACK_ADVERTISE | OPTION_TS)) ==
> +	    OPTION_SACK_ADVERTISE)
> +		return 2;
> +	else if (opts->options & OPTION_WSCALE)
> +		return 1;
> +	return 0;
> +}
> +
> +/* Calculates how long AccECN option will fit to @remaining option space.
> + *
> + * AccECN option can sometimes replace NOPs used for alignment of other
> + * TCP options (up to @max_combine_saving available).
> + *
> + * Only solutions with at least @required AccECN fields are accepted.
> + *
> + * Returns: The size of the AccECN option excluding space repurposed from
> + * the alignment of the other options.
> + */
> +static int tcp_options_fit_accecn(struct tcp_out_options *opts, int required,
> +				  int remaining)
> +{
> +	int size = TCP_ACCECN_MAXSIZE;
> +	int max_combine_saving;
> +
> +	if (opts->use_synack_ecn_bytes)
> +		max_combine_saving = tcp_synack_options_combine_saving(opts);
> +	else
> +		max_combine_saving = opts->num_sack_blocks > 0 ? 2 : 0;
> +	opts->num_accecn_fields = TCP_ACCECN_NUMFIELDS;
> +	while (opts->num_accecn_fields >= required) {
> +		int leftover_size = size & 0x3;
> +		/* Pad to dword if cannot combine */
> +		if (leftover_size > max_combine_saving)
> +			leftover_size = -((4 - leftover_size) & 0x3);

I *think* that with the above you mean something alike:

			size = ALIGN(size, 4);
			leftover_size = 0

?

The used code looks quite obscure to me.

/P