[PATCH] tcp: fix tcp_rcv_rtt_update() use of an unscaled RTT sample

[PATCH] tcp: fix tcp_rcv_rtt_update() use of an unscaled RTT sample

[Neal Cardwell]

Fix a code path in tcp_rcv_rtt_update() that was comparing scaled and
unscaled RTT samples.

The intent in the code was to only use the 'm' measurement if it was a
new minimum.  However, since 'm' had not yet been shifted left 3 bits
but 'new_sample' had, this comparison would nearly always succeed,
leading us to erroneously set our receive-side RTT estimate to the 'm'
sample when that sample could be nearly 8x too high to use.

The overall effect is to often cause the receive-side RTT estimate to
be significantly too large (up to 40% too large for brief periods in
my tests).

Signed-off-by: Neal Cardwell <ncardwell@google.com>
---
 net/ipv4/tcp_input.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index e886e2f..e7b54d2 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -474,8 +474,11 @@ static void tcp_rcv_rtt_update(struct tcp_sock *tp, u32 sample, int win_dep)
 		if (!win_dep) {
 			m -= (new_sample >> 3);
 			new_sample += m;
-		} else if (m < new_sample)
-			new_sample = m << 3;
+		} else {
+			m <<= 3;
+			if (m < new_sample)
+				new_sample = m;
+		}
 	} else {
 		/* No previous measure. */
 		new_sample = m << 3;
-- 
1.7.7.3

Re: [PATCH] tcp: fix tcp_rcv_rtt_update() use of an unscaled RTT sample

[Eric Dumazet]

On Tue, Apr 10, 2012 at 7:59 PM, Neal Cardwell <ncardwell@google.com> wrote:
> Fix a code path in tcp_rcv_rtt_update() that was comparing scaled and
> unscaled RTT samples.
>
> The intent in the code was to only use the 'm' measurement if it was a
> new minimum.  However, since 'm' had not yet been shifted left 3 bits
> but 'new_sample' had, this comparison would nearly always succeed,
> leading us to erroneously set our receive-side RTT estimate to the 'm'
> sample when that sample could be nearly 8x too high to use.
>
> The overall effect is to often cause the receive-side RTT estimate to
> be significantly too large (up to 40% too large for brief periods in
> my tests).
>
> Signed-off-by: Neal Cardwell <ncardwell@google.com>
> ---
>  net/ipv4/tcp_input.c |    7 +++++--
>  1 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index e886e2f..e7b54d2 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -474,8 +474,11 @@ static void tcp_rcv_rtt_update(struct tcp_sock *tp, u32 sample, int win_dep)
>                if (!win_dep) {
>                        m -= (new_sample >> 3);
>                        new_sample += m;
> -               } else if (m < new_sample)
> -                       new_sample = m << 3;
> +               } else {
> +                       m <<= 3;
> +                       if (m < new_sample)
> +                               new_sample = m;
> +               }
>        } else {
>                /* No previous measure. */
>                new_sample = m << 3;
> --
> 1.7.7.3
>

Acked-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH] tcp: fix tcp_rcv_rtt_update() use of an unscaled RTT sample

[David Miller]

From: Neal Cardwell <ncardwell@google.com>
Date: Tue, 10 Apr 2012 13:59:20 -0400

> Fix a code path in tcp_rcv_rtt_update() that was comparing scaled and
> unscaled RTT samples.
> 
> The intent in the code was to only use the 'm' measurement if it was a
> new minimum.  However, since 'm' had not yet been shifted left 3 bits
> but 'new_sample' had, this comparison would nearly always succeed,
> leading us to erroneously set our receive-side RTT estimate to the 'm'
> sample when that sample could be nearly 8x too high to use.
> 
> The overall effect is to often cause the receive-side RTT estimate to
> be significantly too large (up to 40% too large for brief periods in
> my tests).
> 
> Signed-off-by: Neal Cardwell <ncardwell@google.com>

Applied, thanks.

Re: [PATCH] tcp: fix tcp_rcv_rtt_update() use of an unscaled RTT sample

[Dave Taht]

On Tue, Apr 10, 2012 at 11:47 AM, David Miller <davem@davemloft.net> wrote:
> From: Neal Cardwell <ncardwell@google.com>
> Date: Tue, 10 Apr 2012 13:59:20 -0400
>
>> Fix a code path in tcp_rcv_rtt_update() that was comparing scaled and
>> unscaled RTT samples.
>>
>> The intent in the code was to only use the 'm' measurement if it was a
>> new minimum.  However, since 'm' had not yet been shifted left 3 bits
>> but 'new_sample' had, this comparison would nearly always succeed,
>> leading us to erroneously set our receive-side RTT estimate to the 'm'
>> sample when that sample could be nearly 8x too high to use.
>>
>> The overall effect is to often cause the receive-side RTT estimate to
>> be significantly too large (up to 40% too large for brief periods in
>> my tests).
>>
>> Signed-off-by: Neal Cardwell <ncardwell@google.com>
>
> Applied, thanks.
> --


awesome, is this needed in stable?

-- 
Dave Täht
SKYPE: davetaht
US Tel: 1-239-829-5608
http://www.bufferbloat.net

Re: [PATCH] tcp: fix tcp_rcv_rtt_update() use of an unscaled RTT sample

[David Miller]

From: Dave Taht <dave.taht@gmail.com>
Date: Tue, 10 Apr 2012 13:46:38 -0700

> awesome, is this needed in stable?

I have it queued up there already.

But you didn't need to write your email at all, you could
have checked this yourself:

http://patchwork.ozlabs.org/user/bundle/2566/?state=*

Re: [PATCH] tcp: fix tcp_rcv_rtt_update() use of an unscaled RTT sample

[Eric Dumazet]

On Tue, 2012-04-10 at 17:07 -0400, David Miller wrote:
> From: Dave Taht <dave.taht@gmail.com>
> Date: Tue, 10 Apr 2012 13:46:38 -0700
> 
> > awesome, is this needed in stable?
> 
> I have it queued up there already.
> 
> But you didn't need to write your email at all, you could
> have checked this yourself:
> 
> http://patchwork.ozlabs.org/user/bundle/2566/?state=*

Not sure this URL works (it doesnt for me)

Re: [PATCH] tcp: fix tcp_rcv_rtt_update() use of an unscaled RTT sample

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 10 Apr 2012 23:11:34 +0200

> On Tue, 2012-04-10 at 17:07 -0400, David Miller wrote:
>> From: Dave Taht <dave.taht@gmail.com>
>> Date: Tue, 10 Apr 2012 13:46:38 -0700
>> 
>> > awesome, is this needed in stable?
>> 
>> I have it queued up there already.
>> 
>> But you didn't need to write your email at all, you could
>> have checked this yourself:
>> 
>> http://patchwork.ozlabs.org/user/bundle/2566/?state=*
> 
> Not sure this URL works (it doesnt for me)

Sorry, that's my private URL and only works if you're logged
in as me :-)  This one is better:

http://patchwork.ozlabs.org/bundle/davem/stable/?state=*

Re: [PATCH] tcp: fix tcp_rcv_rtt_update() use of an unscaled RTT sample

[Eric Dumazet]

On Tue, 2012-04-10 at 17:16 -0400, David Miller wrote:

> Sorry, that's my private URL and only works if you're logged
> in as me :-)  This one is better:
> 
> http://patchwork.ozlabs.org/bundle/davem/stable/?state=*

Thanks, added to my bookmarks ;)

Re: [PATCH] tcp: fix tcp_rcv_rtt_update() use of an unscaled RTT sample

[Dave Taht]

On Tue, Apr 10, 2012 at 2:22 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Tue, 2012-04-10 at 17:16 -0400, David Miller wrote:
>
>> Sorry, that's my private URL and only works if you're logged
>> in as me :-)  This one is better:
>>
>> http://patchwork.ozlabs.org/bundle/davem/stable/?state=*
>
> Thanks, added to my bookmarks ;)

Mine too! I had no idea. Useful.

Is there an emacs interface? :)

>
>



-- 
Dave Täht
SKYPE: davetaht
US Tel: 1-239-829-5608
http://www.bufferbloat.net