| Message ID | 1321469885-10885-3-git-send-email-ncardwell@google.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On Wed, 16 Nov 2011, Neal Cardwell wrote: > The bug: When the ACK field is below snd_una (which can happen when > ACKs are reordered), senders ignored DSACKs (preventing undo) and did > not call tcp_fastretrans_alert, so they did not increment > prr_delivered to reflect newly-SACKed sequence ranges, and did not > call tcp_xmit_retransmit_queue, thus passing up chances to send out > more retransmitted and new packets based on any newly-SACKed packets. > > The change: When the ACK field is below snd_una (the "old_ack" goto > label), call tcp_fastretrans_alert to allow undo based on any > newly-arrived DSACKs and try to send out more packets based on > newly-SACKed packets. > > Other patches in this series will provide other changes that are > necessary to fully fix this problem. > > Signed-off-by: Neal Cardwell <ncardwell@google.com> > --- > net/ipv4/tcp_input.c | 10 +++++++--- > 1 files changed, 7 insertions(+), 3 deletions(-) > > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c > index b49e418..751d390 100644 > --- a/net/ipv4/tcp_input.c > +++ b/net/ipv4/tcp_input.c > @@ -3805,10 +3805,14 @@ invalid_ack: > return -1; > > old_ack: > + /* If data was SACKed, tag it and see if we should send more data. > + * If data was DSACKed, see if we can undo a cwnd reduction. > + */ > if (TCP_SKB_CB(skb)->sacked) { > - tcp_sacktag_write_queue(sk, skb, prior_snd_una); > - if (icsk->icsk_ca_state == TCP_CA_Open) > - tcp_try_keep_open(sk); > + flag |= tcp_sacktag_write_queue(sk, skb, prior_snd_una); > + newly_acked_sacked = tp->sacked_out - prior_sacked; > + tcp_fastretrans_alert(sk, pkts_acked, newly_acked_sacked, > + is_dupack, flag); I gave FLAG_* some thought and couldn't find something that would go wrong here (due to goto not all flag enablers are checked for). Acked-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi> ...unrelated to the fix, I realized that FRTO is not fully thought through in this old ACK case, also its RFC seems to lack considerations on what to do in such case. ...I'll need to think the FRTO stuff a bit more.
From: "Ilpo Järvinen" <ilpo.jarvinen@helsinki.fi> Date: Thu, 17 Nov 2011 03:52:37 +0200 (EET) > On Wed, 16 Nov 2011, Neal Cardwell wrote: > >> The bug: When the ACK field is below snd_una (which can happen when >> ACKs are reordered), senders ignored DSACKs (preventing undo) and did >> not call tcp_fastretrans_alert, so they did not increment >> prr_delivered to reflect newly-SACKed sequence ranges, and did not >> call tcp_xmit_retransmit_queue, thus passing up chances to send out >> more retransmitted and new packets based on any newly-SACKed packets. >> >> The change: When the ACK field is below snd_una (the "old_ack" goto >> label), call tcp_fastretrans_alert to allow undo based on any >> newly-arrived DSACKs and try to send out more packets based on >> newly-SACKed packets. >> >> Other patches in this series will provide other changes that are >> necessary to fully fix this problem. >> >> Signed-off-by: Neal Cardwell <ncardwell@google.com> ... > Acked-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi> Applied to net-next. > ...unrelated to the fix, I realized that FRTO is not fully thought through > in this old ACK case, also its RFC seems to lack considerations on what to > do in such case. ...I'll need to think the FRTO stuff a bit more. Every feature added to TCP beginning with timestamps has not been well thought out wrt. reordering. :-) -- 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
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c index b49e418..751d390 100644 --- a/net/ipv4/tcp_input.c +++ b/net/ipv4/tcp_input.c @@ -3805,10 +3805,14 @@ invalid_ack: return -1; old_ack: + /* If data was SACKed, tag it and see if we should send more data. + * If data was DSACKed, see if we can undo a cwnd reduction. + */ if (TCP_SKB_CB(skb)->sacked) { - tcp_sacktag_write_queue(sk, skb, prior_snd_una); - if (icsk->icsk_ca_state == TCP_CA_Open) - tcp_try_keep_open(sk); + flag |= tcp_sacktag_write_queue(sk, skb, prior_snd_una); + newly_acked_sacked = tp->sacked_out - prior_sacked; + tcp_fastretrans_alert(sk, pkts_acked, newly_acked_sacked, + is_dupack, flag); } SOCK_DEBUG(sk, "Ack %u before %u:%u\n", ack, tp->snd_una, tp->snd_nxt);
The bug: When the ACK field is below snd_una (which can happen when ACKs are reordered), senders ignored DSACKs (preventing undo) and did not call tcp_fastretrans_alert, so they did not increment prr_delivered to reflect newly-SACKed sequence ranges, and did not call tcp_xmit_retransmit_queue, thus passing up chances to send out more retransmitted and new packets based on any newly-SACKed packets. The change: When the ACK field is below snd_una (the "old_ack" goto label), call tcp_fastretrans_alert to allow undo based on any newly-arrived DSACKs and try to send out more packets based on newly-SACKed packets. Other patches in this series will provide other changes that are necessary to fully fix this problem. Signed-off-by: Neal Cardwell <ncardwell@google.com> --- net/ipv4/tcp_input.c | 10 +++++++--- 1 files changed, 7 insertions(+), 3 deletions(-)