[PATCH] tcp: Fix RFC reference in comment

[PATCH] tcp: Fix RFC reference in comment

[Debabrata Banerjee]

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)
 {
-- 
2.2.1

Re: [PATCH] tcp: Fix RFC reference in comment

[Yuchung Cheng]

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

Re: [PATCH] tcp: Fix RFC reference in comment

[Banerjee, Debabrata]

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

Re: [PATCH] tcp: Fix RFC reference in comment

[John Heffner]

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

Re: [PATCH] tcp: Fix RFC reference in comment

[Banerjee, Debabrata]

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