On Wed, Apr 4, 2018 at 6:35 AM Ilpo Järvinen <ilpo.jarvi...@helsinki.fi> wrote:
> On Wed, 28 Mar 2018, Yuchung Cheng wrote: > > 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; FWIW I'd vote for this version. > Of course I could put the back there but I really like the new place more > (which was a result of your suggestion to place the code elsewhere). > IMHO, it makes more sense to have it in tcp_clean_rtx_queue() because we > weren't successful in proving (there in tcp_clean_rtx_queue) that progress > was due original transmission and thus I would not want falsely indicate > it with that flag. And there's the non-SACK related block anyway already > there so it keeps the non-SACK "pollution" off from the SACK code paths. I think that's a compelling argument. In particular, in terms of long-term maintenance it seems risky to allow an unsound/buggy FLAG_ORIG_SACK_ACKED bit be returned by tcp_clean_rtx_queue(). If we return an incorrect/imcomplete FLAG_ORIG_SACK_ACKED bit then I worry that one day we will forget that for non-SACK flows that bit is incorrect/imcomplete, and we will add code using that bit but forgetting to check (tcp_is_sack(tp) || !FLAG_RETRANS_DATA_ACKED). > (In addition, I'd actually also like to rename FLAG_ORIG_SACK_ACKED to > FLAG_ORIG_PROGRESS, the latter is more descriptive about the condition > we're after regardless of SACK and less ambiguous in non-SACK case). I'm neutral on this. Not sure if the extra clarity is worth the code churn. cheers, neal
