On 4/22/25 5:35 PM, chia-yu.ch...@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;
> if (sk->sk_state == TCP_SYN_RECV && sk->sk_socket && th->ack &&
> TCP_SKB_CB(skb)->seq + 1 == TCP_SKB_CB(skb)->end_seq &&
> TCP_SKB_CB(skb)->seq + 1 == tp->rcv_nxt &&
> - 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
