Bug #376

Fwd: [PATCH] tcp: fix infinite cwnd in tcp_complete_cwr()

Added by David Taht on Apr 28, 2012. Updated on Jun 24, 2014.
Closed High David Taht

Description

grumble

———- Forwarded message ———-
From: Yuchung Cheng ycheng@google.com
Date: Sat, Apr 28, 2012 at 12:04 PM
Subject: [PATCH] tcp: fix infinite cwnd in tcp_complete_cwr()
To: davem@davemloft.net, ilpo.jarvinen@helsinki.fi
Cc: ncardwell@google.com, nanditad@google.com, netdev@vger.kernel.org,
Yuchung Cheng ycheng@google.com

When the cwnd reduction is done, ssthresh may be infinite
if TCP enters CWR via ECN or F-RTO. If cwnd is not undone, i.e.,
undo_marker is set, tcp_complete_cwr() falsely set cwnd to the
infinite ssthresh value. The correct operation is to keep cwnd
intact because it has been updated in ECN or F-RTO.

Signed-off-by: Yuchung Cheng ycheng@google.com

 net/ipv4/tcp_input.c |    8 **+—
 1 files changed, 5 insertions(+), 3 deletions(-)

diff –git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index c93b0cb..ac417de 100644
— a/net/ipv4/tcp_input.c
**+ b/net/ipv4/tcp_input.c
@ -2868,11 +2868,13@ static inline void tcp_complete_cwr(struct sock *sk)

       /* Do not moderate cwnd if it’s already undone in cwr or recovery. */
       if (tp->undo_marker) {
-               if (inet_csk(sk)->icsk_ca_state TCP_CA_CWR) +               if (inet_csk(sk)->icsk_ca_state TCP_CA_CWR) {
                       tp->snd_cwnd = min(tp->snd_cwnd, tp->snd_ssthresh);
-               else /* PRR */
+               } else if (tp->snd_ssthresh < TCP_INFINITE_SSTHRESH) {
+                       /* PRR algorithm. */
                       tp->snd_cwnd = tp->snd_ssthresh;
-               tp->snd_cwnd_stamp = tcp_time_stamp;
+                       tp->snd_cwnd_stamp = tcp_time_stamp;
+               }
       }
       tcp_ca_event(sk, CA_EVENT_COMPLETE_CWR);
 }

History

Updated by Dave Täht on Apr 29, 2012.
———- Forwarded message ———-
From: Neal Cardwell ncardwell@google.com
Date: Sun, Apr 29, 2012 at 8:49 PM
Subject: Re: [PATCH] tcp: fix infinite cwnd in tcp_complete_cwr()
To: Yuchung Cheng ycheng@google.com
Cc: davem@davemloft.net, ilpo.jarvinen@helsinki.fi,
nanditad@google.com, netdev@vger.kernel.org

On Sat, Apr 28, 2012 at 3:04 PM, Yuchung Cheng ycheng@google.com wrote:

When the cwnd reduction is done, ssthresh may be infinite if TCP enters CWR via ECN or F-RTO. If cwnd is not undone, i.e., undo_marker is set, tcp_complete_cwr() falsely set cwnd to the infinite ssthresh value. The correct operation is to keep cwnd intact because it has been updated in ECN or F-RTO.
Signed-off-by: Yuchung Cheng ycheng@google.com net/ipv4/tcp_input.c | 8 — 1 files changed, 5 insertions(), 3 deletions(-)

diff -git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c index c93b0cb..ac417de 100644 – a/net/ipv4/tcp_input.c **+ b/net/ipv4/tcp_input.c @ -2868,11 +2868,13 @ static inline void tcp_complete_cwr(struct sock *sk)

/* Do not moderate cwnd if it’s already undone in cwr or recovery. */ if (tp->undo_marker) { - if (inet_csk(sk)icsk_ca_state TCP_CA_CWR) + if (inet_csk(sk)>icsk_ca_state TCP_CA_CWR) { tp->snd_cwnd = min(tp->snd_cwnd, tp->snd_ssthresh);

In this TCP_CA_CWR case the patch needs to add the assignment:

tp->snd_cwnd_stamp = tcp_time_stamp;

I know that was in earlier versions of your patch; looks like it just
got accidentally dropped on this latest version.

Otherwise, looks good to me.

Updated by David Taht on Apr 30, 2012.
final patch

———- Forwarded message ———-
From: Yuchung Cheng ycheng@google.com
Date: Mon, Apr 30, 2012 at 9:00 AM
Subject: [PATCH v2] tcp: fix infinite cwnd in tcp_complete_cwr()
To: davem@davemloft.net, ilpo.jarvinen@helsinki.fi, ncardwell@google.com
Cc: nanditad@google.com, netdev@vger.kernel.org, Yuchung Cheng
ycheng@google.com

When the cwnd reduction is done, ssthresh may be infinite
if TCP enters CWR via ECN or F-RTO. If cwnd is not undone, i.e.,
undo_marker is set, tcp_complete_cwr() falsely set cwnd to the
infinite ssthresh value. The correct operation is to keep cwnd
intact because it has been updated in ECN or F-RTO.

Signed-off-by: Yuchung Cheng ycheng@google.com

ChangeLog since v1:
 - Add snd_cwnd_stamp timestamping in CWR mode

 net/ipv4/tcp_input.c |    9 **—
 1 files changed, 6 insertions(+), 3 deletions(-)

diff –git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index c93b0cb..e0a9b89 100644
— a/net/ipv4/tcp_input.c
**+ b/net/ipv4/tcp_input.c
@ -2868,11 +2868,14@ static inline void tcp_complete_cwr(struct sock *sk)

       /* Do not moderate cwnd if it’s already undone in cwr or recovery. */
       if (tp->undo_marker) {
-               if (inet_csk(sk)->icsk_ca_state TCP_CA_CWR) +               if (inet_csk(sk)->icsk_ca_state TCP_CA_CWR) {
                       tp->snd_cwnd = min(tp->snd_cwnd, tp->snd_ssthresh);
-               else /* PRR */
+                       tp->snd_cwnd_stamp = tcp_time_stamp;
+               } else if (tp->snd_ssthresh < TCP_INFINITE_SSTHRESH) {
+                       /* PRR algorithm. */
                       tp->snd_cwnd = tp->snd_ssthresh;
-               tp->snd_cwnd_stamp = tcp_time_stamp;
+                       tp->snd_cwnd_stamp = tcp_time_stamp;
+               }
       }
       tcp_ca_event(sk, CA_EVENT_COMPLETE_CWR);
 }

Updated by Dave Täht on Jun 24, 2014.

This is a static export of the original bufferbloat.net issue database. As such, no further commenting is possible; the information is solely here for archival purposes.
RSS feed

Recent News & Articles

Sep 6, 2018 Wiki page
Pete Heist's Thoughts on ECN
Sep 5, 2018 Wiki page
Dave Taht's Stance on ECN
Sep 4, 2018 Wiki page
Jonathan Morton's Take on ECN
Sep 3, 2018 Wiki page
ECN-Sane Project
Aug 24, 2018 Wiki page
ECN-Sane Project

Find us elsewhere

Bufferbloat Mailing Lists
#bufferbloat on Twitter
Google+ group
Archived Bufferbloat pages from the Wayback Machine

Sponsors

Comcast Research Innovation Fund
Nlnet Foundation
Shuttleworth Foundation
GoFundMe

Bufferbloat Related Projects

Congestion Control Blog
Lede Project (OpenWrt)
Flent Network Test Suite
Sqm-Scripts
The Cake shaper
AQMs in BSD
IETF AQM WG

Network Performance Related Resources


Jim Gettys' Blog - The chairman of the Fjord
Toke's Blog - Karlstad University's work on bloat
Voip Users Conference - Weekly Videoconference mostly about voip
Candelatech - A wifi testing company that "gets it".