> -----Original Message----- > From: Ilpo Järvinen <[email protected]> > Sent: Tuesday, May 6, 2025 1:10 AM > To: Paolo Abeni <[email protected]> > Cc: Chia-Yu Chang (Nokia) <[email protected]>; > [email protected]; [email protected]; [email protected]; > [email protected]; [email protected]; [email protected]; > [email protected]; [email protected]; [email protected]; > [email protected]; [email protected]; [email protected]; > [email protected]; [email protected]; [email protected]; > [email protected]; [email protected]; [email protected]; > [email protected]; [email protected]; Koen De Schepper > (Nokia) <[email protected]>; g.white > <[email protected]>; [email protected]; > [email protected]; [email protected]; [email protected]; > [email protected]; vidhi_goel <[email protected]> > Subject: Re: [PATCH v5 net-next 07/15] tcp: allow embedding leftover into > option padding > > > 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 Tue, 29 Apr 2025, Paolo Abeni wrote: > > > On 4/22/25 5:35 PM, [email protected] wrote: > > > @@ -709,6 +709,8 @@ static __be32 *process_tcp_ao_options(struct tcp_sock > > > *tp, > > > return ptr; > > > } > > > > > > +#define NOP_LEFTOVER ((TCPOPT_NOP << 8) | TCPOPT_NOP) > > > + > > > /* Write previously computed TCP options to the packet. > > > * > > > * Beware: Something in the Internet is very sensitive to the > > > ordering of @@ -727,8 +729,10 @@ static void tcp_options_write(struct > > > tcphdr *th, struct tcp_sock *tp, > > > struct tcp_out_options *opts, > > > struct tcp_key *key) { > > > + u16 leftover_bytes = NOP_LEFTOVER; /* replace next NOPs if avail > > > */ > > > __be32 *ptr = (__be32 *)(th + 1); > > > u16 options = opts->options; /* mungable copy */ > > > + int leftover_size = 2; > > > > > > if (tcp_key_is_md5(key)) { > > > *ptr++ = htonl((TCPOPT_NOP << 24) | (TCPOPT_NOP << 16) | > > > @@ -763,17 +767,22 @@ static void tcp_options_write(struct tcphdr *th, > > > struct tcp_sock *tp, > > > } > > > > > > if (unlikely(OPTION_SACK_ADVERTISE & options)) { > > > - *ptr++ = htonl((TCPOPT_NOP << 24) | > > > - (TCPOPT_NOP << 16) | > > > + *ptr++ = htonl((leftover_bytes << 16) | > > > (TCPOPT_SACK_PERM << 8) | > > > TCPOLEN_SACK_PERM); > > > + leftover_bytes = NOP_LEFTOVER; > > > > Why? isn't leftover_bytes already == NOP_LEFTOVER? > > > > > } > > > > > > if (unlikely(OPTION_WSCALE & options)) { > > > - *ptr++ = htonl((TCPOPT_NOP << 24) | > > > + u8 highbyte = TCPOPT_NOP; > > > + > > > + if (unlikely(leftover_size == 1)) > > > > How can the above conditional be true? > > > > > + highbyte = leftover_bytes >> 8; > > > + *ptr++ = htonl((highbyte << 24) | > > > (TCPOPT_WINDOW << 16) | > > > (TCPOLEN_WINDOW << 8) | > > > opts->ws); > > > + leftover_bytes = NOP_LEFTOVER; > > > > Why? isn't leftover_bytes already == NOP_LEFTOVER? > > > > > } > > > > > > if (unlikely(opts->num_sack_blocks)) { @@ -781,8 +790,7 @@ > > > 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))); @@ -794,6 +802,10 @@ static void > > > tcp_options_write(struct tcphdr *th, struct tcp_sock *tp, > > > } > > > > > > tp->rx_opt.dsack = 0; > > > + } else if (unlikely(leftover_bytes != NOP_LEFTOVER)) { > > > > I really feel like I'm missing some code chunk, but I don't see any > > possible value for leftover_bytes other than NOP_LEFTOVER > > Hi, > > I split it this way to keep the generic part of the leftover mechanism in own > patch separate from AccECN option change itself (as you did later discover). > This is among the most convoluted parts in the entire AccECN patch series so > it seemed wise to split it as small as I could. Having those non-sensical > looking assigns here were to avoid code churn in the latter patch. The > changelog could have stated that clearly though (my fault from years back). > > > -- > i.
Hi Ilpo, Thanks for further clarifications, and I will add more comments in this patch. Chia-Yu
