[PATCH net] tcp: Fix TLP implementation in case receive window limits send

[PATCH net] tcp: Fix TLP implementation in case receive window limits send

[Itzcak Pechtalt]

From: Itzcak Pechtalt <itzcak@flashnetworks.com>

TCP Tail loss probe (TLP) algorithm implementation has some problem.
According to paper (draft-dukkipati-tcpm-tcp-loss-probe-0 ):
In case recive window of receiver limits send of new packet in probe time than
a retransmit of last packet send should be done.

Actually, return code from tcp_write_xmit is not checked and only RTO is
scheduled,
So, it will take more time for reovery in this case than without TLP.

Signed-off-by: Itzcak Pechtalt <itzcak@flashnetworks.com>
---
net/ipv4/tcp_output.c  |    2 +-
1 file changed, 1 insertions(+),  1 deletions(-)

diff -up a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
--- a/net/ipv4/tcp_output.c     2014-09-23 18:35:37.113771694 +0300
+++ b/net/ipv4/tcp_output.c     2014-09-23 18:39:58.207726420 +0300
@@ -2100,7 +2100,8 @@ void tcp_send_loss_probe(struct sock *sk

        if (tcp_send_head(sk) != NULL) {
                err = tcp_write_xmit(sk, mss, TCP_NAGLE_OFF, 2, GFP_ATOMIC);
-              goto rearm_timer;
+             if (!err)
+                       goto rearm_timer;
        }

        /* At most one outstanding TLP retransmission. */

Re: [PATCH net] tcp: Fix TLP implementation in case receive window limits send

[Yuchung Cheng]

On Tue, Sep 23, 2014 at 9:28 AM, Itzcak Pechtalt
<itzcak@flashnetworks.com> wrote:
> From: Itzcak Pechtalt <itzcak@flashnetworks.com>
>
> TCP Tail loss probe (TLP) algorithm implementation has some problem.
> According to paper (draft-dukkipati-tcpm-tcp-loss-probe-0 ):
> In case recive window of receiver limits send of new packet in probe time than
> a retransmit of last packet send should be done.
>
> Actually, return code from tcp_write_xmit is not checked and only RTO is
> scheduled,
> So, it will take more time for reovery in this case than without TLP.
>
> Signed-off-by: Itzcak Pechtalt <itzcak@flashnetworks.com>
> ---
> net/ipv4/tcp_output.c  |    2 +-
> 1 file changed, 1 insertions(+),  1 deletions(-)
>
> diff -up a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> --- a/net/ipv4/tcp_output.c     2014-09-23 18:35:37.113771694 +0300
> +++ b/net/ipv4/tcp_output.c     2014-09-23 18:39:58.207726420 +0300
> @@ -2100,7 +2100,8 @@ void tcp_send_loss_probe(struct sock *sk
>
>         if (tcp_send_head(sk) != NULL) {
>                 err = tcp_write_xmit(sk, mss, TCP_NAGLE_OFF, 2, GFP_ATOMIC);
> -              goto rearm_timer;
Doesn't tcp_write_xmit() return true if tcp_snd_wnd_test() fails? not
sure I understand the fix.

> +             if (!err)
> +                       goto rearm_timer;
>         }
>
>         /* At most one outstanding TLP retransmission. */
>
> --
> 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 net] tcp: Fix TLP implementation in case receive window limits send

[Yuchung Cheng]

On Tue, Sep 23, 2014 at 10:52 AM, Yuchung Cheng <ycheng@google.com> wrote:
> On Tue, Sep 23, 2014 at 9:28 AM, Itzcak Pechtalt
> <itzcak@flashnetworks.com> wrote:
>> From: Itzcak Pechtalt <itzcak@flashnetworks.com>
>>
>> TCP Tail loss probe (TLP) algorithm implementation has some problem.
>> According to paper (draft-dukkipati-tcpm-tcp-loss-probe-0 ):
>> In case recive window of receiver limits send of new packet in probe time than
>> a retransmit of last packet send should be done.
>>
>> Actually, return code from tcp_write_xmit is not checked and only RTO is
>> scheduled,
>> So, it will take more time for reovery in this case than without TLP.
>>
>> Signed-off-by: Itzcak Pechtalt <itzcak@flashnetworks.com>
>> ---
>> net/ipv4/tcp_output.c  |    2 +-
>> 1 file changed, 1 insertions(+),  1 deletions(-)
>>
>> diff -up a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
>> --- a/net/ipv4/tcp_output.c     2014-09-23 18:35:37.113771694 +0300
>> +++ b/net/ipv4/tcp_output.c     2014-09-23 18:39:58.207726420 +0300
>> @@ -2100,7 +2100,8 @@ void tcp_send_loss_probe(struct sock *sk
>>
>>         if (tcp_send_head(sk) != NULL) {
>>                 err = tcp_write_xmit(sk, mss, TCP_NAGLE_OFF, 2, GFP_ATOMIC);
>> -              goto rearm_timer;
> Doesn't tcp_write_xmit() return true if tcp_snd_wnd_test() fails? not
> sure I understand the fix.
Oops I read an old version of the code. Nevermind...

>
>> +             if (!err)
>> +                       goto rearm_timer;
>>         }
>>
>>         /* At most one outstanding TLP retransmission. */
>>
>> --
>> 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 net] tcp: Fix TLP implementation in case receive window limits send

[Yuchung Cheng]

On Tue, Sep 23, 2014 at 9:28 AM, Itzcak Pechtalt
<itzcak@flashnetworks.com> wrote:
> From: Itzcak Pechtalt <itzcak@flashnetworks.com>
>
> TCP Tail loss probe (TLP) algorithm implementation has some problem.
> According to paper (draft-dukkipati-tcpm-tcp-loss-probe-0 ):
> In case recive window of receiver limits send of new packet in probe time than
> a retransmit of last packet send should be done.
>
> Actually, return code from tcp_write_xmit is not checked and only RTO is
> scheduled,
> So, it will take more time for reovery in this case than without TLP.
>
> Signed-off-by: Itzcak Pechtalt <itzcak@flashnetworks.com>
Acked-by: Yuchung Cheng <ycheng@google.com>
> ---
> net/ipv4/tcp_output.c  |    2 +-
> 1 file changed, 1 insertions(+),  1 deletions(-)
>
> diff -up a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> --- a/net/ipv4/tcp_output.c     2014-09-23 18:35:37.113771694 +0300
> +++ b/net/ipv4/tcp_output.c     2014-09-23 18:39:58.207726420 +0300
> @@ -2100,7 +2100,8 @@ void tcp_send_loss_probe(struct sock *sk
>
>         if (tcp_send_head(sk) != NULL) {
>                 err = tcp_write_xmit(sk, mss, TCP_NAGLE_OFF, 2, GFP_ATOMIC);
> -              goto rearm_timer;
> +             if (!err)
> +                       goto rearm_timer;
>         }
>
>         /* At most one outstanding TLP retransmission. */
>
> --
> 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 net] tcp: Fix TLP implementation in case receive window limits send

[Nandita Dukkipati]

> On Tue, Sep 23, 2014 at 9:28 AM, Itzcak Pechtalt
> <itzcak@flashnetworks.com> wrote:
>> From: Itzcak Pechtalt <itzcak@flashnetworks.com>
>>
>> TCP Tail loss probe (TLP) algorithm implementation has some problem.
>> According to paper (draft-dukkipati-tcpm-tcp-loss-probe-0 ):
>> In case recive window of receiver limits send of new packet in probe time than
>> a retransmit of last packet send should be done.
>>
>> Actually, return code from tcp_write_xmit is not checked and only RTO is
>> scheduled,
>> So, it will take more time for reovery in this case than without TLP.
>>
>> Signed-off-by: Itzcak Pechtalt <itzcak@flashnetworks.com>
> Acked-by: Yuchung Cheng <ycheng@google.com>
Acked-by: Nandita Dukkipati <nanditad@google.com>

Re: [PATCH net] tcp: Fix TLP implementation in case receive window limits send

[Neal Cardwell]

On Wed, Sep 24, 2014 at 3:00 PM, Nandita Dukkipati <nanditad@google.com> wrote:
>> On Tue, Sep 23, 2014 at 9:28 AM, Itzcak Pechtalt
>> <itzcak@flashnetworks.com> wrote:
>>> From: Itzcak Pechtalt <itzcak@flashnetworks.com>
>>>
>>> TCP Tail loss probe (TLP) algorithm implementation has some problem.
>>> According to paper (draft-dukkipati-tcpm-tcp-loss-probe-0 ):
>>> In case recive window of receiver limits send of new packet in probe time than
>>> a retransmit of last packet send should be done.
>>>
>>> Actually, return code from tcp_write_xmit is not checked and only RTO is
>>> scheduled,
>>> So, it will take more time for reovery in this case than without TLP.
>>>
>>> Signed-off-by: Itzcak Pechtalt <itzcak@flashnetworks.com>
>> Acked-by: Yuchung Cheng <ycheng@google.com>
> Acked-by: Nandita Dukkipati <nanditad@google.com>

Acked-by: Neal Cardwell <ncardwell@google.com>

BTW, Itzcak, have you been able to construct a test case where
this patch now allows a TLP in a case where the sender is limited
by the receive window?

Often we will be prevented from doing any kind of TLP if we are
limited by the receive window, due to these lines in
tcp_schedule_loss_probe():

  if ((tp->snd_cwnd > tcp_packets_in_flight(tp)) &&
       tcp_send_head(sk))
          return false;

But there is something else going on as well, since I haven't been
able to force a TLP even when cwnd == rwin == tcp_packets_in_flight()
== 10. But I haven't had much time to spend on it.

neal

RE: [PATCH net] tcp: Fix TLP implementation in case receive window limits send

[Itzcak Pechtalt]

> -----Original Message-----
> From: Neal Cardwell [mailto:ncardwell@google.com]
> Sent: Wednesday, September 24, 2014 10:25 PM
> To: Nandita Dukkipati
> Cc: Yuchung Cheng; Itzcak Pechtalt; netdev@vger.kernel.org
> Subject: Re: [PATCH net] tcp: Fix TLP implementation in case receive
> window limits send
> 
> On Wed, Sep 24, 2014 at 3:00 PM, Nandita Dukkipati
> <nanditad@google.com> wrote:
> >> On Tue, Sep 23, 2014 at 9:28 AM, Itzcak Pechtalt
> >> <itzcak@flashnetworks.com> wrote:
> >>> From: Itzcak Pechtalt <itzcak@flashnetworks.com>
> >>>
> >>> TCP Tail loss probe (TLP) algorithm implementation has some problem.
> >>> According to paper (draft-dukkipati-tcpm-tcp-loss-probe-0 ):
> >>> In case recive window of receiver limits send of new packet in probe
> >>> time than a retransmit of last packet send should be done.
> >>>
> >>> Actually, return code from tcp_write_xmit is not checked and only
> >>> RTO is scheduled, So, it will take more time for reovery in this
> >>> case than without TLP.
> >>>
> >>> Signed-off-by: Itzcak Pechtalt <itzcak@flashnetworks.com>
> >> Acked-by: Yuchung Cheng <ycheng@google.com>
> > Acked-by: Nandita Dukkipati <nanditad@google.com>
> 
> Acked-by: Neal Cardwell <ncardwell@google.com>
> 
> BTW, Itzcak, have you been able to construct a test case where this patch
> now allows a TLP in a case where the sender is limited by the receive
> window?
> 
> Often we will be prevented from doing any kind of TLP if we are limited by
> the receive window, due to these lines in
> tcp_schedule_loss_probe():
> 
>   if ((tp->snd_cwnd > tcp_packets_in_flight(tp)) &&
>        tcp_send_head(sk))
>           return false;
> 
> But there is something else going on as well, since I haven't been able to
> force a TLP even when cwnd == rwin == tcp_packets_in_flight() == 10. But I
> haven't had much time to spend on it.
> 
> neal

The scenario will be  in case the sender process calls "send" per packet size data, 
so when tcp_schedule_loss_probe  is called there is no additional data 
(tcp_send_head(sk)) returns false),  but further calls to send fail due to receive 
window limit  (The same is right if data bulk size ended exactly with receive 
window limit).

But really I can't understand what is the goal of the above 3 lines of code.
If send was stopped from any reason, why not schedule TLP timer?
Anyway next send will clear the timer by tcp_event_new_data_sent function.

What is your opinion about another patch to remove these 3 lines of code?

Itzcak