Re: [RFC] tcp: Consider mtu probing for tcp_xmit_size_goal

[Leonard Crestez]

On 26.04.2021 18:40, Neal Cardwell wrote:
> Thanks for the patch. Some thoughts, in-line below:
>
> On Sun, Apr 25, 2021 at 10:22 PM Leonard Crestez <lcrestez@xxxxxxxxxxxxx> wrote:
> > According to RFC4821 Section 7.4 "Protocols MAY delay sending non-probes
> > in order to accumulate enough data" but linux almost never does that.
> >
> > Linux checks for probe_size + (1 + retries) * mss_cache to be available
>
> I think this means to say "reordering" rather than "retries"?
>
> > in the send buffer and if that condition is not met it will send anyway
> > using the current MSS. The feature can be made to work by sending very
> > large chunks of data from userspace (for example 128k) but for small
> > writes on fast links tcp mtu probes almost never happen.
> >
> > This patch tries to take mtu probe into account in tcp_xmit_size_goal, a
> > function which otherwise attempts to accumulate a packet suitable for
> > TSO. No delays are introduced beyond existing autocork heuristics.
>
> I would suggest phrasing this paragraph as something like:
>
>    This patch generalizes tcp_xmit_size_goal(), a
>    function which is used in autocorking in order to attempt to
>    accumulate suitable transmit sizes, so that it takes into account
>    not only TSO and receive window, but now also transmit
>    sizes needed for efficient PMTU discovery (when a flow is
>    eligible for PMTU discovery).
>
> > Suggested-by: Matt Mathis <mattmathis@xxxxxxxxxx>
> > Signed-off-by: Leonard Crestez <lcrestez@xxxxxxxxxxxxx>
> > ---
> ...
> > +
> > +               /* Timer for wait_data */
> > +               struct    timer_list    wait_data_timer;
>
> This timer_list seems left over from a previous patch version, and no
> longer used?

Yes, I'll most a cleaned-up version.

> > @@ -1375,10 +1376,12 @@ static inline void tcp_slow_start_after_idle_check(struct sock *sk)
> >          s32 delta;
> >
> >          if (!sock_net(sk)->ipv4.sysctl_tcp_slow_start_after_idle || tp->packets_out ||
> >              ca_ops->cong_control)
> >                  return;
> > +       if (inet_csk(sk)->icsk_mtup.wait_data)
> > +               return;
> >          delta = tcp_jiffies32 - tp->lsndtime;
> >          if (delta > inet_csk(sk)->icsk_rto)
> >                  tcp_cwnd_restart(sk, delta);
> >   }
>
> I would argue that the MTU probing heuristics should not override
> congestion control. If congestion control thinks it's proper to reset
> the cwnd to a lower value due to the connection being idle, then this
> should happen irrespective of MTU probing activity.

This is a hack from the previous version, removed.

> >   int tcp_send_mss(struct sock *sk, int *size_goal, int flags)
> >   {
> >          int mss_now;
> > diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> > index bde781f46b41..c15ed548a48a 100644
> > --- a/net/ipv4/tcp_output.c
> > +++ b/net/ipv4/tcp_output.c
> > @@ -2311,10 +2311,23 @@ static bool tcp_can_coalesce_send_queue_head(struct sock *sk, int len)
> >          }
> >
> >          return true;
> >   }
> >
> > +int tcp_mtu_probe_size_needed(struct sock *sk)
> > +{
> > +       struct inet_connection_sock *icsk = inet_csk(sk);
> > +       struct tcp_sock *tp = tcp_sk(sk);
> > +       int probe_size;
> > +       int size_needed;
> > +
> > +       probe_size = tcp_mtu_to_mss(sk, (icsk->icsk_mtup.search_high + icsk->icsk_mtup.search_low) >> 1);
> > +       size_needed = probe_size + (tp->reordering + 1) * tp->mss_cache;
>
> Adding this helper without refactoring tcp_mtu_probe() to use this
> helper would leave some significant unnecessary code duplication. To
> avoid duplication, AFAICT your tcp_mtu_probe() should call your new
> helper.

Yes. This function should also check if RACK is enabled and try to send 
smaller probes in that case.

> You may also want to make this little helper static inline, since
> AFAICT it is called in some hot paths.

It's only called when tp->icsk_mtup.wait_data is marked true so only for 
a brief while every ten minutes or so. It is possible that this "wait" 
stretches for a long time though.

> >   static bool tcp_write_xmit(struct sock *sk, unsigned int mss_now, int nonagle,
> >                             int push_one, gfp_t gfp)
> >   {
> > +       struct inet_connection_sock *icsk = inet_csk(sk);
> >          struct tcp_sock *tp = tcp_sk(sk);
> > +       struct net *net = sock_net(sk);
> >          struct sk_buff *skb;
> >          unsigned int tso_segs, sent_pkts;
> >          int cwnd_quota;
> >          int result;
> >          bool is_cwnd_limited = false, is_rwnd_limited = false;
> >          u32 max_segs;
> >
> >          sent_pkts = 0;
> >
> >          tcp_mstamp_refresh(tp);
> > +       /*
> > +        * Waiting for tcp probe data also applies when push_one=1
> > +        * If user does many small writes we hold them until we have have enough
> > +        * for a probe.
> > +        */
>
> This comment seems incomplete; with this patch AFAICT small writes are
> not simply always held unconditionally until there is enough data for
> a full-sized MTU probe. Rather the autocorking mechanism is used, so
> that if the flow if a flow is eligible for MTU probes, autocorking
> attempts to wait for a full amount of data with which to probe (and as
> is the case with autocorking, if this does not become available then
> when the local send queues are empty then the flow sends out whatever
> it has).

Comment is from a previous version, should be deleted.

--
Regards,
Leonard