[PATCH] net: tcp: add correct check for tcp_retransmit_skb()

[PATCH] net: tcp: add correct check for tcp_retransmit_skb()

[Sharath Chandra Vurukala]

when the tcp_retranmission_timer expires and tcp_retranmsit_skb is
called if the retranmsission fails due to local congestion,
backoff should not incremented.

tcp_retransmit_skb() returns non-zero negative value in some cases of
failure but the caller tcp_retransmission_timer() has a check for
failure which checks if the return value is greater than zero.
The check is corrected to check for non-zero value.

Signed-off-by: Sharath Chandra Vurukala <sharathv@codeaurora.org>
---
 net/ipv4/tcp_timer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
index 091c5392..c19f371 100644
--- a/net/ipv4/tcp_timer.c
+++ b/net/ipv4/tcp_timer.c
@@ -511,7 +511,7 @@ void tcp_retransmit_timer(struct sock *sk)
 
 	tcp_enter_loss(sk);
 
-	if (tcp_retransmit_skb(sk, tcp_rtx_queue_head(sk), 1) > 0) {
+	if (tcp_retransmit_skb(sk, tcp_rtx_queue_head(sk), 1) != 0) {
 		/* Retransmission failed because of local congestion,
 		 * do not backoff.
 		 */
-- 
1.9.1

Re: [PATCH] net: tcp: add correct check for tcp_retransmit_skb()

[Eric Dumazet]

On 11/30/2018 10:28 AM, Sharath Chandra Vurukala wrote:
> when the tcp_retranmission_timer expires and tcp_retranmsit_skb is
> called if the retranmsission fails due to local congestion,
> backoff should not incremented.
> 
> tcp_retransmit_skb() returns non-zero negative value in some cases of
> failure but the caller tcp_retransmission_timer() has a check for
> failure which checks if the return value is greater than zero.
> The check is corrected to check for non-zero value.
> 
> Signed-off-by: Sharath Chandra Vurukala <sharathv@codeaurora.org>
>

This looks wrong.

Yuchung has cooked a patch series to really address issues, please wait for it

We are waiting for David Miller to merge a prior patch series into net tree, then
in net-next.

Thanks.

Re: [PATCH] net: tcp: add correct check for tcp_retransmit_skb()

[Yuchung Cheng]

On Fri, Nov 30, 2018 at 10:28 AM Sharath Chandra Vurukala
<sharathv@codeaurora.org> wrote:
>
> when the tcp_retranmission_timer expires and tcp_retranmsit_skb is
> called if the retranmsission fails due to local congestion,
> backoff should not incremented.
>
> tcp_retransmit_skb() returns non-zero negative value in some cases of
> failure but the caller tcp_retransmission_timer() has a check for
> failure which checks if the return value is greater than zero.
> The check is corrected to check for non-zero value.
>
> Signed-off-by: Sharath Chandra Vurukala <sharathv@codeaurora.org>
Perhaps my previous comment was not clear: your bug-fix patch is incorrect.

On local congestion, tcp_retransmit_skb returns positive values
*only*. negative values do not indicate local congestion.

> ---
>  net/ipv4/tcp_timer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
> index 091c5392..c19f371 100644
> --- a/net/ipv4/tcp_timer.c
> +++ b/net/ipv4/tcp_timer.c
> @@ -511,7 +511,7 @@ void tcp_retransmit_timer(struct sock *sk)
>
>         tcp_enter_loss(sk);
>
> -       if (tcp_retransmit_skb(sk, tcp_rtx_queue_head(sk), 1) > 0) {
> +       if (tcp_retransmit_skb(sk, tcp_rtx_queue_head(sk), 1) != 0) {
>                 /* Retransmission failed because of local congestion,
>                  * do not backoff.
>                  */
> --
> 1.9.1
>

[PATCH] net: tcp: add correct check for tcp_retransmit_skb()

[Sharath Chandra Vurukala]

when the tcp_retranmission_timer expires and tcp_retranmsit_skb is
called if the retranmsission fails due to local congestion,
backoff should not incremented.

tcp_retransmit_skb() returns non-zero negative value in some cases of
failure but the caller tcp_retransmission_timer() has a check for
failure which checks if the return value is greater than zero.
The check is corrected to check for non-zero value.

Change-Id: I494fed73b2e385216402c91e9558d5c2884add5b
Signed-off-by: Sharath Chandra Vurukala <sharathv@codeaurora.org>
---
 net/ipv4/tcp_timer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
index 4be66e4..a70b4a9 100644
--- a/net/ipv4/tcp_timer.c
+++ b/net/ipv4/tcp_timer.c
@@ -536,7 +536,7 @@ void tcp_retransmit_timer(struct sock *sk)
 
 	tcp_enter_loss(sk);
 
-	if (tcp_retransmit_skb(sk, tcp_write_queue_head(sk), 1) > 0) {
+	if (tcp_retransmit_skb(sk, tcp_write_queue_head(sk), 1) != 0) {
 		/* Retransmission failed because of local congestion,
 		 * do not backoff.
 		 */
-- 
1.9.1

Re: [PATCH] net: tcp: add correct check for tcp_retransmit_skb()

[Joey Pabalinas]

[-- Attachment #1: Type: text/plain, Size: 842 bytes --]

On Mon, Nov 26, 2018 at 03:05:50PM +0530, Sharath Chandra Vurukala wrote:
> when the tcp_retranmission_timer expires and tcp_retranmsit_skb is
> called if the retranmsission fails due to local congestion,
> backoff should not incremented.
> 
> tcp_retransmit_skb() returns non-zero negative value in some cases of
> failure but the caller tcp_retransmission_timer() has a check for
> failure which checks if the return value is greater than zero.
> The check is corrected to check for non-zero value.
> 
> Change-Id: I494fed73b2e385216402c91e9558d5c2884add5b
> Signed-off-by: Sharath Chandra Vurukala <sharathv@codeaurora.org>

You are right, tcp_retransmit_skb() calls `int err = __tcp_retransmit_skb()`
which returns non-zero on error.

Reviewed-by: Joey Pabalinas <joeypabalinas@gmail.com>

-- 
Cheers,
Joey Pabalinas

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] net: tcp: add correct check for tcp_retransmit_skb()

[David Miller]

From: Sharath Chandra Vurukala <sharathv@codeaurora.org>
Date: Mon, 26 Nov 2018 15:05:50 +0530

> when the tcp_retranmission_timer expires and tcp_retranmsit_skb is
> called if the retranmsission fails due to local congestion,
> backoff should not incremented.
> 
> tcp_retransmit_skb() returns non-zero negative value in some cases of
> failure but the caller tcp_retransmission_timer() has a check for
> failure which checks if the return value is greater than zero.
> The check is corrected to check for non-zero value.
> 
> Change-Id: I494fed73b2e385216402c91e9558d5c2884add5b

No changeids please.

> Signed-off-by: Sharath Chandra Vurukala <sharathv@codeaurora.org>

This patch doesn't apply cleanly to my net tree.

Re: [PATCH] net: tcp: add correct check for tcp_retransmit_skb()

[Yuchung Cheng]

On Mon, Nov 26, 2018 at 1:35 AM, Sharath Chandra Vurukala
<sharathv@codeaurora.org> wrote:
> when the tcp_retranmission_timer expires and tcp_retranmsit_skb is
> called if the retranmsission fails due to local congestion,
> backoff should not incremented.
>
> tcp_retransmit_skb() returns non-zero negative value in some cases of
> failure but the caller tcp_retransmission_timer() has a check for
> failure which checks if the return value is greater than zero.
> The check is corrected to check for non-zero value.
Not sure about this fix. The specific check is to handle local
congestion which is only indicated by positive return values.

>
> Change-Id: I494fed73b2e385216402c91e9558d5c2884add5b
> Signed-off-by: Sharath Chandra Vurukala <sharathv@codeaurora.org>
> ---
>  net/ipv4/tcp_timer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
> index 4be66e4..a70b4a9 100644
> --- a/net/ipv4/tcp_timer.c
> +++ b/net/ipv4/tcp_timer.c
> @@ -536,7 +536,7 @@ void tcp_retransmit_timer(struct sock *sk)
>
>         tcp_enter_loss(sk);
>
> -       if (tcp_retransmit_skb(sk, tcp_write_queue_head(sk), 1) > 0) {
> +       if (tcp_retransmit_skb(sk, tcp_write_queue_head(sk), 1) != 0) {
>                 /* Retransmission failed because of local congestion,
>                  * do not backoff.
>                  */
> --
> 1.9.1
>