Re: [PATCH v7 3/5] tcp: Resend SYN for inbound connections

[Yumei Huang <yuhuang@redhat.com>]

On Fri, Nov 7, 2025 at 6:05 PM Stefano Brivio <sbrivio@redhat.com> wrote:
>
> On Fri, 7 Nov 2025 17:56:54 +0800
> Yumei Huang <yuhuang@redhat.com> wrote:
>
> > On Wed, Nov 5, 2025 at 3:01 PM Stefano Brivio <sbrivio@redhat.com> wrote:
> > >
> > > On Wed, 5 Nov 2025 15:24:44 +1100
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > >
> > > > On Tue, Nov 04, 2025 at 05:42:33AM +0100, Stefano Brivio wrote:
> > > > > On Fri, 31 Oct 2025 13:42:40 +0800
> > > > > Yumei Huang <yuhuang@redhat.com> wrote:
> > > > [snip]
> > > > > > +/**
> > > > > > + * tcp_get_rto_params() - Get host kernel RTO parameters
> > > > > > + * @c:           Execution context
> > > > > > +*/
> > > > > > +void tcp_get_rto_params(struct ctx *c)
> > > > > > +{
> > > > > > + intmax_t tcp_syn_retries, syn_linear_timeouts;
> > > > > > +
> > > > > > + tcp_syn_retries = read_file_integer(
> > > > > > +         TCP_SYN_RETRIES, TCP_SYN_RETRIES_DEFAULT);
> > > > > > + syn_linear_timeouts = read_file_integer(
> > > > > > +         TCP_SYN_LINEAR_TIMEOUTS, TCP_SYN_LINEAR_TIMEOUTS_DEFAULT);
> > > > >
> > > > > I think this is a bit hard to read. Now:
> > > > >
> > > > >     tcp_syn_retries = read_file_integer(TCP_SYN_RETRIES,
> > > > >                                         TCP_SYN_RETRIES_DEFAULT);
> > > > >     syn_linear_timeouts = read_file_integer(TCP_SYN_LINEAR_TIMEOUTS,
> > > > >                                             TCP_SYN_LINEAR_TIMEOUTS_DEFAULT);
> > > > >
> > > > > would be a bit closer to the coding style we're adopting, but I wonder:
> > > > >
> > > > > - does read_file_integer() really need to have "integer" in the name?
> > > >
> > > > >   In a language where integers are called 'int', perhaps
> > > > >   read_file_int() is clear enough?
> > > >
> > > > I think the idea is that read_file_integer() can be used for any
> > > > (signed) integer type (with range checking performed after the call).
> > > > read_file_int() might suggest it reads exactly an 'int', not anything
> > > > bigger or smaller.
> > >
> > > Oh, I see. It could be read_file_num() then, even if it's slightly less
> > > accurate, or it can even remain read_file_integer(). If something like
> > > this:
> > >
> > >         v = read_file_integer(SYN_RETRIES, SYN_RETRIES_DEFAULT);
> > >         c->tcp.tcp_syn_retries = MIN(v, UINT8_MAX);
> > >
> > >         v = read_file_integer(SYN_LINEAR_TIMEOUTS, SYN_LINEAR_TIMEOUTS_DEFAULT);
> > >         c->tcp.syn_linear_timeouts = MIN(v, UINT8_MAX);
> > >
> > > fits 80 columns, I'm taking it as a sign that the function name isn't
> > > exceedingly long. No particular preference from me.
> >
> > The longest line just exceeds 80 columns.
>
> No, why? It's 80 columns, look:
>
>         v = read_file_integer(SYN_LINEAR_TIMEOUTS, SYN_LINEAR_TIMEOUTS_DEFAULT);
> 01234567890123456789012345678901234567890123456789012345678901234567890123456789

You are right. Somehow I counted 81 columns. Then I will keep the name
as it is.  (To me, read_file_num() sounds like it can read any kind of
number.)
>
> > I can rename the function to read_file_num() if there is no objection from David.
>
> Sure, up to you and David.
>
> > Another thing, while I was revising the patches, I noticed the
> > parameter "const struct ctx *c" was removed from tcp_timer_ctl() by
> > commit dd5302dd7bf51 ("tcp, flow: Replace per-connection in_epoll flag
> > with an epollid in flow_common") from Laurent, but we need it to
> > access c->tcp.syn_retries and c->tcp.syn_linear_timeouts. Should we
> > add it back?
>
> Yes, I don't see any other way around it, and I think I'll need it back
> anyway for some fixes I'm working on.
>
> It's one type of trivial conflict I would typically fix up on merge, by
> the way, but given that you will send another version, you could/should
> rebase on latest HEAD anyway.

Sure.
>
> > (Adding Laurent in the loop.)
>
> --
> Stefano
>


-- 
Thanks,

Yumei Huang