Hi,
I'm sorry for the late reply.
On 6/21/25 1:22 AM, Chia-Yu Chang (Nokia) wrote:
From: Paolo Abeni <pabeni@redhat.com Sent: Tuesday, June 17, 2025 11:27 AM
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.
I see. Still it would be more clear to use the avail unused space first. The final effect/layout would be the same. Or add an explicit note in the commit message.
@@ -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().
My point is that the 2 above chunks are identical, so you could possibly move more (idenical) code into the helper and reduce the code duplication.
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.
AFAICS the 3 lines inside the if branch are identical. You could create an helper for that.
Side note: I'm spending quite a bit of time trimming the irrelevant part of the reply to make it as straightforward as possible. Please do the same: having to navigate hundred of lines of unrelated quoted text to find a single line of contents maximize the chances of missing it.
Thanks,
Paolo
/P