[PATCH v3] tcp: fix tp->undo_retrans accounting in tcp_sacktag_one()

Subsystems: networking [general], networking [tcp], the rest

STALE1846d

6 messages, 5 authors, 2021-09-14 · open the first message on its own page

[PATCH v3] tcp: fix tp->undo_retrans accounting in tcp_sacktag_one()

From: zhenggy <hidden>
Date: 2021-09-13 10:41:11 

Commit 10d3be569243 ("tcp-tso: do not split TSO packets at retransmit
time") may directly retrans a multiple segments TSO/GSO packet without
split, Since this commit, we can no longer assume that a retransmitted
packet is a single segment.

This patch fixes the tp->undo_retrans accounting in tcp_sacktag_one()
that use the actual segments(pcount) of the retransmitted packet.

Before that commit (10d3be569243), the assumption underlying the
tp->undo_retrans-- seems correct.

Fixes: 10d3be569243 ("tcp-tso: do not split TSO packets at retransmit time")

Signed-off-by: zhenggy <redacted>
---
 net/ipv4/tcp_input.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 3f7bd7a..141e85e 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -1346,7 +1346,7 @@ static u8 tcp_sacktag_one(struct sock *sk,
 	if (dup_sack && (sacked & TCPCB_RETRANS)) {
 		if (tp->undo_marker && tp->undo_retrans > 0 &&
 		    after(end_seq, tp->undo_marker))
-			tp->undo_retrans--;
+			tp->undo_retrans = max_t(int, 0, tp->undo_retrans - pcount);
 		if ((sacked & TCPCB_SACKED_ACKED) &&
 		    before(start_seq, state->reord))
 				state->reord = start_seq;
-- 
1.8.3.1

Re: [PATCH v3] tcp: fix tp->undo_retrans accounting in tcp_sacktag_one()

From: Jakub Kicinski <kuba@kernel.org>
Date: 2021-09-13 15:52:20 

On Mon, 30 Aug 2021 05:00:10 -0400 zhenggy wrote:
Commit 10d3be569243 ("tcp-tso: do not split TSO packets at retransmit
time") may directly retrans a multiple segments TSO/GSO packet without
split, Since this commit, we can no longer assume that a retransmitted
packet is a single segment.

This patch fixes the tp->undo_retrans accounting in tcp_sacktag_one()
that use the actual segments(pcount) of the retransmitted packet.

Before that commit (10d3be569243), the assumption underlying the
tp->undo_retrans-- seems correct.

Fixes: 10d3be569243 ("tcp-tso: do not split TSO packets at retransmit time")
Please remove this empty line. There should be no empty lines between
tags.

Signed-off-by: zhenggy <redacted>
Please fix the data on your system and repost. I'm pretty sure this
wasn't posted on Aug 30th.

Re: [PATCH v3] tcp: fix tp->undo_retrans accounting in tcp_sacktag_one()

From: Eric Dumazet <hidden>
Date: 2021-09-13 15:52:21 

On Mon, Sep 13, 2021 at 3:51 AM zhenggy [off-list ref] wrote:
Commit 10d3be569243 ("tcp-tso: do not split TSO packets at retransmit
time") may directly retrans a multiple segments TSO/GSO packet without
split, Since this commit, we can no longer assume that a retransmitted
packet is a single segment.

This patch fixes the tp->undo_retrans accounting in tcp_sacktag_one()
that use the actual segments(pcount) of the retransmitted packet.

Before that commit (10d3be569243), the assumption underlying the
tp->undo_retrans-- seems correct.

Fixes: 10d3be569243 ("tcp-tso: do not split TSO packets at retransmit time")
nit: We normally do not add an empty line between Fixes: tag and others.

Reviewed-by: Eric Dumazet <redacted>
quoted hunk
Signed-off-by: zhenggy <redacted>
---
 net/ipv4/tcp_input.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 3f7bd7a..141e85e 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -1346,7 +1346,7 @@ static u8 tcp_sacktag_one(struct sock *sk,
        if (dup_sack && (sacked & TCPCB_RETRANS)) {
                if (tp->undo_marker && tp->undo_retrans > 0 &&
                    after(end_seq, tp->undo_marker))
-                       tp->undo_retrans--;
+                       tp->undo_retrans = max_t(int, 0, tp->undo_retrans - pcount);
                if ((sacked & TCPCB_SACKED_ACKED) &&
                    before(start_seq, state->reord))
                                state->reord = start_seq;
--
1.8.3.1

Re: [PATCH v3] tcp: fix tp->undo_retrans accounting in tcp_sacktag_one()

From: Yuchung Cheng <hidden>
Date: 2021-09-13 16:11:07 

On Mon, Sep 13, 2021 at 8:49 AM Eric Dumazet [off-list ref] wrote:
On Mon, Sep 13, 2021 at 3:51 AM zhenggy [off-list ref] wrote:
quoted
Commit 10d3be569243 ("tcp-tso: do not split TSO packets at retransmit
time") may directly retrans a multiple segments TSO/GSO packet without
split, Since this commit, we can no longer assume that a retransmitted
packet is a single segment.

This patch fixes the tp->undo_retrans accounting in tcp_sacktag_one()
that use the actual segments(pcount) of the retransmitted packet.

Before that commit (10d3be569243), the assumption underlying the
tp->undo_retrans-- seems correct.

Fixes: 10d3be569243 ("tcp-tso: do not split TSO packets at retransmit time")
nit: We normally do not add an empty line between Fixes: tag and others.

Reviewed-by: Eric Dumazet <redacted>
Acked-by: Yuchung Cheng <redacted>

This is a nice fix that would increase the effectiveness of TCP undo
in high-speed networks.

quoted
Signed-off-by: zhenggy <redacted>
---
 net/ipv4/tcp_input.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 3f7bd7a..141e85e 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -1346,7 +1346,7 @@ static u8 tcp_sacktag_one(struct sock *sk,
        if (dup_sack && (sacked & TCPCB_RETRANS)) {
                if (tp->undo_marker && tp->undo_retrans > 0 &&
                    after(end_seq, tp->undo_marker))
-                       tp->undo_retrans--;
+                       tp->undo_retrans = max_t(int, 0, tp->undo_retrans - pcount);
                if ((sacked & TCPCB_SACKED_ACKED) &&
                    before(start_seq, state->reord))
                                state->reord = start_seq;
--
1.8.3.1





  

    
Re: [PATCH v3] tcp: fix tp->undo_retrans accounting in tcp_sacktag_one()

    

      From: Neal Cardwell <ncardwell@google.com>

      Date: 2021-09-13 16:28:06 
    

  

  
On Mon, Sep 13, 2021 at 12:11 PM Yuchung Cheng [off-list ref] wrote:
On Mon, Sep 13, 2021 at 8:49 AM Eric Dumazet [off-list ref] wrote:
quoted
On Mon, Sep 13, 2021 at 3:51 AM zhenggy [off-list ref] wrote:
quoted
Commit 10d3be569243 ("tcp-tso: do not split TSO packets at retransmit
time") may directly retrans a multiple segments TSO/GSO packet without
split, Since this commit, we can no longer assume that a retransmitted
packet is a single segment.

This patch fixes the tp->undo_retrans accounting in tcp_sacktag_one()
that use the actual segments(pcount) of the retransmitted packet.

Before that commit (10d3be569243), the assumption underlying the
tp->undo_retrans-- seems correct.

Fixes: 10d3be569243 ("tcp-tso: do not split TSO packets at retransmit time")
nit: We normally do not add an empty line between Fixes: tag and others.

Reviewed-by: Eric Dumazet <redacted>
Acked-by: Yuchung Cheng <redacted>

This is a nice fix that would increase the effectiveness of TCP undo
in high-speed networks.
Yes, thanks for the fix!

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

neal





  

    
Re: [PATCH v3] tcp: fix tp->undo_retrans accounting in tcp_sacktag_one()

    

      From: zhenggy <hidden>

      Date: 2021-09-14 00:18:14 
    

  

  
Thanks very much for all your comments, i will repost a v4 patch later.

在 2021/9/14 0:27, Neal Cardwell 写道:
On Mon, Sep 13, 2021 at 12:11 PM Yuchung Cheng [off-list ref] wrote:
quoted
On Mon, Sep 13, 2021 at 8:49 AM Eric Dumazet [off-list ref] wrote:
quoted
On Mon, Sep 13, 2021 at 3:51 AM zhenggy [off-list ref] wrote:
quoted
Commit 10d3be569243 ("tcp-tso: do not split TSO packets at retransmit
time") may directly retrans a multiple segments TSO/GSO packet without
split, Since this commit, we can no longer assume that a retransmitted
packet is a single segment.

This patch fixes the tp->undo_retrans accounting in tcp_sacktag_one()
that use the actual segments(pcount) of the retransmitted packet.

Before that commit (10d3be569243), the assumption underlying the
tp->undo_retrans-- seems correct.

Fixes: 10d3be569243 ("tcp-tso: do not split TSO packets at retransmit time")
nit: We normally do not add an empty line between Fixes: tag and others.

Reviewed-by: Eric Dumazet <redacted>
Acked-by: Yuchung Cheng <redacted>

This is a nice fix that would increase the effectiveness of TCP undo
in high-speed networks.
Yes, thanks for the fix!

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

neal




    
  

  
    

      

        Keyboard shortcuts
      

      

        
          
hback out one level

          
jnext message in thread

          
kprevious message in thread

          
ldrill in

          
Escclose help / fold thread tree

          
?toggle this help