Hi,
I'm sorry for the late reply.
On 6/21/25 1:22 AM, Chia-Yu Chang (Nokia) wrote:
> From: Paolo Abeni <pab...@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.ch...@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
