* Why we discard all rtt samples when only some of the acked skbs have been retransmited in processing ack? @ 2013-09-17 4:01 LovelyLich 2013-09-17 5:11 ` Eric Dumazet 0 siblings, 1 reply; 4+ messages in thread From: LovelyLich @ 2013-09-17 4:01 UTC (permalink / raw) To: Eric Dumazet; +Cc: netdev Hi Eric, In tcp_clean_rtx_queue(), we set the flag FLAG_RETRANS_DATA_ACKED when we encounter one ever retransmited skb A. But if there is one( or more) skb B after this retransmited skb, and we calculate the rtt for skb B. The question is because we have set the flag FLAG_RETRANS_DATA_ACKED, and we will just return in tcp_ack_no_tstamp() ! Two questions: 1. if we will just ignore all packets in this ack, we do not need to calculate skb B's rtt sample. 2. what I want to know, even if A's rtt sample is not reliable, but B's rtt sample can be trusted. Why we discard it ? Thanks in advanced. regards, Yi ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: Why we discard all rtt samples when only some of the acked skbs have been retransmited in processing ack? 2013-09-17 4:01 Why we discard all rtt samples when only some of the acked skbs have been retransmited in processing ack? LovelyLich @ 2013-09-17 5:11 ` Eric Dumazet 2013-09-17 13:31 ` Neal Cardwell 0 siblings, 1 reply; 4+ messages in thread From: Eric Dumazet @ 2013-09-17 5:11 UTC (permalink / raw) To: LovelyLich, Yuchung Cheng; +Cc: netdev On Tue, 2013-09-17 at 12:01 +0800, LovelyLich wrote: > Hi Eric, > > In tcp_clean_rtx_queue(), we set the flag FLAG_RETRANS_DATA_ACKED when we > > encounter one ever retransmited skb A. But if there is one( or more) skb B > > after this retransmited skb, and we calculate the rtt for skb B. The question > > is because we have set the flag FLAG_RETRANS_DATA_ACKED, and we will just > > return in tcp_ack_no_tstamp() ! > > Two questions: > > 1. if we will just ignore all packets in this ack, we do not need to calculate > > skb B's rtt sample. > > 2. what I want to know, even if A's rtt sample is not reliable, but B's rtt > > sample can be trusted. Why we discard it ? > > > > Thanks in advanced. > Good point ! Yuchung, what do you think of following patch ? diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c index 25a89ea..7f12b96 100644 --- a/net/ipv4/tcp_input.c +++ b/net/ipv4/tcp_input.c @@ -2971,7 +2971,7 @@ static int tcp_clean_rtx_queue(struct sock *sk, int prior_fackets, struct sk_buff *skb; u32 now = tcp_time_stamp; int fully_acked = true; - int flag = 0; + int flag = FLAG_RETRANS_DATA_ACKED; u32 pkts_acked = 0; u32 reord = tp->packets_out; u32 prior_sacked = tp->sacked_out; @@ -3002,7 +3002,6 @@ static int tcp_clean_rtx_queue(struct sock *sk, int prior_fackets, if (sacked & TCPCB_RETRANS) { if (sacked & TCPCB_SACKED_RETRANS) tp->retrans_out -= acked_pcount; - flag |= FLAG_RETRANS_DATA_ACKED; } else { ca_seq_rtt = now - scb->when; last_ackt = skb->tstamp; @@ -3013,6 +3012,7 @@ static int tcp_clean_rtx_queue(struct sock *sk, int prior_fackets, reord = min(pkts_acked, reord); if (!after(scb->end_seq, tp->high_seq)) flag |= FLAG_ORIG_SACK_ACKED; + flag &= ~FLAG_RETRANS_DATA_ACKED; } if (sacked & TCPCB_SACKED_ACKED) ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: Why we discard all rtt samples when only some of the acked skbs have been retransmited in processing ack? 2013-09-17 5:11 ` Eric Dumazet @ 2013-09-17 13:31 ` Neal Cardwell 2013-09-17 14:53 ` Yuchung Cheng 0 siblings, 1 reply; 4+ messages in thread From: Neal Cardwell @ 2013-09-17 13:31 UTC (permalink / raw) To: Eric Dumazet; +Cc: LovelyLich, Yuchung Cheng, Netdev On Tue, Sep 17, 2013 at 1:11 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > On Tue, 2013-09-17 at 12:01 +0800, LovelyLich wrote: >> Hi Eric, >> >> In tcp_clean_rtx_queue(), we set the flag FLAG_RETRANS_DATA_ACKED when we >> >> encounter one ever retransmited skb A. But if there is one( or more) skb B >> >> after this retransmited skb, and we calculate the rtt for skb B. The question >> >> is because we have set the flag FLAG_RETRANS_DATA_ACKED, and we will just >> >> return in tcp_ack_no_tstamp() ! >> >> Two questions: >> >> 1. if we will just ignore all packets in this ack, we do not need to calculate >> >> skb B's rtt sample. >> >> 2. what I want to know, even if A's rtt sample is not reliable, but B's rtt >> >> sample can be trusted. Why we discard it ? >> >> >> >> Thanks in advanced. >> > > Good point ! > > Yuchung, what do you think of following patch ? > > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c > index 25a89ea..7f12b96 100644 > --- a/net/ipv4/tcp_input.c > +++ b/net/ipv4/tcp_input.c > @@ -2971,7 +2971,7 @@ static int tcp_clean_rtx_queue(struct sock *sk, int prior_fackets, > struct sk_buff *skb; > u32 now = tcp_time_stamp; > int fully_acked = true; > - int flag = 0; > + int flag = FLAG_RETRANS_DATA_ACKED; > u32 pkts_acked = 0; > u32 reord = tp->packets_out; > u32 prior_sacked = tp->sacked_out; > @@ -3002,7 +3002,6 @@ static int tcp_clean_rtx_queue(struct sock *sk, int prior_fackets, > if (sacked & TCPCB_RETRANS) { > if (sacked & TCPCB_SACKED_RETRANS) > tp->retrans_out -= acked_pcount; > - flag |= FLAG_RETRANS_DATA_ACKED; > } else { > ca_seq_rtt = now - scb->when; > last_ackt = skb->tstamp; > @@ -3013,6 +3012,7 @@ static int tcp_clean_rtx_queue(struct sock *sk, int prior_fackets, > reord = min(pkts_acked, reord); > if (!after(scb->end_seq, tp->high_seq)) > flag |= FLAG_ORIG_SACK_ACKED; > + flag &= ~FLAG_RETRANS_DATA_ACKED; > } > > if (sacked & TCPCB_SACKED_ACKED) I think the existing logic is better than the patch. If we get a cumulative ACK that covers a retransmitted packet, then any RTT sample we try to extract is suspect, and likely to be at least 2x too high. Consider the following common scenario: t=0: send pkts 1, 2, 3, 4 t=1*RTT: receive dupack with SACK for pkts 2,3,4; fast retransmit pkt 1 t=2*RTT: receive cumulative ack for all pkts through 4 With the existing logic, because the ACK we get at t=2*RTT covers the retransmitted pkt 1, we do not attempt to take an RTT sample. With that proposed patch, when we get the ACK at t=2*RTT we see that there are non-retransmitted pkts 2,3,4 being ACKed, so we clear the FLAG_RETRANS_DATA_ACKED bit and take an RTT sample of 2*RTT. But this is 2x too big, and will distort our RTT sample. Note that with Yuchung's recent patch to gather RTT samples from SACKed packets (59c9af4234b0c21a1ed05cf65bf014d0c1a67bfd "tcp: measure RTT from new SACK"), we will already be extracting essentially all the RTT samples that we possibly can out of such scenarios with retransmitted packets (for OSes that support SACK, which is basically everyone). In the example above, Yuchung's new SACK-based RTT scheme will correctly take RTT samples at t=1*RTT for the SACKed packets. neal ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: Why we discard all rtt samples when only some of the acked skbs have been retransmited in processing ack? 2013-09-17 13:31 ` Neal Cardwell @ 2013-09-17 14:53 ` Yuchung Cheng 0 siblings, 0 replies; 4+ messages in thread From: Yuchung Cheng @ 2013-09-17 14:53 UTC (permalink / raw) To: Neal Cardwell; +Cc: Eric Dumazet, LovelyLich, Netdev On Tue, Sep 17, 2013 at 6:31 AM, Neal Cardwell <ncardwell@google.com> wrote: > On Tue, Sep 17, 2013 at 1:11 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote: >> On Tue, 2013-09-17 at 12:01 +0800, LovelyLich wrote: >>> Hi Eric, >>> >>> In tcp_clean_rtx_queue(), we set the flag FLAG_RETRANS_DATA_ACKED when we >>> >>> encounter one ever retransmited skb A. But if there is one( or more) skb B >>> >>> after this retransmited skb, and we calculate the rtt for skb B. The question >>> >>> is because we have set the flag FLAG_RETRANS_DATA_ACKED, and we will just >>> >>> return in tcp_ack_no_tstamp() ! >>> >>> Two questions: >>> >>> 1. if we will just ignore all packets in this ack, we do not need to calculate >>> >>> skb B's rtt sample. >>> >>> 2. what I want to know, even if A's rtt sample is not reliable, but B's rtt >>> >>> sample can be trusted. Why we discard it ? >>> >>> >>> >>> Thanks in advanced. >>> >> >> Good point ! >> >> Yuchung, what do you think of following patch ? >> >> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c >> index 25a89ea..7f12b96 100644 >> --- a/net/ipv4/tcp_input.c >> +++ b/net/ipv4/tcp_input.c >> @@ -2971,7 +2971,7 @@ static int tcp_clean_rtx_queue(struct sock *sk, int prior_fackets, >> struct sk_buff *skb; >> u32 now = tcp_time_stamp; >> int fully_acked = true; >> - int flag = 0; >> + int flag = FLAG_RETRANS_DATA_ACKED; >> u32 pkts_acked = 0; >> u32 reord = tp->packets_out; >> u32 prior_sacked = tp->sacked_out; >> @@ -3002,7 +3002,6 @@ static int tcp_clean_rtx_queue(struct sock *sk, int prior_fackets, >> if (sacked & TCPCB_RETRANS) { >> if (sacked & TCPCB_SACKED_RETRANS) >> tp->retrans_out -= acked_pcount; >> - flag |= FLAG_RETRANS_DATA_ACKED; >> } else { >> ca_seq_rtt = now - scb->when; >> last_ackt = skb->tstamp; >> @@ -3013,6 +3012,7 @@ static int tcp_clean_rtx_queue(struct sock *sk, int prior_fackets, >> reord = min(pkts_acked, reord); >> if (!after(scb->end_seq, tp->high_seq)) >> flag |= FLAG_ORIG_SACK_ACKED; >> + flag &= ~FLAG_RETRANS_DATA_ACKED; >> } >> >> if (sacked & TCPCB_SACKED_ACKED) > > I think the existing logic is better than the patch. If we get a > cumulative ACK that covers a retransmitted packet, then any RTT sample > we try to extract is suspect, and likely to be at least 2x too high. > > Consider the following common scenario: > > t=0: send pkts 1, 2, 3, 4 > t=1*RTT: receive dupack with SACK for pkts 2,3,4; fast retransmit pkt 1 > t=2*RTT: receive cumulative ack for all pkts through 4 > > With the existing logic, because the ACK we get at t=2*RTT covers the > retransmitted pkt 1, we do not attempt to take an RTT sample. > > With that proposed patch, when we get the ACK at t=2*RTT we see that > there are non-retransmitted pkts 2,3,4 being ACKed, so we clear the > FLAG_RETRANS_DATA_ACKED bit and take an RTT sample of 2*RTT. But this > is 2x too big, and will distort our RTT sample. Yes I completely agree with Neal's analysis. In fact I've considered the proposed change when I implement the patch quoted below, and later realized it actually breaks Karn's algorithm: to properly take an RTT sample w/o ambiguity, all, not just some, of the packets cumulatively acked must not been retransmitted. > > Note that with Yuchung's recent patch to gather RTT samples from > SACKed packets (59c9af4234b0c21a1ed05cf65bf014d0c1a67bfd "tcp: measure > RTT from new SACK"), we will already be extracting essentially all the > RTT samples that we possibly can out of such scenarios with > retransmitted packets (for OSes that support SACK, which is basically > everyone). In the example above, Yuchung's new SACK-based RTT scheme > will correctly take RTT samples at t=1*RTT for the SACKed packets. > > neal ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2013-09-17 14:53 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-09-17 4:01 Why we discard all rtt samples when only some of the acked skbs have been retransmited in processing ack? LovelyLich 2013-09-17 5:11 ` Eric Dumazet 2013-09-17 13:31 ` Neal Cardwell 2013-09-17 14:53 ` Yuchung Cheng
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).