| Message ID | 1630314010-15792-1-git-send-email-zhenggy@chinatelecom.cn (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v3] tcp: fix tp->undo_retrans accounting in tcp_sacktag_one() | expand |
On Mon, Sep 13, 2021 at 3:51 AM zhenggy <zhenggy@chinatelecom.cn> 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 <edumazet@google.com> > Signed-off-by: zhenggy <zhenggy@chinatelecom.cn> > --- > 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 >
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 <zhenggy@chinatelecom.cn> Please fix the data on your system and repost. I'm pretty sure this wasn't posted on Aug 30th.
On Mon, Sep 13, 2021 at 8:49 AM Eric Dumazet <edumazet@google.com> wrote: > > On Mon, Sep 13, 2021 at 3:51 AM zhenggy <zhenggy@chinatelecom.cn> 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 <edumazet@google.com> Acked-by: Yuchung Cheng <ycheng@google.com> This is a nice fix that would increase the effectiveness of TCP undo in high-speed networks. > > > Signed-off-by: zhenggy <zhenggy@chinatelecom.cn> > > --- > > 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 > >
On Mon, Sep 13, 2021 at 12:11 PM Yuchung Cheng <ycheng@google.com> wrote: > > On Mon, Sep 13, 2021 at 8:49 AM Eric Dumazet <edumazet@google.com> wrote: > > > > On Mon, Sep 13, 2021 at 3:51 AM zhenggy <zhenggy@chinatelecom.cn> 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 <edumazet@google.com> > Acked-by: Yuchung Cheng <ycheng@google.com> > > 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
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 <ycheng@google.com> wrote: >> >> On Mon, Sep 13, 2021 at 8:49 AM Eric Dumazet <edumazet@google.com> wrote: >>> >>> On Mon, Sep 13, 2021 at 3:51 AM zhenggy <zhenggy@chinatelecom.cn> 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 <edumazet@google.com> >> Acked-by: Yuchung Cheng <ycheng@google.com> >> >> 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 >
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;
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 <zhenggy@chinatelecom.cn> --- net/ipv4/tcp_input.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)