[PATCH] tcp: Fix for stalling connections

[PATCH] tcp: Fix for stalling connections

[Damian Lukowski]

This patch fixes a problem in the TCP connection timeout calculation.
Currently, timeout decisions are made on the basis of the current
tcp_time_stamp and retrans_stamp, which is usually set at the first
retransmission.
However, if the retransmission fails in tcp_retransmit_skb(),
retrans_stamp is not updated and remains zero. This leads to wrong
decisions in retransmits_timed_out() if tcp_time_stamp is larger than
the specified timeout, which is very likely.
In this case, the TCP connection dies after the first attempted
(and unsuccessful) retransmission.

With this patch, tcp_skb_cb->when is used instead, when retrans_stamp
is not available.

Thanks to Ilpo Järvinen for code suggestions.

Signed-off-by: Damian Lukowski <damian@tvk.rwth-aachen.de>
---
 include/net/tcp.h |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/include/net/tcp.h b/include/net/tcp.h
index 03a49c7..46f06e0 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -1267,10 +1267,17 @@ static inline bool retransmits_timed_out(const struct sock *sk,
 					 unsigned int boundary)
 {
 	unsigned int timeout, linear_backoff_thresh;
+	unsigned int start_ts;
 
 	if (!inet_csk(sk)->icsk_retransmits)
 		return false;
 
+	if (unlikely(!tcp_sk(sk)->retrans_stamp))
+		start_ts = TCP_SKB_CB(tcp_write_queue_head(
+					(struct sock *)sk))->when;
+	else
+		start_ts = tcp_sk(sk)->retrans_stamp;
+
 	linear_backoff_thresh = ilog2(TCP_RTO_MAX/TCP_RTO_MIN);
 
 	if (boundary <= linear_backoff_thresh)
@@ -1279,7 +1286,7 @@ static inline bool retransmits_timed_out(const struct sock *sk,
 		timeout = ((2 << linear_backoff_thresh) - 1) * TCP_RTO_MIN +
 			  (boundary - linear_backoff_thresh) * TCP_RTO_MAX;
 
-	return (tcp_time_stamp - tcp_sk(sk)->retrans_stamp) >= timeout;
+	return (tcp_time_stamp - start_ts) >= timeout;
 }
 
 static inline struct sk_buff *tcp_send_head(struct sock *sk)
-- 
1.6.4.4

Re: [PATCH] tcp: Fix for stalling connections

[Damian Lukowski]

Damian Lukowski schrieb:
> This patch fixes a problem in the TCP connection timeout calculation.
> Currently, timeout decisions are made on the basis of the current
> tcp_time_stamp and retrans_stamp, which is usually set at the first
> retransmission.
> However, if the retransmission fails in tcp_retransmit_skb(),
> retrans_stamp is not updated and remains zero. This leads to wrong
> decisions in retransmits_timed_out() if tcp_time_stamp is larger than
> the specified timeout, which is very likely.
> In this case, the TCP connection dies after the first attempted
> (and unsuccessful) retransmission.
> 
> With this patch, tcp_skb_cb->when is used instead, when retrans_stamp
> is not available.
> 
> Thanks to Ilpo Järvinen for code suggestions.

... and Frederic Leroy for testing. ;)

Re: [PATCH] tcp: Fix for stalling connections

[Eric Dumazet]

Damian Lukowski a écrit :
> This patch fixes a problem in the TCP connection timeout calculation.
> Currently, timeout decisions are made on the basis of the current
> tcp_time_stamp and retrans_stamp, which is usually set at the first
> retransmission.
> However, if the retransmission fails in tcp_retransmit_skb(),
> retrans_stamp is not updated and remains zero. This leads to wrong
> decisions in retransmits_timed_out() if tcp_time_stamp is larger than
> the specified timeout, which is very likely.
> In this case, the TCP connection dies after the first attempted
> (and unsuccessful) retransmission.
> 
> With this patch, tcp_skb_cb->when is used instead, when retrans_stamp
> is not available.
> 
> Thanks to Ilpo Järvinen for code suggestions.
> 
> Signed-off-by: Damian Lukowski <damian@tvk.rwth-aachen.de>

Hmm, how old is this bug ?

You should a hint of faulty commit so that stable team can apply
this patch to 2.6.32 & 2.6.31

git describe 6fa12c85031485dff38ce550c24f10da23b0adaa

v2.6.31-rc5-1853-g6fa12c8

Or maybe David handles this for us, I dont know...

Minor note : retransmits_timed_out() is used in from net/ipv4/tcp_timer.c
I wonder why its a "static inline" in include/net/tcp.h

Thanks

Re: [PATCH] tcp: Fix for stalling connections

[Damian Lukowski]

Eric Dumazet schrieb:
> Damian Lukowski a écrit :
>> This patch fixes a problem in the TCP connection timeout calculation.
>> Currently, timeout decisions are made on the basis of the current
>> tcp_time_stamp and retrans_stamp, which is usually set at the first
>> retransmission.
>> However, if the retransmission fails in tcp_retransmit_skb(),
>> retrans_stamp is not updated and remains zero. This leads to wrong
>> decisions in retransmits_timed_out() if tcp_time_stamp is larger than
>> the specified timeout, which is very likely.
>> In this case, the TCP connection dies after the first attempted
>> (and unsuccessful) retransmission.
>>
>> With this patch, tcp_skb_cb->when is used instead, when retrans_stamp
>> is not available.
>>
>> Thanks to Ilpo Järvinen for code suggestions.
>>
>> Signed-off-by: Damian Lukowski <damian@tvk.rwth-aachen.de>
> 
> Hmm, how old is this bug ?
> 
> You should a hint of faulty commit so that stable team can apply
> this patch to 2.6.32 & 2.6.31
> 
> git describe 6fa12c85031485dff38ce550c24f10da23b0adaa
> 
> v2.6.31-rc5-1853-g6fa12c8

It has been introduced with retransmits_timed_out() in 2.6.32,
as timeout calculations have based on the number of
retransmissions before. The patch is needed for 2.6.32
and 2.6.33, but 2.6.31 does not use retransmits_timed_out().

> Or maybe David handles this for us, I dont know...
> 
> Minor note : retransmits_timed_out() is used in from net/ipv4/tcp_timer.c
> I wonder why its a "static inline" in include/net/tcp.h

I can place it in tcp_timer.c, if it's that, what you mean.

Regards.

> Thanks
> 
> --
> 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 for stalling connections

[David Miller]

From: Damian Lukowski <damian@tvk.rwth-aachen.de>
Date: Mon, 07 Dec 2009 12:17:46 +0100

> Eric Dumazet schrieb:
>> You should a hint of faulty commit so that stable team can apply
>> this patch to 2.6.32 & 2.6.31
>> 
>> git describe 6fa12c85031485dff38ce550c24f10da23b0adaa
>> 
>> v2.6.31-rc5-1853-g6fa12c8
> 
> It has been introduced with retransmits_timed_out() in 2.6.32,
> as timeout calculations have based on the number of
> retransmissions before. The patch is needed for 2.6.32
> and 2.6.33, but 2.6.31 does not use retransmits_timed_out().

I think also the commit introducing the regression should be
explicitly referenced in the commit message of the fix.

>> Or maybe David handles this for us, I dont know...
>> 
>> Minor note : retransmits_timed_out() is used in from net/ipv4/tcp_timer.c
>> I wonder why its a "static inline" in include/net/tcp.h
> 
> I can place it in tcp_timer.c, if it's that, what you mean.

I'm pretty sure that's what he means :-)

[PATCHv2 0/2] tcp: Stalling connections

[Damian Lukowski]

This series of patches fixes a problem with the new RTO
timeout calculation as introduced for 2.6.32. Under some
circumstances, if a retransmission fails, the connection
is likely to die silently.

Changelog since v1:
Moved retransmits_timed_out from include/net/tcp.h
to net/ipv4/tcp_timer.c, where it is used.

Signed-off-by: Damian Lukowski <damian@tvk.rwth-aachen.de>

Thanks.

[PATCHv2 1/2] tcp: Stalling connections: Move timeout calculation routine

[Damian Lukowski]

This patch moves retransmits_timed_out() from include/net/tcp.h
to tcp_timer.c, where it is used.

Signed-off-by: Damian Lukowski <damian@tvk.rwth-aachen.de>
---
 include/net/tcp.h    |   22 ----------------------
 net/ipv4/tcp_timer.c |   23 +++++++++++++++++++++++
 2 files changed, 23 insertions(+), 22 deletions(-)

diff --git a/include/net/tcp.h b/include/net/tcp.h
index 03a49c7..ffab98a 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -1259,28 +1259,6 @@ static inline struct sk_buff *tcp_write_queue_prev(struct sock *sk, struct sk_bu
 #define tcp_for_write_queue_from_safe(skb, tmp, sk)			\
 	skb_queue_walk_from_safe(&(sk)->sk_write_queue, skb, tmp)
 
-/* This function calculates a "timeout" which is equivalent to the timeout of a
- * TCP connection after "boundary" unsucessful, exponentially backed-off
- * retransmissions with an initial RTO of TCP_RTO_MIN.
- */
-static inline bool retransmits_timed_out(const struct sock *sk,
-					 unsigned int boundary)
-{
-	unsigned int timeout, linear_backoff_thresh;
-
-	if (!inet_csk(sk)->icsk_retransmits)
-		return false;
-
-	linear_backoff_thresh = ilog2(TCP_RTO_MAX/TCP_RTO_MIN);
-
-	if (boundary <= linear_backoff_thresh)
-		timeout = ((2 << boundary) - 1) * TCP_RTO_MIN;
-	else
-		timeout = ((2 << linear_backoff_thresh) - 1) * TCP_RTO_MIN +
-			  (boundary - linear_backoff_thresh) * TCP_RTO_MAX;
-
-	return (tcp_time_stamp - tcp_sk(sk)->retrans_stamp) >= timeout;
-}
 
 static inline struct sk_buff *tcp_send_head(struct sock *sk)
 {
diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
index cdb2ca7..5c5f739 100644
--- a/net/ipv4/tcp_timer.c
+++ b/net/ipv4/tcp_timer.c
@@ -132,6 +132,29 @@ static void tcp_mtu_probing(struct inet_connection_sock *icsk, struct sock *sk)
 	}
 }
 
+/* This function calculates a "timeout" which is equivalent to the timeout of a
+ * TCP connection after "boundary" unsucessful, exponentially backed-off
+ * retransmissions with an initial RTO of TCP_RTO_MIN.
+ */
+static bool retransmits_timed_out(const struct sock *sk,
+				  unsigned int boundary)
+{
+	unsigned int timeout, linear_backoff_thresh;
+
+	if (!inet_csk(sk)->icsk_retransmits)
+		return false;
+
+	linear_backoff_thresh = ilog2(TCP_RTO_MAX/TCP_RTO_MIN);
+
+	if (boundary <= linear_backoff_thresh)
+		timeout = ((2 << boundary) - 1) * TCP_RTO_MIN;
+	else
+		timeout = ((2 << linear_backoff_thresh) - 1) * TCP_RTO_MIN +
+			  (boundary - linear_backoff_thresh) * TCP_RTO_MAX;
+
+	return (tcp_time_stamp - tcp_sk(sk)->retrans_stamp) >= timeout;
+}
+
 /* A write timeout has occurred. Process the after effects. */
 static int tcp_write_timeout(struct sock *sk)
 {
-- 
1.6.4.4

[PATCHv2 2/2] tcp: Stalling connections: Fix timeout calculation routine

[Damian Lukowski]

This patch fixes a problem in the TCP connection timeout calculation.
Currently, timeout decisions are made on the basis of the current
tcp_time_stamp and retrans_stamp, which is usually set at the first
retransmission.
However, if the retransmission fails in tcp_retransmit_skb(),
retrans_stamp is not updated and remains zero. This leads to wrong
decisions in retransmits_timed_out() if tcp_time_stamp is larger than
the specified timeout, which is very likely.
In this case, the TCP connection dies after the first attempted
(and unsuccessful) retransmission.

With this patch, tcp_skb_cb->when is used instead, when retrans_stamp
is not available.

This bug has been introduced together with retransmits_timed_out()
in 2.6.32, as the number of retransmissions has been used for timeout
decisions before.

Thanks to Ilpo Järvinen for code suggestions and Frederic Leroy for
testing.

Signed-off-by: Damian Lukowski <damian@tvk.rwth-aachen.de>
---
 net/ipv4/tcp_timer.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
index 5c5f739..a9d2891 100644
--- a/net/ipv4/tcp_timer.c
+++ b/net/ipv4/tcp_timer.c
@@ -140,10 +140,17 @@ static bool retransmits_timed_out(const struct sock *sk,
 				  unsigned int boundary)
 {
 	unsigned int timeout, linear_backoff_thresh;
+	unsigned int start_ts;
 
 	if (!inet_csk(sk)->icsk_retransmits)
 		return false;
 
+	if (unlikely(!tcp_sk(sk)->retrans_stamp))
+		start_ts = TCP_SKB_CB(tcp_write_queue_head(
+					(struct sock *)sk))->when;
+	else
+		start_ts = tcp_sk(sk)->retrans_stamp;
+
 	linear_backoff_thresh = ilog2(TCP_RTO_MAX/TCP_RTO_MIN);
 
 	if (boundary <= linear_backoff_thresh)
@@ -152,7 +159,7 @@ static bool retransmits_timed_out(const struct sock *sk,
 		timeout = ((2 << linear_backoff_thresh) - 1) * TCP_RTO_MIN +
 			  (boundary - linear_backoff_thresh) * TCP_RTO_MAX;
 
-	return (tcp_time_stamp - tcp_sk(sk)->retrans_stamp) >= timeout;
+	return (tcp_time_stamp - start_ts) >= timeout;
 }
 
 /* A write timeout has occurred. Process the after effects. */
-- 
1.6.4.4

Re: [PATCHv2 1/2] tcp: Stalling connections: Move timeout calculation routine

[Frederic Leroy]

Le Mon, 07 Dec 2009 12:41:27 +0100,
Damian Lukowski <damian@tvk.rwth-aachen.de> a écrit :

> This patch moves retransmits_timed_out() from include/net/tcp.h
> to tcp_timer.c, where it is used.

I just finish to make .18 and .19 trace with your previous patch.
It's still available in http://www.starox.org/pub/scp_stall/

The copy was successful.
.18 is with your patch + some printk.
.19 is with your patch only.
I also checked that it still fails without.

Do you wan't me to make new trace with the new series of patch ?
I couldn't do it before this evening.

-- 
Frédéric Leroy

Re: [PATCHv2 2/2] tcp: Stalling connections: Fix timeout calculation routine

[Ilpo Järvinen]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2426 bytes --]

On Mon, 7 Dec 2009, Damian Lukowski wrote:

> This patch fixes a problem in the TCP connection timeout calculation.
> Currently, timeout decisions are made on the basis of the current
> tcp_time_stamp and retrans_stamp, which is usually set at the first
> retransmission.
> However, if the retransmission fails in tcp_retransmit_skb(),
> retrans_stamp is not updated and remains zero. This leads to wrong
> decisions in retransmits_timed_out() if tcp_time_stamp is larger than
> the specified timeout, which is very likely.
> In this case, the TCP connection dies after the first attempted
> (and unsuccessful) retransmission.
> 
> With this patch, tcp_skb_cb->when is used instead, when retrans_stamp
> is not available.
> 
> This bug has been introduced together with retransmits_timed_out()
> in 2.6.32, as the number of retransmissions has been used for timeout
> decisions before.
> 
> Thanks to Ilpo Järvinen for code suggestions and Frederic Leroy for
> testing.
> 
> Signed-off-by: Damian Lukowski <damian@tvk.rwth-aachen.de>
> ---
>  net/ipv4/tcp_timer.c |    9 ++++++++-
>  1 files changed, 8 insertions(+), 1 deletions(-)
> 
> diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
> index 5c5f739..a9d2891 100644
> --- a/net/ipv4/tcp_timer.c
> +++ b/net/ipv4/tcp_timer.c
> @@ -140,10 +140,17 @@ static bool retransmits_timed_out(const struct sock *sk,
>  				  unsigned int boundary)
>  {
>  	unsigned int timeout, linear_backoff_thresh;
> +	unsigned int start_ts;
>  
>  	if (!inet_csk(sk)->icsk_retransmits)
>  		return false;
>  
> +	if (unlikely(!tcp_sk(sk)->retrans_stamp))
> +		start_ts = TCP_SKB_CB(tcp_write_queue_head(
> +					(struct sock *)sk))->when;

Grr, a cast.... 

> +	else
> +		start_ts = tcp_sk(sk)->retrans_stamp;
> +
>  	linear_backoff_thresh = ilog2(TCP_RTO_MAX/TCP_RTO_MIN);
>  
>  	if (boundary <= linear_backoff_thresh)
> @@ -152,7 +159,7 @@ static bool retransmits_timed_out(const struct sock *sk,
>  		timeout = ((2 << linear_backoff_thresh) - 1) * TCP_RTO_MIN +
>  			  (boundary - linear_backoff_thresh) * TCP_RTO_MAX;
>  
> -	return (tcp_time_stamp - tcp_sk(sk)->retrans_stamp) >= timeout;
> +	return (tcp_time_stamp - start_ts) >= timeout;
>  }
>  
>  /* A write timeout has occurred. Process the after effects. */
> 

Also, in here it's more useful to provide the fix as the first patch (1/2) 
since it's going to stable and those people don't want the move patch 
there.

-- 
 i.

Re: [PATCHv2 2/2] tcp: Stalling connections: Fix timeout calculation routine

[Damian Lukowski]

Am 07.12.2009, 13:08 Uhr, schrieb Ilpo Järvinen  
<ilpo.jarvinen@helsinki.fi>:

> On Mon, 7 Dec 2009, Damian Lukowski wrote:
>
>> This patch fixes a problem in the TCP connection timeout calculation.
>> Currently, timeout decisions are made on the basis of the current
>> tcp_time_stamp and retrans_stamp, which is usually set at the first
>> retransmission.
>> However, if the retransmission fails in tcp_retransmit_skb(),
>> retrans_stamp is not updated and remains zero. This leads to wrong
>> decisions in retransmits_timed_out() if tcp_time_stamp is larger than
>> the specified timeout, which is very likely.
>> In this case, the TCP connection dies after the first attempted
>> (and unsuccessful) retransmission.
>>
>> With this patch, tcp_skb_cb->when is used instead, when retrans_stamp
>> is not available.
>>
>> This bug has been introduced together with retransmits_timed_out()
>> in 2.6.32, as the number of retransmissions has been used for timeout
>> decisions before.
>>
>> Thanks to Ilpo Järvinen for code suggestions and Frederic Leroy for
>> testing.
>>
>> Signed-off-by: Damian Lukowski <damian@tvk.rwth-aachen.de>
>> ---
>>  net/ipv4/tcp_timer.c |    9 ++++++++-
>>  1 files changed, 8 insertions(+), 1 deletions(-)
>>
>> diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
>> index 5c5f739..a9d2891 100644
>> --- a/net/ipv4/tcp_timer.c
>> +++ b/net/ipv4/tcp_timer.c
>> @@ -140,10 +140,17 @@ static bool retransmits_timed_out(const struct  
>> sock *sk,
>>  				  unsigned int boundary)
>>  {
>>  	unsigned int timeout, linear_backoff_thresh;
>> +	unsigned int start_ts;
>>
>>  	if (!inet_csk(sk)->icsk_retransmits)
>>  		return false;
>>
>> +	if (unlikely(!tcp_sk(sk)->retrans_stamp))
>> +		start_ts = TCP_SKB_CB(tcp_write_queue_head(
>> +					(struct sock *)sk))->when;
>
> Grr, a cast....

I'm a little bit confused now (not the first time :)).
Without the cast, there are a lot of compiler warnings.
Also, I remember that I have specified const in the function signature
on purpose because of other warnings.
But now, it seems to work without const and no cast ...

>
>> +	else
>> +		start_ts = tcp_sk(sk)->retrans_stamp;
>> +
>>  	linear_backoff_thresh = ilog2(TCP_RTO_MAX/TCP_RTO_MIN);
>>
>>  	if (boundary <= linear_backoff_thresh)
>> @@ -152,7 +159,7 @@ static bool retransmits_timed_out(const struct sock  
>> *sk,
>>  		timeout = ((2 << linear_backoff_thresh) - 1) * TCP_RTO_MIN +
>>  			  (boundary - linear_backoff_thresh) * TCP_RTO_MAX;
>>
>> -	return (tcp_time_stamp - tcp_sk(sk)->retrans_stamp) >= timeout;
>> +	return (tcp_time_stamp - start_ts) >= timeout;
>>  }
>>
>>  /* A write timeout has occurred. Process the after effects. */
>>
>
> Also, in here it's more useful to provide the fix as the first patch  
> (1/2)
> since it's going to stable and those people don't want the move patch
> there.

Ok, I will patch the other way round, without the cast.

Damian

Re: [PATCHv2 2/2] tcp: Stalling connections: Fix timeout calculation routine

[Ilpo Järvinen]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2719 bytes --]

On Mon, 7 Dec 2009, Damian Lukowski wrote:

> Am 07.12.2009, 13:08 Uhr, schrieb Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>:
> 
> > On Mon, 7 Dec 2009, Damian Lukowski wrote:
> > 
> > > This patch fixes a problem in the TCP connection timeout calculation.
> > > Currently, timeout decisions are made on the basis of the current
> > > tcp_time_stamp and retrans_stamp, which is usually set at the first
> > > retransmission.
> > > However, if the retransmission fails in tcp_retransmit_skb(),
> > > retrans_stamp is not updated and remains zero. This leads to wrong
> > > decisions in retransmits_timed_out() if tcp_time_stamp is larger than
> > > the specified timeout, which is very likely.
> > > In this case, the TCP connection dies after the first attempted
> > > (and unsuccessful) retransmission.
> > > 
> > > With this patch, tcp_skb_cb->when is used instead, when retrans_stamp
> > > is not available.
> > > 
> > > This bug has been introduced together with retransmits_timed_out()
> > > in 2.6.32, as the number of retransmissions has been used for timeout
> > > decisions before.
> > > 
> > > Thanks to Ilpo Järvinen for code suggestions and Frederic Leroy for
> > > testing.
> > > 
> > > Signed-off-by: Damian Lukowski <damian@tvk.rwth-aachen.de>
> > > ---
> > > net/ipv4/tcp_timer.c |    9 ++++++++-
> > > 1 files changed, 8 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
> > > index 5c5f739..a9d2891 100644
> > > --- a/net/ipv4/tcp_timer.c
> > > +++ b/net/ipv4/tcp_timer.c
> > > @@ -140,10 +140,17 @@ static bool retransmits_timed_out(const struct sock
> > > *sk,
> > > 				  unsigned int boundary)
> > > {
> > > 	unsigned int timeout, linear_backoff_thresh;
> > > +	unsigned int start_ts;
> > > 
> > > 	if (!inet_csk(sk)->icsk_retransmits)
> > > 		return false;
> > > 
> > > +	if (unlikely(!tcp_sk(sk)->retrans_stamp))
> > > +		start_ts = TCP_SKB_CB(tcp_write_queue_head(
> > > +					(struct sock *)sk))->when;
> > 
> > Grr, a cast....
> 
> I'm a little bit confused now (not the first time :)).
> Without the cast, there are a lot of compiler warnings.
> Also, I remember that I have specified const in the function signature
> on purpose because of other warnings.
> But now, it seems to work without const and no cast ...

I guess you could just drop the const? Maybe you called it from some other 
place back then? To me it seems that both functions which call 
retransmits_timed_out have a non-const sk?

> > Also, in here it's more useful to provide the fix as the first patch (1/2)
> > since it's going to stable and those people don't want the move patch
> > there.
> 
> Ok, I will patch the other way round, without the cast.

Thanks.

-- 
 i.

[PATCHv3 0/2] tcp: Stalling connections

[Damian Lukowski]

This series of patches fixes a problem with the new RTO
timeout calculation as introduced for 2.6.32 in
commit 6fa12c85031485dff38ce550c24f10da23b0adaa.
Under some circumstances, if a retransmission fails, the connection
is likely to die silently.

Changes since v2:
The bugfix has to be applied first, so that the code cleanup
is optional.

Thanks

Signed-off-by: Damian Lukowski <damian@tvk.rwth-aachen.de>
---

[PATCHv3 1/2] tcp: Stalling connections: Fix timeout calculation routine

[Damian Lukowski]

This patch fixes a problem in the TCP connection timeout calculation.
Currently, timeout decisions are made on the basis of the current
tcp_time_stamp and retrans_stamp, which is usually set at the first
retransmission.
However, if the retransmission fails in tcp_retransmit_skb(),
retrans_stamp is not updated and remains zero. This leads to wrong
decisions in retransmits_timed_out() if tcp_time_stamp is larger than
the specified timeout, which is very likely.
In this case, the TCP connection dies after the first attempted
(and unsuccessful) retransmission.

With this patch, tcp_skb_cb->when is used instead, when retrans_stamp
is not available.

This bug has been introduced together with retransmits_timed_out()
in 2.6.32, as the number of retransmissions has been used for timeout
decisions before. The corresponding commit was
6fa12c85031485dff38ce550c24f10da23b0adaa.

Thanks to Ilpo Järvinen for code suggestions and Frederic Leroy for
testing.

Signed-off-by: Damian Lukowski <damian@tvk.rwth-aachen.de>
---
 include/net/tcp.h |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/include/net/tcp.h b/include/net/tcp.h
index 03a49c7..842ac4d 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -1263,14 +1263,20 @@ static inline struct sk_buff *tcp_write_queue_prev(struct sock *sk, struct sk_bu
  * TCP connection after "boundary" unsucessful, exponentially backed-off
  * retransmissions with an initial RTO of TCP_RTO_MIN.
  */
-static inline bool retransmits_timed_out(const struct sock *sk,
+static inline bool retransmits_timed_out(struct sock *sk,
 					 unsigned int boundary)
 {
 	unsigned int timeout, linear_backoff_thresh;
+	unsigned int start_ts;
 
 	if (!inet_csk(sk)->icsk_retransmits)
 		return false;
 
+	if (unlikely(!tcp_sk(sk)->retrans_stamp))
+		start_ts = TCP_SKB_CB(tcp_write_queue_head(sk))->when;
+	else
+		start_ts = tcp_sk(sk)->retrans_stamp;
+
 	linear_backoff_thresh = ilog2(TCP_RTO_MAX/TCP_RTO_MIN);
 
 	if (boundary <= linear_backoff_thresh)
@@ -1279,7 +1285,7 @@ static inline bool retransmits_timed_out(const struct sock *sk,
 		timeout = ((2 << linear_backoff_thresh) - 1) * TCP_RTO_MIN +
 			  (boundary - linear_backoff_thresh) * TCP_RTO_MAX;
 
-	return (tcp_time_stamp - tcp_sk(sk)->retrans_stamp) >= timeout;
+	return (tcp_time_stamp - start_ts) >= timeout;
 }
 
 static inline struct sk_buff *tcp_send_head(struct sock *sk)
-- 
1.6.4.4

[PATCHv3 2/2] tcp: Stalling connections: Move timeout calculation routine

[Damian Lukowski]

This patch moves retransmits_timed_out() from include/net/tcp.h
to tcp_timer.c, where it is used.

Signed-off-by: Damian Lukowski <damian@tvk.rwth-aachen.de>
---
 include/net/tcp.h    |   28 ----------------------------
 net/ipv4/tcp_timer.c |   29 +++++++++++++++++++++++++++++
 2 files changed, 29 insertions(+), 28 deletions(-)

diff --git a/include/net/tcp.h b/include/net/tcp.h
index 842ac4d..ffab98a 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -1259,34 +1259,6 @@ static inline struct sk_buff *tcp_write_queue_prev(struct sock *sk, struct sk_bu
 #define tcp_for_write_queue_from_safe(skb, tmp, sk)			\
 	skb_queue_walk_from_safe(&(sk)->sk_write_queue, skb, tmp)
 
-/* This function calculates a "timeout" which is equivalent to the timeout of a
- * TCP connection after "boundary" unsucessful, exponentially backed-off
- * retransmissions with an initial RTO of TCP_RTO_MIN.
- */
-static inline bool retransmits_timed_out(struct sock *sk,
-					 unsigned int boundary)
-{
-	unsigned int timeout, linear_backoff_thresh;
-	unsigned int start_ts;
-
-	if (!inet_csk(sk)->icsk_retransmits)
-		return false;
-
-	if (unlikely(!tcp_sk(sk)->retrans_stamp))
-		start_ts = TCP_SKB_CB(tcp_write_queue_head(sk))->when;
-	else
-		start_ts = tcp_sk(sk)->retrans_stamp;
-
-	linear_backoff_thresh = ilog2(TCP_RTO_MAX/TCP_RTO_MIN);
-
-	if (boundary <= linear_backoff_thresh)
-		timeout = ((2 << boundary) - 1) * TCP_RTO_MIN;
-	else
-		timeout = ((2 << linear_backoff_thresh) - 1) * TCP_RTO_MIN +
-			  (boundary - linear_backoff_thresh) * TCP_RTO_MAX;
-
-	return (tcp_time_stamp - start_ts) >= timeout;
-}
 
 static inline struct sk_buff *tcp_send_head(struct sock *sk)
 {
diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
index cdb2ca7..dfa0c37 100644
--- a/net/ipv4/tcp_timer.c
+++ b/net/ipv4/tcp_timer.c
@@ -132,6 +132,35 @@ static void tcp_mtu_probing(struct inet_connection_sock *icsk, struct sock *sk)
 	}
 }
 
+/* This function calculates a "timeout" which is equivalent to the timeout of a
+ * TCP connection after "boundary" unsucessful, exponentially backed-off
+ * retransmissions with an initial RTO of TCP_RTO_MIN.
+ */
+static bool retransmits_timed_out(struct sock *sk,
+				  unsigned int boundary)
+{
+	unsigned int timeout, linear_backoff_thresh;
+	unsigned int start_ts;
+
+	if (!inet_csk(sk)->icsk_retransmits)
+		return false;
+
+	if (unlikely(!tcp_sk(sk)->retrans_stamp))
+		start_ts = TCP_SKB_CB(tcp_write_queue_head(sk))->when;
+	else
+		start_ts = tcp_sk(sk)->retrans_stamp;
+
+	linear_backoff_thresh = ilog2(TCP_RTO_MAX/TCP_RTO_MIN);
+
+	if (boundary <= linear_backoff_thresh)
+		timeout = ((2 << boundary) - 1) * TCP_RTO_MIN;
+	else
+		timeout = ((2 << linear_backoff_thresh) - 1) * TCP_RTO_MIN +
+			  (boundary - linear_backoff_thresh) * TCP_RTO_MAX;
+
+	return (tcp_time_stamp - start_ts) >= timeout;
+}
+
 /* A write timeout has occurred. Process the after effects. */
 static int tcp_write_timeout(struct sock *sk)
 {
-- 
1.6.4.4

Re: [PATCHv2 1/2] tcp: Stalling connections: Move timeout calculation routine

[Damian Lukowski]

Am 07.12.2009, 13:01 Uhr, schrieb Frederic Leroy <fredo@starox.org>:

> Le Mon, 07 Dec 2009 12:41:27 +0100,
> Damian Lukowski <damian@tvk.rwth-aachen.de> a écrit :
>
>> This patch moves retransmits_timed_out() from include/net/tcp.h
>> to tcp_timer.c, where it is used.
>
> I just finish to make .18 and .19 trace with your previous patch.
> It's still available in http://www.starox.org/pub/scp_stall/
>
> The copy was successful.
> .18 is with your patch + some printk.
> .19 is with your patch only.
> I also checked that it still fails without.
>
> Do you wan't me to make new trace with the new series of patch ?
> I couldn't do it before this evening.

Hi,
you could test v3 if it is acked at least by Ilpo. But since v3
leads to the same code as v2, it think it will be fine.

Thank you.

Damian

Re: [PATCHv3 1/2] tcp: Stalling connections: Fix timeout calculation routine

[Ilpo Järvinen]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2845 bytes --]

On Mon, 7 Dec 2009, Damian Lukowski wrote:

> This patch fixes a problem in the TCP connection timeout calculation.
> Currently, timeout decisions are made on the basis of the current
> tcp_time_stamp and retrans_stamp, which is usually set at the first
> retransmission.
> However, if the retransmission fails in tcp_retransmit_skb(),
> retrans_stamp is not updated and remains zero. This leads to wrong
> decisions in retransmits_timed_out() if tcp_time_stamp is larger than
> the specified timeout, which is very likely.
> In this case, the TCP connection dies after the first attempted
> (and unsuccessful) retransmission.
> 
> With this patch, tcp_skb_cb->when is used instead, when retrans_stamp
> is not available.
> 
> This bug has been introduced together with retransmits_timed_out()
> in 2.6.32, as the number of retransmissions has been used for timeout
> decisions before. The corresponding commit was
> 6fa12c85031485dff38ce550c24f10da23b0adaa.
> 
> Thanks to Ilpo Järvinen for code suggestions and Frederic Leroy for
> testing.
> 
> Signed-off-by: Damian Lukowski <damian@tvk.rwth-aachen.de>
> ---
>  include/net/tcp.h |   10 ++++++++--
>  1 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index 03a49c7..842ac4d 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -1263,14 +1263,20 @@ static inline struct sk_buff *tcp_write_queue_prev(struct sock *sk, struct sk_bu
>   * TCP connection after "boundary" unsucessful, exponentially backed-off
>   * retransmissions with an initial RTO of TCP_RTO_MIN.
>   */
> -static inline bool retransmits_timed_out(const struct sock *sk,
> +static inline bool retransmits_timed_out(struct sock *sk,
>  					 unsigned int boundary)
>  {
>  	unsigned int timeout, linear_backoff_thresh;
> +	unsigned int start_ts;
>  
>  	if (!inet_csk(sk)->icsk_retransmits)
>  		return false;
>  
> +	if (unlikely(!tcp_sk(sk)->retrans_stamp))
> +		start_ts = TCP_SKB_CB(tcp_write_queue_head(sk))->when;
> +	else
> +		start_ts = tcp_sk(sk)->retrans_stamp;
> +
>  	linear_backoff_thresh = ilog2(TCP_RTO_MAX/TCP_RTO_MIN);
>  
>  	if (boundary <= linear_backoff_thresh)
> @@ -1279,7 +1285,7 @@ static inline bool retransmits_timed_out(const struct sock *sk,
>  		timeout = ((2 << linear_backoff_thresh) - 1) * TCP_RTO_MIN +
>  			  (boundary - linear_backoff_thresh) * TCP_RTO_MAX;
>  
> -	return (tcp_time_stamp - tcp_sk(sk)->retrans_stamp) >= timeout;
> +	return (tcp_time_stamp - start_ts) >= timeout;
>  }
>  
>  static inline struct sk_buff *tcp_send_head(struct sock *sk)

Acked-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>

Also, this should be added:

Reported-by: Frederic Leroy <fredo@starox.org>

...and I think he has already tested this fix a number of times in 
different forms, so I'd already include his Tested-by: too.

-- 
 i.

Re: [PATCHv3 1/2] tcp: Stalling connections: Fix timeout calculation routine

[Frederic Leroy]

Le Mon, 7 Dec 2009 20:50:11 +0200 (EET),
"Ilpo Järvinen" <ilpo.jarvinen@helsinki.fi> a écrit :

> On Mon, 7 Dec 2009, Damian Lukowski wrote:
> 
> > This patch fixes a problem in the TCP connection timeout
> > calculation. Currently, timeout decisions are made on the basis of
> > 6fa12c85031485dff38ce550c24f10da23b0adaa.
> [...]
>> >  static inline struct sk_buff *tcp_send_head(struct sock *sk) 
> 
> Acked-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>
> 
> Also, this should be added:
> 
> Reported-by: Frederic Leroy <fredo@starox.org>
> 
> ...and I think he has already tested this fix a number of times in 
> different forms, so I'd already include his Tested-by: too.

If you need a bad connection to do some test, don't hesitate !
Thank you guys ;)

-- 
Frédéric Leroy

Re: [PATCHv3 1/2] tcp: Stalling connections: Fix timeout calculation routine

[David Miller]

From: "Ilpo J^[$(D+#^[(Brvinen" <ilpo.jarvinen@helsinki.fi>
Date: Mon, 7 Dec 2009 20:50:11 +0200 (EET)

> Acked-by: Ilpo J^[$(D+#^[(Brvinen <ilpo.jarvinen@helsinki.fi>
> 
> Also, this should be added:
> 
> Reported-by: Frederic Leroy <fredo@starox.org>
> 
> ...and I think he has already tested this fix a number of times in 
> different forms, so I'd already include his Tested-by: too.

Done, applied, and I'll get this to -stable as quickly as I can.

Thanks everyone for all of your hard work!

TCP not retransmitting

[Ilpo Järvinen]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2226 bytes --]

Changed subject and dropped all but netdev from Cc.

On Mon, 7 Dec 2009, Frederic Leroy wrote:

> Le Mon, 7 Dec 2009 20:50:11 +0200 (EET),
> "Ilpo Järvinen" <ilpo.jarvinen@helsinki.fi> a écrit :
> 
> > On Mon, 7 Dec 2009, Damian Lukowski wrote:
> > 
> > > This patch fixes a problem in the TCP connection timeout
> > > calculation. Currently, timeout decisions are made on the basis of
> > > 6fa12c85031485dff38ce550c24f10da23b0adaa.
> > [...]
> >> >  static inline struct sk_buff *tcp_send_head(struct sock *sk) 
> > 
> > Acked-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>
> > 
> > Also, this should be added:
> > 
> > Reported-by: Frederic Leroy <fredo@starox.org>
> > 
> > ...and I think he has already tested this fix a number of times in 
> > different forms, so I'd already include his Tested-by: too.
> 
> If you need a bad connection to do some test, don't hesitate !
> Thank you guys ;)

Hi,

Now that the more important issues are out of the way, could try with the 
following debug patch, if we could get some information on why the 
retransmission are not happening when they're supposed to (the EAGAIN 
problem we noticed earlier). Besides trying with 2.6.32, it might also be 
worth of the effort to try that in 2.6.31 to see if that also gives those 
EAGAIN things or if it's a new issue. It is quite insignificant whether 
to previous fix patch is included or not though perhaps easier to know 
that the problem really happened if a stall is experiencable vs. the fixed 
kernel.

-- 
 i.


diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index fcd278a..c29aed0 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -1894,6 +1894,8 @@ int tcp_retransmit_skb(struct sock *sk, struct sk_buff *skb)
 		icsk->icsk_mtup.probe_size = 0;
 	}
 
+	printk("sk: %p wm_a: %u wm_q: %u sbuf: %u\n", sk,
+		atomic_read(&sk->sk_wmem_alloc), sk->sk_wmem_queued, sk->sk_sndbuf);
 	/* Do not sent more than we queued. 1/4 is reserved for possible
 	 * copying overhead: fragmentation, tunneling, mangling etc.
 	 */
@@ -1986,6 +1988,7 @@ int tcp_retransmit_skb(struct sock *sk, struct sk_buff *skb)
 		 */
 		TCP_SKB_CB(skb)->ack_seq = tp->snd_nxt;
 	}
+	printk("sk: %p E %u", sk, err);
 	return err;
 }

Re: TCP not retransmitting

[Frederic Leroy]

Hi Ilpo,

On Thu, Dec 10, 2009 at 03:24:20PM +0200, Ilpo Järvinen wrote:
> Hi,
> 
> Now that the more important issues are out of the way, could try with the 
> following debug patch, if we could get some information on why the 
> retransmission are not happening when they're supposed to (the EAGAIN 
> problem we noticed earlier). Besides trying with 2.6.32, it might also be 
> worth of the effort to try that in 2.6.31 to see if that also gives those 
> EAGAIN things or if it's a new issue. It is quite insignificant whether 
> to previous fix patch is included or not though perhaps easier to know 
> that the problem really happened if a stall is experiencable vs. the fixed 
> kernel.

I made 3 test this time. Unlike the other, I put them appart in a new directory : 
http://www.starox.org/pub/scp_stall/eagain/

The first was made with 2.6.31 kernel.
I disconnected the ethernet cable for about 30s.
There was some eagain error.

The two other were made with 2.6.32, without the fix.
I didn't disconnected anything as I know it will fails quickly :)
The first try has an eagain error.

> 
> -- 
>  i.
> 
> 
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index fcd278a..c29aed0 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -1894,6 +1894,8 @@ int tcp_retransmit_skb(struct sock *sk, struct sk_buff *skb)
>  		icsk->icsk_mtup.probe_size = 0;
>  	}
>  
> +	printk("sk: %p wm_a: %u wm_q: %u sbuf: %u\n", sk,
> +		atomic_read(&sk->sk_wmem_alloc), sk->sk_wmem_queued, sk->sk_sndbuf);
>  	/* Do not sent more than we queued. 1/4 is reserved for possible
>  	 * copying overhead: fragmentation, tunneling, mangling etc.
>  	 */
> @@ -1986,6 +1988,7 @@ int tcp_retransmit_skb(struct sock *sk, struct sk_buff *skb)
>  		 */
>  		TCP_SKB_CB(skb)->ack_seq = tp->snd_nxt;
>  	}
> +	printk("sk: %p E %u", sk, err);
                           ^
                      missing \n ;-)
>  	return err;
>  }

-- 
Frédéric Leroy