Re: [PATCH mptcp-next v4] mptcp: honour configured min/max RTO in retransmit paths

Hi Kalpan,

> The MPTCP-level retransmit timers (DATA_FIN retransmissions and the
> fallback timeout) used the hard-coded TCP_RTO_MIN / TCP_RTO_MAX
> constants, ignoring the tcp_rto_min_us and tcp_rto_max_ms sysctls.
>
> Make them follow the sysctls instead: seed icsk_rto_min / icsk_rto_max
> on the MPTCP socket from the per-netns sysctls in __mptcp_init_sock()
> -- the msk does not go through tcp_init_sock(), so these fields would
> otherwise stay zero -- and read them directly where the constants were
> used:
>
> - mptcp_set_datafin_timeout(): both the backoff cap computation and
>   the resulting timer_ival. The two sysctls are validated
>   independently, so rto_min > rto_max is a valid configuration; keep
>   a max_t() guard so ilog2() is never called with 0.
>
> - __mptcp_set_timeout(): the fallback when no subflow timeout is
>   available.
>
> The icsk fields are read directly instead of using the
> tcp_rto_min()/tcp_rto_max() helpers: the MPTCP socket does not perform
> routing lookups in these paths, so the rto_min route metric checked by
> tcp_rto_min() can never apply here. The TCP_RTO_MIN_US /
> TCP_RTO_MAX_MS socket options are not supported by MPTCP setsockopt()
> either; this can be revisited if they get supported on MPTCP sockets.
>
> The remaining uses of TCP_RTO_MAX in net/mptcp/ctrl.c (default
> add_addr_timeout) and net/mptcp/subflow.c (MP_FAIL timeout) are
> intentionally left unchanged: they use the constant as a default
> duration, not as an RTO bound on a retransmit timer.
>
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/618
> Reported-by: Li Xiasong <[email protected]>
> Closes: https://lore.kernel.org/all/[email protected]/

I guess we can remove these two lines because it was about fixes related
to the route checks that are no longer used, and this might be confusing
for others, right?

If yes, no need to send a new version, I can remove them when applying
the patch. The rest looks good to me:

Reviewed-by: Matthieu Baerts (NGI0) <[email protected]>

Also, usually there is no Reported-by tags when addressing review
comments: you can either use 'Suggested-by' when the whole idea was
given by someone else, mention a person in the commit message if a part
of the idea is coming from someone else, or only in the changelog.

BTW, thank you, Li, for the previous review!

-- 
Matthieu Baerts (NGI0) <[email protected]>