TCP NewReno and single retransmit

TCP NewReno and single retransmit

[Marcelo Ricardo Leitner]

Hi,

We have a report from a customer saying that on a very calm connection, like 
having only a single data packet within some minutes, if this packet gets to 
be re-transmitted, retrans_stamp is only cleared when the next acked packet is 
received. But this may make we abort the connection too soon if this next 
packet also gets lost, because the reference for the initial loss is still for 
a big while ago..

                    local-machine              remote-machine
                         |                           |
          send#1---->(*1)|--------> data#1 --------->|
                   |     |                           |
                  RTO    :                           :
                   |     |                           |
                  ---(*2)|----> data#1(retrans) ---->|
                   | (*3)|<---------- ACK <----------|
                   |     |                           |
                   |     :                           :
                   |     :                           :
                   |     :                           :
                 16 minutes (or more)                :
                   |     :                           :
                   |     :                           :
                   |     :                           :
                   |     |                           |
          send#2---->(*4)|--------> data#2 --------->|
                   |     |                           |
                  RTO    :                           :
                   |     |                           |
                  ---(*5)|----> data#2(retrans) ---->|
                   |     |                           |
                   |     |                           |
                 RTO*2   :                           :
                   |     |                           |
                   |     |                           |
       ETIMEDOUT<----(*6)|                           |
    (diagram is not mine)

ETIMEDOUT happens way too early, because that's based on (*2) stamp.

Question is, can't we really clear retrans_stamp on step (*3)? Like with:

@@ -2382,31 +2382,32 @@ static inline bool tcp_may_undo(const struct tcp_sock *tp)
  static bool tcp_try_undo_recovery(struct sock *sk)
  {
         struct tcp_sock *tp = tcp_sk(sk);

         if (tcp_may_undo(tp)) {
                 int mib_idx;

                 /* Happy end! We did not retransmit anything
                  * or our original transmission succeeded.
                  */
                 DBGUNDO(sk, inet_csk(sk)->icsk_ca_state == TCP_CA_Loss ? 
"loss" : "retrans");
                 tcp_undo_cwnd_reduction(sk, false);
                 if (inet_csk(sk)->icsk_ca_state == TCP_CA_Loss)
                         mib_idx = LINUX_MIB_TCPLOSSUNDO;
                 else
                         mib_idx = LINUX_MIB_TCPFULLUNDO;

                 NET_INC_STATS_BH(sock_net(sk), mib_idx);
         }
         if (tp->snd_una == tp->high_seq && tcp_is_reno(tp)) {
                 /* Hold old state until something *above* high_seq
                  * is ACKed. For Reno it is MUST to prevent false
                  * fast retransmits (RFC2582). SACK TCP is safe. */
                 tcp_moderate_cwnd(tp);
+               tp->retrans_stamp = 0;
                 return true;
         }
         tcp_set_ca_state(sk, TCP_CA_Open);
         return false;
  }

We would still hold state, at least part of it.. WDYT?

Thanks,
Marcelo

Re: TCP NewReno and single retransmit

[Neal Cardwell]

On Mon, Oct 27, 2014 at 2:49 PM, Marcelo Ricardo Leitner
<mleitner@redhat.com> wrote:
> Hi,
>
> We have a report from a customer saying that on a very calm connection, like
> having only a single data packet within some minutes, if this packet gets to
> be re-transmitted, retrans_stamp is only cleared when the next acked packet
> is received. But this may make we abort the connection too soon if this next
> packet also gets lost, because the reference for the initial loss is still
> for a big while ago..
...
> @@ -2382,31 +2382,32 @@ static inline bool tcp_may_undo(const struct
> tcp_sock *tp)
>  static bool tcp_try_undo_recovery(struct sock *sk)
...
>         if (tp->snd_una == tp->high_seq && tcp_is_reno(tp)) {
>                 /* Hold old state until something *above* high_seq
>                  * is ACKed. For Reno it is MUST to prevent false
>                  * fast retransmits (RFC2582). SACK TCP is safe. */
>                 tcp_moderate_cwnd(tp);
> +               tp->retrans_stamp = 0;
>                 return true;
>         }
>         tcp_set_ca_state(sk, TCP_CA_Open);
>         return false;
>  }
>
> We would still hold state, at least part of it.. WDYT?

This approach sounds OK to me as long as we include a check of
tcp_any_retrans_done(), as we do in the similar code paths (for
motivation, see the comment above tcp_any_retrans_done()).

So it sounds fine to me if you change that one new line to the following 2:

+  if (!tcp_any_retrans_done(sk))
+    tp->retrans_stamp = 0;

Nice catch!

neal

Re: TCP NewReno and single retransmit

[Marcelo Ricardo Leitner]

On 30-10-2014 00:03, Neal Cardwell wrote:
> On Mon, Oct 27, 2014 at 2:49 PM, Marcelo Ricardo Leitner
> <mleitner@redhat.com> wrote:
>> Hi,
>>
>> We have a report from a customer saying that on a very calm connection, like
>> having only a single data packet within some minutes, if this packet gets to
>> be re-transmitted, retrans_stamp is only cleared when the next acked packet
>> is received. But this may make we abort the connection too soon if this next
>> packet also gets lost, because the reference for the initial loss is still
>> for a big while ago..
> ...
>> @@ -2382,31 +2382,32 @@ static inline bool tcp_may_undo(const struct
>> tcp_sock *tp)
>>   static bool tcp_try_undo_recovery(struct sock *sk)
> ...
>>          if (tp->snd_una == tp->high_seq && tcp_is_reno(tp)) {
>>                  /* Hold old state until something *above* high_seq
>>                   * is ACKed. For Reno it is MUST to prevent false
>>                   * fast retransmits (RFC2582). SACK TCP is safe. */
>>                  tcp_moderate_cwnd(tp);
>> +               tp->retrans_stamp = 0;
>>                  return true;
>>          }
>>          tcp_set_ca_state(sk, TCP_CA_Open);
>>          return false;
>>   }
>>
>> We would still hold state, at least part of it.. WDYT?
>
> This approach sounds OK to me as long as we include a check of
> tcp_any_retrans_done(), as we do in the similar code paths (for
> motivation, see the comment above tcp_any_retrans_done()).

Yes, okay. I thought that this would be taken care of already by then but 
reading the code again now after your comment, I can see what you're saying. 
Thanks.

> So it sounds fine to me if you change that one new line to the following 2:
>
> +  if (!tcp_any_retrans_done(sk))
> +    tp->retrans_stamp = 0;

Will do.

> Nice catch!

A good part of it (including the diagram) was done by customer. :)
I'll post the patch as soon as we sync with them (credits).

Marcelo

Re: TCP NewReno and single retransmit

[Yuchung Cheng]

On Thu, Oct 30, 2014 at 7:24 PM, Marcelo Ricardo Leitner
<mleitner@redhat.com> wrote:
> On 30-10-2014 00:03, Neal Cardwell wrote:
>>
>> On Mon, Oct 27, 2014 at 2:49 PM, Marcelo Ricardo Leitner
>> <mleitner@redhat.com> wrote:
>>>
>>> Hi,
>>>
>>> We have a report from a customer saying that on a very calm connection,
>>> like
>>> having only a single data packet within some minutes, if this packet gets
>>> to
>>> be re-transmitted, retrans_stamp is only cleared when the next acked
>>> packet
>>> is received. But this may make we abort the connection too soon if this
>>> next
>>> packet also gets lost, because the reference for the initial loss is
>>> still
>>> for a big while ago..
>>
>> ...
>>>
>>> @@ -2382,31 +2382,32 @@ static inline bool tcp_may_undo(const struct
>>> tcp_sock *tp)
>>>   static bool tcp_try_undo_recovery(struct sock *sk)
>>
>> ...
>>>
>>>          if (tp->snd_una == tp->high_seq && tcp_is_reno(tp)) {
>>>                  /* Hold old state until something *above* high_seq
>>>                   * is ACKed. For Reno it is MUST to prevent false
>>>                   * fast retransmits (RFC2582). SACK TCP is safe. */
Or we can just remove this strange state-holding logic?

I couldn't find such a "MUST" statement in RFC2582. RFC2582 section 3
step 5 suggests exiting the recovery procedure when an ACK acknowledges
the "recover" variable (== tp->high_seq - 1).

Since we've called tcp_reset_reno_sack() before tcp_try_undo_recovery(),
I couldn't see how false fast retransmits can be triggered without
this state-holding.

Any insights?



>>>                  tcp_moderate_cwnd(tp);
>>> +               tp->retrans_stamp = 0;
>>>                  return true;
>>>          }
>>>          tcp_set_ca_state(sk, TCP_CA_Open);
>>>          return false;
>>>   }
>>>
>>> We would still hold state, at least part of it.. WDYT?
>>
>>
>> This approach sounds OK to me as long as we include a check of
>> tcp_any_retrans_done(), as we do in the similar code paths (for
>> motivation, see the comment above tcp_any_retrans_done()).
>
>
> Yes, okay. I thought that this would be taken care of already by then but
> reading the code again now after your comment, I can see what you're saying.
> Thanks.
>
>> So it sounds fine to me if you change that one new line to the following
>> 2:
>>
>> +  if (!tcp_any_retrans_done(sk))
>> +    tp->retrans_stamp = 0;
>
>
> Will do.
>
>> Nice catch!
>
>
> A good part of it (including the diagram) was done by customer. :)
> I'll post the patch as soon as we sync with them (credits).
>
> Marcelo
>

Re: TCP NewReno and single retransmit

[Marcelo Ricardo Leitner]

On 31-10-2014 01:51, Yuchung Cheng wrote:
> On Thu, Oct 30, 2014 at 7:24 PM, Marcelo Ricardo Leitner
> <mleitner@redhat.com> wrote:
>> On 30-10-2014 00:03, Neal Cardwell wrote:
>>>
>>> On Mon, Oct 27, 2014 at 2:49 PM, Marcelo Ricardo Leitner
>>> <mleitner@redhat.com> wrote:
>>>>
>>>> Hi,
>>>>
>>>> We have a report from a customer saying that on a very calm connection,
>>>> like
>>>> having only a single data packet within some minutes, if this packet gets
>>>> to
>>>> be re-transmitted, retrans_stamp is only cleared when the next acked
>>>> packet
>>>> is received. But this may make we abort the connection too soon if this
>>>> next
>>>> packet also gets lost, because the reference for the initial loss is
>>>> still
>>>> for a big while ago..
>>>
>>> ...
>>>>
>>>> @@ -2382,31 +2382,32 @@ static inline bool tcp_may_undo(const struct
>>>> tcp_sock *tp)
>>>>    static bool tcp_try_undo_recovery(struct sock *sk)
>>>
>>> ...
>>>>
>>>>           if (tp->snd_una == tp->high_seq && tcp_is_reno(tp)) {
>>>>                   /* Hold old state until something *above* high_seq
>>>>                    * is ACKed. For Reno it is MUST to prevent false
>>>>                    * fast retransmits (RFC2582). SACK TCP is safe. */
> Or we can just remove this strange state-holding logic?
>
> I couldn't find such a "MUST" statement in RFC2582. RFC2582 section 3
> step 5 suggests exiting the recovery procedure when an ACK acknowledges
> the "recover" variable (== tp->high_seq - 1).
>
> Since we've called tcp_reset_reno_sack() before tcp_try_undo_recovery(),
> I couldn't see how false fast retransmits can be triggered without
> this state-holding.
>
> Any insights?

Nice one, me neither. Neal?

 From RFC2582, Section 5, Avoiding Multiple Fast Retransmits:

    Nevertheless, unnecessary Fast Retransmits can occur with Reno or
    NewReno TCP, particularly if a Retransmit Timeout occurs during Fast
    Recovery.  (This is illustrated for Reno on page 6 of [F98], and for
    NewReno on page 8 of [F98].)  With NewReno, the data sender remains
    in Fast Recovery until either a Retransmit Timeout, or *until all of
    the data outstanding when Fast Retransmit was entered has been
    acknowledged*.  Thus with NewReno, the problem of multiple Fast
    Retransmits from a single window of data can only occur after a
    Retransmit Timeout.

Bolding mark is mine. If I didn't miss anything, as that condition was met, we 
should be good to keep that cwnd reduction (required by section 3 step 5) and 
but get back to Open state right away.

Marcelo

>>>>                   tcp_moderate_cwnd(tp);
>>>> +               tp->retrans_stamp = 0;
>>>>                   return true;
>>>>           }
>>>>           tcp_set_ca_state(sk, TCP_CA_Open);
>>>>           return false;
>>>>    }
>>>>
>>>> We would still hold state, at least part of it.. WDYT?
>>>
>>>
>>> This approach sounds OK to me as long as we include a check of
>>> tcp_any_retrans_done(), as we do in the similar code paths (for
>>> motivation, see the comment above tcp_any_retrans_done()).
>>
>>
>> Yes, okay. I thought that this would be taken care of already by then but
>> reading the code again now after your comment, I can see what you're saying.
>> Thanks.
>>
>>> So it sounds fine to me if you change that one new line to the following
>>> 2:
>>>
>>> +  if (!tcp_any_retrans_done(sk))
>>> +    tp->retrans_stamp = 0;
>>
>>
>> Will do.
>>
>>> Nice catch!
>>
>>
>> A good part of it (including the diagram) was done by customer. :)
>> I'll post the patch as soon as we sync with them (credits).
>>
>> Marcelo
>>
>

Re: TCP NewReno and single retransmit

[Neal Cardwell]

On Mon, Nov 3, 2014 at 11:38 AM, Marcelo Ricardo Leitner
<mleitner@redhat.com> wrote:
> On 31-10-2014 01:51, Yuchung Cheng wrote:
>>>>>           if (tp->snd_una == tp->high_seq && tcp_is_reno(tp)) {
>>>>>                   /* Hold old state until something *above* high_seq
>>>>>                    * is ACKed. For Reno it is MUST to prevent false
>>>>>                    * fast retransmits (RFC2582). SACK TCP is safe. */
>>
>> Or we can just remove this strange state-holding logic?
>>
>> I couldn't find such a "MUST" statement in RFC2582. RFC2582 section 3
>> step 5 suggests exiting the recovery procedure when an ACK acknowledges
>> the "recover" variable (== tp->high_seq - 1).
>>
>> Since we've called tcp_reset_reno_sack() before tcp_try_undo_recovery(),
>> I couldn't see how false fast retransmits can be triggered without
>> this state-holding.
>>
>> Any insights?
>
>
> Nice one, me neither. Neal?

Since there are no literal IETF-style "MUST" statements in RFC2582, I
think the "MUST" in the code here is expressing the design philosophy
behind the author. :-)

AFAICT the "Hold old state" code in tcp_try_undo_recovery() is a
pretty reasonable implementation of a mechanism specified in NewReno
RFCs to deal with the fundamental ambiguity between (1) dupacks for
packets the receiver received above a hole above snd_una, and (2)
dupacks for spurious retransmits below snd_una. I think the motivation
behind the "Hold old state" code is to stay in Recovery so that we do
not treat (2) dupacks as (1) dupacks.

I find RFC 2582 not very clear on this point, and the newest NewReno
RFC, RFC 6582, is also not so clear IMHO. But the RFC 3782 version of
NewReno - https://tools.ietf.org/html/rfc3782 - has a reasonable
discussion of this issue. There is a discussion in
https://tools.ietf.org/html/rfc3782#section-11
of this ambiguity:

   There are two separate scenarios in which the TCP sender could
   receive three duplicate acknowledgements acknowledging "recover" but
   no more than "recover".  One scenario would be that the data sender
   transmitted four packets with sequence numbers higher than "recover",
   that the first packet was dropped in the network, and the following
   three packets triggered three duplicate acknowledgements
   acknowledging "recover".  The second scenario would be that the
   sender unnecessarily retransmitted three packets below "recover", and
   that these three packets triggered three duplicate acknowledgements
   acknowledging "recover".  In the absence of SACK, the TCP sender is
   unable to distinguish between these two scenarios.

AFAICT RFC 3782 uses the term "recover" for the sequence number Linux
calls tp->high_seq. The specification in RFC 3782 Sec 3 -
https://tools.ietf.org/html/rfc3782#section-3 - of the criteria for
entering Fast Recovery say that we shouldn't go into a new recovery if
the cumulative ACK field doesn't cover more than high_seq/"recover":

   1)  Three duplicate ACKs:
       When the third duplicate ACK is received and the sender is not
       already in the Fast Recovery procedure, check to see if the
       Cumulative Acknowledgement field covers more than "recover".  If
       so, go to Step 1A.  Otherwise, go to Step 1B.

   1A) Invoking Fast Retransmit:
       ...
       In addition, record the highest sequence number transmitted in
       the variable "recover", and go to Step 2.

   1B) Not invoking Fast Retransmit:
       ...

The last few slides of this presentation by Sally Floyd also talk
about this point:

  http://www.icir.org/floyd/talks/newreno-Mar03.pdf

neal

Re: TCP NewReno and single retransmit

[Marcelo Ricardo Leitner]

On 03-11-2014 18:08, Neal Cardwell wrote:
> On Mon, Nov 3, 2014 at 11:38 AM, Marcelo Ricardo Leitner
> <mleitner@redhat.com> wrote:
>> On 31-10-2014 01:51, Yuchung Cheng wrote:
>>>>>>            if (tp->snd_una == tp->high_seq && tcp_is_reno(tp)) {
>>>>>>                    /* Hold old state until something *above* high_seq
>>>>>>                     * is ACKed. For Reno it is MUST to prevent false
>>>>>>                     * fast retransmits (RFC2582). SACK TCP is safe. */
>>>
>>> Or we can just remove this strange state-holding logic?
>>>
>>> I couldn't find such a "MUST" statement in RFC2582. RFC2582 section 3
>>> step 5 suggests exiting the recovery procedure when an ACK acknowledges
>>> the "recover" variable (== tp->high_seq - 1).
>>>
>>> Since we've called tcp_reset_reno_sack() before tcp_try_undo_recovery(),
>>> I couldn't see how false fast retransmits can be triggered without
>>> this state-holding.
>>>
>>> Any insights?
>>
>>
>> Nice one, me neither. Neal?
>
> Since there are no literal IETF-style "MUST" statements in RFC2582, I
> think the "MUST" in the code here is expressing the design philosophy
> behind the author. :-)
>
> AFAICT the "Hold old state" code in tcp_try_undo_recovery() is a
> pretty reasonable implementation of a mechanism specified in NewReno
> RFCs to deal with the fundamental ambiguity between (1) dupacks for
> packets the receiver received above a hole above snd_una, and (2)
> dupacks for spurious retransmits below snd_una. I think the motivation
> behind the "Hold old state" code is to stay in Recovery so that we do
> not treat (2) dupacks as (1) dupacks.
>
> I find RFC 2582 not very clear on this point, and the newest NewReno
> RFC, RFC 6582, is also not so clear IMHO. But the RFC 3782 version of
> NewReno - https://tools.ietf.org/html/rfc3782 - has a reasonable
> discussion of this issue. There is a discussion in
> https://tools.ietf.org/html/rfc3782#section-11
> of this ambiguity:
>
>     There are two separate scenarios in which the TCP sender could
>     receive three duplicate acknowledgements acknowledging "recover" but
>     no more than "recover".  One scenario would be that the data sender
>     transmitted four packets with sequence numbers higher than "recover",
>     that the first packet was dropped in the network, and the following
>     three packets triggered three duplicate acknowledgements
>     acknowledging "recover".  The second scenario would be that the
>     sender unnecessarily retransmitted three packets below "recover", and
>     that these three packets triggered three duplicate acknowledgements
>     acknowledging "recover".  In the absence of SACK, the TCP sender is
>     unable to distinguish between these two scenarios.
>
> AFAICT RFC 3782 uses the term "recover" for the sequence number Linux
> calls tp->high_seq. The specification in RFC 3782 Sec 3 -
> https://tools.ietf.org/html/rfc3782#section-3 - of the criteria for
> entering Fast Recovery say that we shouldn't go into a new recovery if
> the cumulative ACK field doesn't cover more than high_seq/"recover":
>
>     1)  Three duplicate ACKs:
>         When the third duplicate ACK is received and the sender is not
>         already in the Fast Recovery procedure, check to see if the
>         Cumulative Acknowledgement field covers more than "recover".  If
>         so, go to Step 1A.  Otherwise, go to Step 1B.
>
>     1A) Invoking Fast Retransmit:
>         ...
>         In addition, record the highest sequence number transmitted in
>         the variable "recover", and go to Step 2.
>
>     1B) Not invoking Fast Retransmit:
>         ...
>
> The last few slides of this presentation by Sally Floyd also talk
> about this point:
>
>    http://www.icir.org/floyd/talks/newreno-Mar03.pdf

So by sticking with the recovery for a bit longer will help disambiguate the 3 
dupacks on high_seq, if they ever happen, and with that avoid re-entering Fast 
Retransmit if initial (2) happen. It's at the cost of leaving the fast 
retransmit an ack later but if (2) happens the impact would be much worse, AFAICS.

Cool, thanks Neal. If Yuchung is okay with it, I'll proceed with just zeroing 
that tstamp as initially proposed.

Marcelo

Re: TCP NewReno and single retransmit

[Neal Cardwell]

On Mon, Nov 3, 2014 at 4:35 PM, Marcelo Ricardo Leitner
<mleitner@redhat.com> wrote:
> So by sticking with the recovery for a bit longer will help disambiguate the
> 3 dupacks on high_seq, if they ever happen, and with that avoid re-entering
> Fast Retransmit if initial (2) happen. It's at the cost of leaving the fast
> retransmit an ack later but if (2) happens the impact would be much worse,
> AFAICS.

Yes, that's my sense.

> Cool, thanks Neal. If Yuchung is okay with it, I'll proceed with just
> zeroing that tstamp as initially proposed.

Yes, that sounds good to me for a short-term fix for the "net" branch,
as long as it's:

+  if (!tcp_any_retrans_done(sk))
+    tp->retrans_stamp = 0;

Longer-term ("net-next"?) perhaps it makes sense to remove the hold
state and protect against this spurious FR situation at the time we
decide to enter Fast Recovery, which seems to be the model the RFC is
suggesting.

neal

Re: TCP NewReno and single retransmit

[Yuchung Cheng]

On Tue, Nov 4, 2014 at 7:17 AM, Neal Cardwell <ncardwell@google.com> wrote:
> On Mon, Nov 3, 2014 at 4:35 PM, Marcelo Ricardo Leitner
> <mleitner@redhat.com> wrote:
>> So by sticking with the recovery for a bit longer will help disambiguate the
>> 3 dupacks on high_seq, if they ever happen, and with that avoid re-entering
>> Fast Retransmit if initial (2) happen. It's at the cost of leaving the fast
>> retransmit an ack later but if (2) happens the impact would be much worse,
>> AFAICS.
>
> Yes, that's my sense.
>
>> Cool, thanks Neal. If Yuchung is okay with it, I'll proceed with just
>> zeroing that tstamp as initially proposed.
>
> Yes, that sounds good to me for a short-term fix for the "net" branch,
> as long as it's:
>
> +  if (!tcp_any_retrans_done(sk))
> +    tp->retrans_stamp = 0;
>
> Longer-term ("net-next"?) perhaps it makes sense to remove the hold
> state and protect against this spurious FR situation at the time we
> decide to enter Fast Recovery, which seems to be the model the RFC is
> suggesting.
Thanks for checking. So my suggested fix of removing the hold state is
the "less careful variant" that RFC does not recommend. I would rather
have the proposed 2-liner fix than implementing the section 6
heuristics to detect spurious retransmit. It's not worth the effort.
Everyone should use SACK.

>
> neal

RE: TCP NewReno and single retransmit

[David Laight]

> Since there are no literal IETF-style "MUST" statements in RFC2582, I
> think the "MUST" in the code here is expressing the design philosophy
> behind the author. :-)

I wouldn't guarantee that the authors of any RFC (or other
standards document) managed to cover all possible cases.

	David

Re: TCP NewReno and single retransmit

[Marcelo Ricardo Leitner]

On 04-11-2014 05:59, Yuchung Cheng wrote:
> On Tue, Nov 4, 2014 at 7:17 AM, Neal Cardwell <ncardwell@google.com> wrote:
>> On Mon, Nov 3, 2014 at 4:35 PM, Marcelo Ricardo Leitner
>> <mleitner@redhat.com> wrote:
>>> So by sticking with the recovery for a bit longer will help disambiguate the
>>> 3 dupacks on high_seq, if they ever happen, and with that avoid re-entering
>>> Fast Retransmit if initial (2) happen. It's at the cost of leaving the fast
>>> retransmit an ack later but if (2) happens the impact would be much worse,
>>> AFAICS.
>>
>> Yes, that's my sense.
>>
>>> Cool, thanks Neal. If Yuchung is okay with it, I'll proceed with just
>>> zeroing that tstamp as initially proposed.
>>
>> Yes, that sounds good to me for a short-term fix for the "net" branch,
>> as long as it's:
>>
>> +  if (!tcp_any_retrans_done(sk))
>> +    tp->retrans_stamp = 0;
>>
>> Longer-term ("net-next"?) perhaps it makes sense to remove the hold
>> state and protect against this spurious FR situation at the time we
>> decide to enter Fast Recovery, which seems to be the model the RFC is
>> suggesting.
> Thanks for checking. So my suggested fix of removing the hold state is
> the "less careful variant" that RFC does not recommend. I would rather
> have the proposed 2-liner fix than implementing the section 6
> heuristics to detect spurious retransmit. It's not worth the effort.
> Everyone should use SACK.

Yup, agreed.

Thanks,
Marcelo

Re: TCP NewReno and single retransmit

[Neal Cardwell]

On Tue, Nov 4, 2014 at 2:59 AM, Yuchung Cheng <ycheng@google.com> wrote:
> Thanks for checking. So my suggested fix of removing the hold state is
> the "less careful variant" that RFC does not recommend. I would rather
> have the proposed 2-liner fix than implementing the section 6
> heuristics to detect spurious retransmit. It's not worth the effort.
> Everyone should use SACK.

Agreed. The simple 2-liner seems like the simplest and lowest-risk
fix. And given that >95% of public Internet flows and an even higher
proportion of datacenter flows use SACK, it's not worth spending time
optimizing NewReno.

neal