Re: [PATCH][v4] tcp: fix ICMP-RTO war

[Jerry Chu <hkchu@google.com>]

On Sat, May 8, 2010 at 1:30 AM, Damian Lukowski
<damian@tvk.rwth-aachen.de> wrote:
>
> > I'm working on a patch that tries to measure and use the RTT for the passive
> > open side when the TS option is NOT enabled. My code conflicts with your
> > recently added "tcp_ack_update_rtt(sk, 0, 0);" Could you tell me why do you
> > force this call for the no-TS case when obviously "0" is not a measured RTT?
> > If you try to force icsk_rto to be initialized correctly, it is
> > already initialized to
> > TCP_TIMEOUT_INIT by tcp_create_openreq_child(). What am I missing?
>
> Hi,
> the backoff reversion code uses __tcp_set_rto() to recalculate icsk_rto,
> which itself relies on tp->srtt and rttvar.

Guess you are talking about

inet_csk(sk)->icsk_rto = __tcp_set_rto(tp) <<
                                         icsk->icsk_backoff;

inside tcp_v4_err(), right? (I'm looking at 2.6.33 kernel.)

Yes it seems to be a bug when __tcp_set_rto() is called before
tcp_rtt_estimator()
gets a chance to initialize all the variables properly.

But I don't like your fix of adding tcp_ack_update_rtt(sk, 0, 0); to
tcp_rcv_state_process()
because that means you've got a measured RTT of 0 (really meaning < 1 tick) for
the no-TS case, which will cause tcp_rtt_estimator() to compute all
the variables as if
there has been a valid RTT measurement of 1.

A better fix IMHO is to make sure all the variables are properly
initialized when exiting
tcp_init_metrics(), e.g, if srtt remains 0, make sure
tp->mdev = tp->mdev_max = tp->rttvar = TCP_TIMEOUT_INIT;
(mdev already been initialized to TCP_TIMEOUT_INIT. I think you got
hit by rttvar == 0)

> srtt is explicitly set to 0 in tcp_create_openreq_child(), so I didn't change it.
> Initializing it with TCP_TIMEOUT_INIT should also fix that specific bug,
> but I don't know if there are other impacts.

So what do I care? Because I'm mucking with the code in this area and your fix
causes some conflict with my logic.

What do you think?

Best,

Jerry

>
> Regards
>  Damian
>
> > Thanks,
> >
> > Jerry
> >
> >> From: David Miller <davem@davemloft.net>
> >>
> >> Date: Sun, Feb 21, 2010 at 7:10 PM
> >> Subject: Re: [PATCH][v4] tcp: fix ICMP-RTO war
> >> To: ilpo.jarvinen@helsinki.fi
> >> Cc: damian@tvk.rwth-aachen.de, netdev@vger.kernel.org
> >>
> >>
> >> From: "Ilpo Järvinen" <ilpo.jarvinen@helsinki.fi>
> >> Date: Tue, 16 Feb 2010 14:45:25 +0200 (EET)
> >>
> >>> On Wed, 10 Feb 2010, Damian Lukowski wrote:
> >>>
> >>>> @@ -5783,12 +5783,10 @@ int tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb,
> >>>>
> >>>>                              /* tcp_ack considers this ACK as duplicate
> >>>>                               * and does not calculate rtt.
> >>>> -                             * Fix it at least with timestamps.
> >>>> +                             * Force it here.
> >>>>                               */
> >>>> -                            if (tp->rx_opt.saw_tstamp &&
> >>>> -                                tp->rx_opt.rcv_tsecr && !tp->srtt)
> >>>> -                                    tcp_ack_saw_tstamp(sk, 0);
> >>>> -
> >>>> +                            tcp_ack_update_rtt(sk, 0, 0);
> >>>> +
> >>>
> >>> ...Here a zero seq_rtt is given to RTT estimator (it will be effective
> >>> only in the case w/o timestamps, TS case recalculates it from the stored
> >>> timestamps). Maybe we could use some field (timestamp related one comes to
> >>> my mind) in request sock to get a real RTT estimate for non-timestamp case
> >>> too. ...It seems possible to me, though tricky because the request_sock is
> >>> no longer that easily available here so some parameter passing would be
> >>> needed.
> >>
> >> Agreed.
> >>
> >> But even more simply I think we should make even the current
> >> tcp_ack_update_rtt() call here conditional on at least
> >> tp->srtt being zero.
> >>
> >> Damian do you at least agree with that?
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe netdev" in
> >> the body of a message to majordomo@vger.kernel.org
> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>