Message ID | 1421183431-29915-1-git-send-email-dbanerje@akamai.com |
---|---|
State | Rejected, archived |
Delegated to: | David Miller |
Headers | show |
On Tue, Jan 13, 2015 at 1:10 PM, Debabrata Banerjee <dbanerje@akamai.com> wrote: > > Comment in tcp_cwnd_restart() was referencing the wrong RFC for the algorithm > it's implementing. > > Signed-off-by: Debabrata Banerjee <dbanerje@akamai.com> > --- > net/ipv4/tcp_output.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c > index 65caf8b..0c13f88 100644 > --- a/net/ipv4/tcp_output.c > +++ b/net/ipv4/tcp_output.c > @@ -139,7 +139,7 @@ static __u16 tcp_advertise_mss(struct sock *sk) > return (__u16)mss; > } > > -/* RFC2861. Reset CWND after idle period longer RTO to "restart window". > +/* RFC2581 4.1. Reset CWND after idle period longer RTO to "restart window". > * This is the first part of cwnd validation mechanism. */ > static void tcp_cwnd_restart(struct sock *sk, const struct dst_entry *dst) > { RFC2861 resets the cwnd like in RFC2581, but the rest of the code implements RFC2861. So I think the current comment is fine. > > -- > 2.2.1 > > -- > 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 -- 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
On 1/13/15, 4:36 PM, "Yuchung Cheng" <ycheng@google.com> wrote: >On Tue, Jan 13, 2015 at 1:10 PM, Debabrata Banerjee <dbanerje@akamai.com> >wrote: >> >> -/* RFC2861. Reset CWND after idle period longer RTO to "restart >>window". >> +/* RFC2581 4.1. Reset CWND after idle period longer RTO to "restart >>window". >> * This is the first part of cwnd validation mechanism. */ >> static void tcp_cwnd_restart(struct sock *sk, const struct dst_entry >>*dst) >> { > >RFC2861 resets the cwnd like in RFC2581, but the rest of the code >implements RFC2861. So I think the current comment is fine. No RFC2861 is an experimental RFC that's implemented in tcp_cwnd_application_limited(). RFC2861 Recommends reducing the cwnd by averaging the current cwnd and the used cwnd as the new cwnd. RFC2581 4.1 Says to set cwnd to initial cwnd if more than one rto has passed since the last send. This is what is implemented in the function above. -Debabrata -- 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
On Tue, Jan 13, 2015 at 4:42 PM, Banerjee, Debabrata <dbanerje@akamai.com> wrote: > On 1/13/15, 4:36 PM, "Yuchung Cheng" <ycheng@google.com> wrote: > >>On Tue, Jan 13, 2015 at 1:10 PM, Debabrata Banerjee <dbanerje@akamai.com> >>wrote: >>> >>> -/* RFC2861. Reset CWND after idle period longer RTO to "restart >>>window". >>> +/* RFC2581 4.1. Reset CWND after idle period longer RTO to "restart >>>window". >>> * This is the first part of cwnd validation mechanism. */ >>> static void tcp_cwnd_restart(struct sock *sk, const struct dst_entry >>>*dst) >>> { >> >>RFC2861 resets the cwnd like in RFC2581, but the rest of the code >>implements RFC2861. So I think the current comment is fine. > > > No RFC2861 is an experimental RFC that's implemented in > tcp_cwnd_application_limited(). RFC2861 Recommends reducing the cwnd by > averaging the current cwnd and the used cwnd as the new cwnd. > > > RFC2581 4.1 Says to set cwnd to initial cwnd if more than one rto has > passed since the last send. This is what is implemented in the function > above. Look at the code a little closer -- it's decaying cwnd based on number of timeouts as described in 2861, not resetting to IW as recommended in 2581. -John -- 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
On 1/13/15, 5:01 PM, "John Heffner" <johnwheffner@gmail.com> wrote: >On Tue, Jan 13, 2015 at 4:42 PM, Banerjee, Debabrata ><dbanerje@akamai.com> wrote: >> On 1/13/15, 4:36 PM, "Yuchung Cheng" <ycheng@google.com> wrote: >> >>>RFC2861 resets the cwnd like in RFC2581, but the rest of the code >>>implements RFC2861. So I think the current comment is fine. >> >> >> No RFC2861 is an experimental RFC that's implemented in >> tcp_cwnd_application_limited(). RFC2861 Recommends reducing the cwnd by >> averaging the current cwnd and the used cwnd as the new cwnd. >> >> >> RFC2581 4.1 Says to set cwnd to initial cwnd if more than one rto has >> passed since the last send. This is what is implemented in the function >> above. > >Look at the code a little closer -- it's decaying cwnd based on number >of timeouts as described in 2861, not resetting to IW as recommended >in 2581. > > -John You're right it's not RFC2581 I was partially misled by the comment (reset/restart window), but it doesn't appear to be doing what rfc2861 3.2 says either: For i=1 To (tcpnow - T_last)/RTO win = min(cwnd, receiver's declared max window) cwnd = max(win/2, MSS) Versus: u32 restart_cwnd = tcp_init_cwnd(tp, dst); restart_cwnd = min(restart_cwnd, cwnd); while ((delta -= inet_csk(sk)->icsk_rto) > 0 && cwnd > restart_cwnd) cwnd >>= 1; tp->snd_cwnd = max(cwnd, restart_cwnd); It's not using receiver window, it's using cwnd/init_cwnd, it should at least be using tp->snd_wnd, no?. I stumbled onto this because it looks like tcp_cwnd_application_limited() doesn't execute when it should, because tp->snd_cwnd_stamp is being touched much more often than in rfc2861 3.2. Something seems not right here... -Debabrata -- 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_output.c b/net/ipv4/tcp_output.c index 65caf8b..0c13f88 100644 --- a/net/ipv4/tcp_output.c +++ b/net/ipv4/tcp_output.c @@ -139,7 +139,7 @@ static __u16 tcp_advertise_mss(struct sock *sk) return (__u16)mss; } -/* RFC2861. Reset CWND after idle period longer RTO to "restart window". +/* RFC2581 4.1. Reset CWND after idle period longer RTO to "restart window". * This is the first part of cwnd validation mechanism. */ static void tcp_cwnd_restart(struct sock *sk, const struct dst_entry *dst) {
Comment in tcp_cwnd_restart() was referencing the wrong RFC for the algorithm it's implementing. Signed-off-by: Debabrata Banerjee <dbanerje@akamai.com> --- net/ipv4/tcp_output.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)