On 4/22/25 5:35 PM, chia-yu.chang@nokia-bell-labs.com wrote:
diff --git a/include/linux/tcp.h b/include/linux/tcp.h index e36018203bd0..af38fff24aa4 100644 --- a/include/linux/tcp.h +++ b/include/linux/tcp.h @@ -156,6 +156,10 @@ struct tcp_request_sock { #if IS_ENABLED(CONFIG_MPTCP) bool drop_req; #endif
- u8 accecn_ok : 1,
syn_ect_snt: 2,syn_ect_rcv: 2;- u8 accecn_fail_mode:4;
AFAICS this will create a 3 bytes hole. That could be bad if it will also increase the number of cachelines used by struct tcp_request_sock. Please include the pahole info and struct size in the commit message.
If there is no size problem I guess you are better off using a 'bool' for 'accecn_ok'
u32 txhash; u32 rcv_isn; u32 snt_isn; @@ -376,7 +380,10 @@ struct tcp_sock { u8 compressed_ack; u8 dup_ack_counter:2, tlp_retrans:1, /* TLP is a retransmission */
unused:5;
syn_ect_snt:2, /* AccECN ECT memory, only */syn_ect_rcv:2, /* ... needed durign 3WHS + first seqno */wait_third_ack:1; /* Wait 3rd ACK in simultaneous open */
A good bunch of conditionals will be added to the fast path checking this flag. Is simult open really a thing for AccECN? Can we simple disable AccECN in such scenarios and simplify the code a bit? In my limited experience only syzkaller reliably use it.
- u8 accecn_fail_mode:4; /* AccECN failure handling */
This is outside the fastpath area, so possibly the struct size increase is less critical, but AFAICS this will create a 6bits hole (as the next u8 has only 6bit used). I think it's better to read the 'unused' field to mark such hole.
u8 thin_lto : 1,/* Use linear timeouts for thin streams */ fastopen_connect:1, /* FASTOPEN_CONNECT sockopt */ fastopen_no_cookie:1, /* Allow send/recv SYN+data without a cookie */
[...]
+/* See Table 2 of the AccECN draft */ +static void tcp_ecn_rcv_synack(struct sock *sk, const struct tcphdr *th,
u8 ip_dsfield)+{
- struct tcp_sock *tp = tcp_sk(sk);
- u8 ace = tcp_accecn_ace(th);
- switch (ace) {
- case 0x0:
- case 0x7: tcp_ecn_mode_set(tp, TCP_ECN_DISABLED);
break;- case 0x1:
- case 0x5:
Possibly some human readable defines could help instead of magic numbers here.
[...]
@@ -6171,16 +6252,27 @@ static bool tcp_validate_incoming(struct sock *sk, struct sk_buff *skb, * RFC 5961 4.2 : Send a challenge ack */ if (th->syn) {
if (tcp_ecn_mode_accecn(tp))send_accecn_reflector = true;
TCP_SKB_CB(skb)->ack_seq == tp->snd_nxt)
TCP_SKB_CB(skb)->ack_seq == tp->snd_nxt) {if (!tcp_ecn_disabled(tp)) {u8 ect = tp->syn_ect_rcv;tp->wait_third_ack = true;__tcp_send_ack(sk, tp->rcv_nxt,!send_accecn_reflector ? 0 :tcp_accecn_reflector_flags(ect));
The same expression is used above possibly you can create a new helper for this statement.
...
This patch is quite huge. Any hope to break id down to a more palatable size? i.e. moving the 3rd ack/self connect handling to a separate patch (if that thing is really needed).
/P