One question about __tcp_select_window()

One question about __tcp_select_window()

[Wang Jian]

Hi all,

While I read __tcp_select_window() code, I find that it maybe return a
smaller window.
Below is one scenario I thought, may be not right:
In function __tcp_select_window(), assume:
full_space is 6mss, free_space is 2mss, tp->rcv_wnd is 3MSS.
And assume disable window scaling, then
window = tp->rcv_wnd > free_space && window > free_space
then it will round down free_space and return it.

Is this expected behavior? The comment is also saying
"Get the largest window that is a nice multiple of mss."

Should we do something like below ? Or I miss something?
I don't know how to verify it now.

--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -2680,9 +2680,9 @@ u32 __tcp_select_window(struct sock *sk)
                 * We also don't do any window rounding when the free space
                 * is too small.
                 */
-               if (window <= free_space - mss || window > free_space)
+               if (window <= free_space - mss)
                        window = rounddown(free_space, mss);
-               else if (mss == full_space &&
+               else if (window <= free_space && mss == full_space &&
                         free_space > window + (full_space >> 1))
                        window = free_space;
        }

Thanks.

Re: One question about __tcp_select_window()

[Wang Jian]

I test the fix with 4.17.0-rc1+ and it seems work.

1. iperf -c IP -i 20 -t 60 -w 1K
 with-fix vs without-fix : 1.15Gbits/sec vs 1.05Gbits/sec
I also try other windows and have similar results.

2. Use tcp probe trace snd_wind.
with-fix vs without-fix: 1245568 vs 1042816

3. I don't see extra retransmit/drops.

On Sun, Apr 15, 2018 at 8:50 PM, Wang Jian <jianjian.wang1@gmail.com> wrote:
> Hi all,
>
> While I read __tcp_select_window() code, I find that it maybe return a
> smaller window.
> Below is one scenario I thought, may be not right:
> In function __tcp_select_window(), assume:
> full_space is 6mss, free_space is 2mss, tp->rcv_wnd is 3MSS.
> And assume disable window scaling, then
> window = tp->rcv_wnd > free_space && window > free_space
> then it will round down free_space and return it.
>
> Is this expected behavior? The comment is also saying
> "Get the largest window that is a nice multiple of mss."
>
> Should we do something like below ? Or I miss something?
> I don't know how to verify it now.
>
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -2680,9 +2680,9 @@ u32 __tcp_select_window(struct sock *sk)
>                  * We also don't do any window rounding when the free space
>                  * is too small.
>                  */
> -               if (window <= free_space - mss || window > free_space)
> +               if (window <= free_space - mss)
>                         window = rounddown(free_space, mss);
> -               else if (mss == full_space &&
> +               else if (window <= free_space && mss == full_space &&
>                          free_space > window + (full_space >> 1))
>                         window = free_space;
>         }
>
> Thanks.

Re: One question about __tcp_select_window()

[Eric Dumazet]

On 04/17/2018 06:53 AM, Wang Jian wrote:
> I test the fix with 4.17.0-rc1+ and it seems work.
> 
> 1. iperf -c IP -i 20 -t 60 -w 1K
>  with-fix vs without-fix : 1.15Gbits/sec vs 1.05Gbits/sec
> I also try other windows and have similar results.
> 
> 2. Use tcp probe trace snd_wind.
> with-fix vs without-fix: 1245568 vs 1042816
> 
> 3. I don't see extra retransmit/drops.
> 

Unfortunately I have no idea what exact problem you had to solve.

Setting small windows is not exactly the path we are taking.

And I do not know how many side effects your change will have for 'standard' flows
using autotuning or sane windows.

Re: One question about __tcp_select_window()

[Wang Jian]

Thanks for your reply, Eric.

Actually, this is a query about the code while I am reading code.
>From my instinct and the comment, I think we should choose the bigger
one but maybe I miss something(like your said, autotuning)
Anyway, I will read more codes and do more tests.

Thanks.

On Tue, Apr 17, 2018 at 10:43 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
>
> On 04/17/2018 06:53 AM, Wang Jian wrote:
>> I test the fix with 4.17.0-rc1+ and it seems work.
>>
>> 1. iperf -c IP -i 20 -t 60 -w 1K
>>  with-fix vs without-fix : 1.15Gbits/sec vs 1.05Gbits/sec
>> I also try other windows and have similar results.
>>
>> 2. Use tcp probe trace snd_wind.
>> with-fix vs without-fix: 1245568 vs 1042816
>>
>> 3. I don't see extra retransmit/drops.
>>
>
> Unfortunately I have no idea what exact problem you had to solve.
>
> Setting small windows is not exactly the path we are taking.
>
> And I do not know how many side effects your change will have for 'standard' flows
> using autotuning or sane windows.
>