On Tue, Mar 13, 2018 at 3:25 AM, Ilpo Järvinen
<ilpo.jarvi...@helsinki.fi> wrote:
>
> If SACK is not enabled and the first cumulative ACK after the RTO
> retransmission covers more than the retransmitted skb, a spurious
> FRTO undo will trigger (assuming FRTO is enabled for that RTO).
> The reason is that any non-retransmitted segment acknowledged will
> set FLAG_ORIG_SACK_ACKED in tcp_clean_rtx_queue even if there is
> no indication that it would have been delivered for real (the
> scoreboard is not kept with TCPCB_SACKED_ACKED bits in the non-SACK
> case so the check for that bit won't help like it does with SACK).
> Having FLAG_ORIG_SACK_ACKED set results in the spurious FRTO undo
> in tcp_process_loss.
>
> We need to use more strict condition for non-SACK case and check
> that none of the cumulatively ACKed segments were retransmitted
> to prove that progress is due to original transmissions. Only then
> keep FLAG_ORIG_SACK_ACKED set, allowing FRTO undo to proceed in
> non-SACK case.
>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvi...@helsinki.fi>
> ---
> net/ipv4/tcp_input.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index 4a26c09..c60745c 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -3166,6 +3166,15 @@ static int tcp_clean_rtx_queue(struct sock *sk, u32
> prior_fack,
> pkts_acked = rexmit_acked + newdata_acked;
>
> tcp_remove_reno_sacks(sk, pkts_acked);
> +
> + /* If any of the cumulatively ACKed segments was
> + * retransmitted, non-SACK case cannot confirm that
> + * progress was due to original transmission due to
> + * lack of TCPCB_SACKED_ACKED bits even if some of
> + * the packets may have been never retransmitted.
> + */
> + if (flag & FLAG_RETRANS_DATA_ACKED)
> + flag &= ~FLAG_ORIG_SACK_ACKED;
How about keeping your excellent comment but move the fix to F-RTO
code directly so it's more clear? this way the flag remains clear that
indicates some never-retransmitted data are acked/sacked.
// pseudo code for illustration
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 8d480542aa07..f7f3357de618 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -2629,8 +2629,15 @@ static void tcp_process_loss(struct sock *sk,
int flag, bool is_dupack,
if (tp->frto) { /* F-RTO RFC5682 sec 3.1 (sack enhanced version). */
/* Step 3.b. A timeout is spurious if not all data are
* lost, i.e., never-retransmitted data are (s)acked.
+ *
+ * If any of the cumulatively ACKed segments was
+ * retransmitted, non-SACK case cannot confirm that
+ * progress was due to original transmission due to
+ * lack of TCPCB_SACKED_ACKED bits even if some of
+ * the packets may have been never retransmitted.
*/
if ((flag & FLAG_ORIG_SACK_ACKED) &&
+ (tcp_is_sack(tp) || !FLAG_RETRANS_DATA_ACKED) &&
tcp_try_undo_loss(sk, true))
return;
> } else {
> int delta;
>
> --
> 2.7.4
>
