On 4/22/25 5:35 PM, chia-yu.chang@nokia-bell-labs.com wrote:
@@ -302,10 +303,13 @@ struct tcp_sock { u32 snd_up; /* Urgent pointer */ u32 delivered; /* Total data packets delivered incl. rexmits */ u32 delivered_ce; /* Like the above but only ECE marked packets */
- u32 delivered_ecn_bytes[3];
This new fields do not belong to this cacheline group. I'm unsure they belong to fast-path at all. Also u32 will wrap-around very soon.
[...]
diff --git a/include/uapi/linux/tcp.h b/include/uapi/linux/tcp.h index dc8fdc80e16b..74ac8a5d2e00 100644 --- a/include/uapi/linux/tcp.h +++ b/include/uapi/linux/tcp.h @@ -298,6 +298,13 @@ struct tcp_info { __u32 tcpi_snd_wnd; /* peer's advertised receive window after * scaling (bytes) */
- __u32 tcpi_received_ce; /* # of CE marks received */
- __u32 tcpi_delivered_e1_bytes; /* Accurate ECN byte counters */
- __u32 tcpi_delivered_e0_bytes;
- __u32 tcpi_delivered_ce_bytes;
- __u32 tcpi_received_e1_bytes;
- __u32 tcpi_received_e0_bytes;
- __u32 tcpi_received_ce_bytes;
This will break uAPI: new fields must be addded at the end, or must fill existing holes. Also u32 set in stone in uAPI for a byte counter looks way too small.
@@ -5100,7 +5113,7 @@ static void __init tcp_struct_check(void) /* 32bit arches with 8byte alignment on u64 fields might need padding * before tcp_clock_cache. */
- CACHELINE_ASSERT_GROUP_SIZE(struct tcp_sock, tcp_sock_write_txrx, 109 + 7);
- CACHELINE_ASSERT_GROUP_SIZE(struct tcp_sock, tcp_sock_write_txrx, 122 + 6);
The above means an additional cacheline in fast-path WRT the current status. IMHO should be avoided.
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c index 5bd7fc9bcf66..41e45b9aff3f 100644 --- a/net/ipv4/tcp_input.c +++ b/net/ipv4/tcp_input.c @@ -70,6 +70,7 @@ #include <linux/sysctl.h> #include <linux/kernel.h> #include <linux/prefetch.h> +#include <linux/bitops.h> #include <net/dst.h> #include <net/tcp.h> #include <net/proto_memory.h> @@ -499,6 +500,144 @@ static bool tcp_ecn_rcv_ecn_echo(const struct tcp_sock *tp, const struct tcphdr return false; } +/* Maps IP ECN field ECT/CE code point to AccECN option field number, given
- we are sending fields with Accurate ECN Order 1: ECT(1), CE, ECT(0).
- */
+static u8 tcp_ecnfield_to_accecn_optfield(u8 ecnfield) +{
- switch (ecnfield) {
- case INET_ECN_NOT_ECT:
return 0; /* AccECN does not send counts of NOT_ECT */- case INET_ECN_ECT_1:
return 1;- case INET_ECN_CE:
return 2;- case INET_ECN_ECT_0:
return 3;- default:
WARN_ONCE(1, "bad ECN code point: %d\n", ecnfield);
No WARN_ONCE() above please: either the 'ecnfield' data is masked vs INET_ECN_MASK and the WARN_ONCE should not be possible or a remote sender can deterministically trigger a WARN() which nowadays will in turn raise a CVE...
[...]
+static u32 tcp_accecn_field_init_offset(u8 ecnfield) +{
- switch (ecnfield) {
- case INET_ECN_NOT_ECT:
return 0; /* AccECN does not send counts of NOT_ECT */- case INET_ECN_ECT_1:
return TCP_ACCECN_E1B_INIT_OFFSET;- case INET_ECN_CE:
return TCP_ACCECN_CEB_INIT_OFFSET;- case INET_ECN_ECT_0:
return TCP_ACCECN_E0B_INIT_OFFSET;- default:
WARN_ONCE(1, "bad ECN code point: %d\n", ecnfield);
Same as above.
- }
- return 0;
+}
+/* Maps AccECN option field #nr to IP ECN field ECT/CE bits */ +static unsigned int tcp_accecn_optfield_to_ecnfield(unsigned int optfield,
bool order)+{
- u8 tmp;
- optfield = order ? 2 - optfield : optfield;
- tmp = optfield + 2;
- return (tmp + (tmp >> 2)) & INET_ECN_MASK;
+}
+/* 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 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);
- *cnt += delta;
- return (s32)delta;
+}
+/* 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;
- unsigned char *ptr;
- bool order1, res;
- unsigned int i;
- 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;
This assumes optlen is greater then 2, but I don't see the relevant check. Are tcp options present at all?
- WARN_ON_ONCE(ptr[0] != TCPOPT_ACCECN0 && ptr[0] != TCPOPT_ACCECN1);
Please, don't warn for arbitrary wrong data sent from the peer.
- order1 = (ptr[0] == TCPOPT_ACCECN1);
- ptr += 2;
- res = !!estimate_ecnfield;
- for (i = 0; i < 3; i++) {
if (optlen >= TCPOLEN_ACCECN_PERFIELD) {u32 init_offset;u8 ecnfield;s32 delta;u32 *cnt;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) {if (delta < 0) {res = false;ambiguous_ecn_bytes_incr = true;}if (ecnfield != estimate_ecnfield) {if (!first_changed) {tp->est_ecnfield = ecnfield;first_changed = true;} else {res = false;ambiguous_ecn_bytes_incr = true;}
At least 2 indentation levels above the maximum readable.
[...]
@@ -4378,6 +4524,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;
It would be good to be able to zero both 'accecn' and 'saw_unknown' with a single statement.
[...]
@@ -766,6 +769,47 @@ 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);
The above chunck and the contents of patch 7 must be in the same patch. This split makes the review even harder.
[...]
@@ -1117,6 +1235,17 @@ static unsigned int tcp_established_options(struct sock *sk, struct sk_buff *skb opts->num_sack_blocks = 0; }
- 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;
AFACS the above means tcp_options_fit_accecn() must clear any already set options, but apparently it does not do so. Have you tested with something adding largish options like mptcp?
/P