Re: [PATCH v8 net-next 09/15] tcp: accecn: AccECN option

[Paolo Abeni <pabeni@redhat.com>]

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