| Message ID | 1297119424-19956-1-git-send-email-ycheng@google.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Yuchung Cheng <ycheng@google.com> Date: Mon, 7 Feb 2011 14:57:04 -0800 > Fix a bug that undo_retrans is incorrectly decremented when undo_marker is > not set or undo_retrans is already 0. This happens when sender receives > more DSACK ACKs than packets retransmitted during the current > undo phase. This may also happen when sender receives DSACK after > the undo operation is completed or cancelled. > > Fix another bug that undo_retrans is incorrectly incremented when > sender retransmits an skb and tcp_skb_pcount(skb) > 1 (TSO). This case > is rare but not impossible. > > Signed-off-by: Yuchung Cheng <ycheng@google.com> Looks good, Ilpo could you please review this real quick? 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
On Mon, 7 Feb 2011, David Miller wrote: > From: Yuchung Cheng <ycheng@google.com> > Date: Mon, 7 Feb 2011 14:57:04 -0800 > > > Fix a bug that undo_retrans is incorrectly decremented when undo_marker is > > not set or undo_retrans is already 0. This happens when sender receives > > more DSACK ACKs than packets retransmitted during the current > > undo phase. This may also happen when sender receives DSACK after > > the undo operation is completed or cancelled. > > > > Fix another bug that undo_retrans is incorrectly incremented when > > sender retransmits an skb and tcp_skb_pcount(skb) > 1 (TSO). This case > > is rare but not impossible. > > > > Signed-off-by: Yuchung Cheng <ycheng@google.com> > > Looks good, Ilpo could you please review this real quick? I already too a quick look so you're real lucky, only delay of writing is needed... :-) Neither is harmful to "fix" but I think they're partially also checking for things which shouldn't cause problems... E.g., undo_retrans is only used after checking undo_marker's validity first so I don't think undo_marker check is exactly necessary there (but on the other hand it does no harm)... The tcp_retransmit_skb problem I don't understand at all as we should be fragmenting or resetting pcount to 1 (the latter is true only if all bugfixes were included to the kernel where >1 pcount for a rexmitted skb was seen). If pcount is indeed >1 we might have other issues too somewhere but I fail to remember immediately what they would be. That change is not bad though since using +/-1 is something we should be getting rid of anyway and on long term it would be nice to make tcp_retransmit_skb to be able to take advantage of TSO anyway whenever possible. I also noticed that the undo_retrans code in sacktag side is still doing undo_retrans-- ops which could certainly cause real miscounts, though it is extremely unlikely due to the fact that DSACK should be sent immediately for a single segment at a time (so the sender would need to split+recollapse in between).
On Mon, Feb 7, 2011 at 3:36 PM, Ilpo Järvinen <ilpo.jarvinen@helsinki.fi> wrote: > > On Mon, 7 Feb 2011, David Miller wrote: > > > From: Yuchung Cheng <ycheng@google.com> > > Date: Mon, 7 Feb 2011 14:57:04 -0800 > > > > > Fix a bug that undo_retrans is incorrectly decremented when undo_marker is > > > not set or undo_retrans is already 0. This happens when sender receives > > > more DSACK ACKs than packets retransmitted during the current > > > undo phase. This may also happen when sender receives DSACK after > > > the undo operation is completed or cancelled. > > > > > > Fix another bug that undo_retrans is incorrectly incremented when > > > sender retransmits an skb and tcp_skb_pcount(skb) > 1 (TSO). This case > > > is rare but not impossible. > > > > > > Signed-off-by: Yuchung Cheng <ycheng@google.com> > > > > Looks good, Ilpo could you please review this real quick? > > I already too a quick look so you're real lucky, only delay of writing is > needed... :-) thanks. > > Neither is harmful to "fix" but I think they're partially also checking > for things which shouldn't cause problems... E.g., undo_retrans is only > used after checking undo_marker's validity first so I don't think > undo_marker check is exactly necessary there (but on the other hand it > does no harm)... logically we should check the validity of undo_marker/undo_retrans before we use them? The current code has no problem if tcp_fastretrans_alert() always call tcp_try_undo_* functions whenever undo_marker != 0 and undo_retrans == 0. I don't think that's always true. > > The tcp_retransmit_skb problem I don't understand at all as we should be > fragmenting or resetting pcount to 1 (the latter is true only if all > bugfixes were included to the kernel where >1 pcount for a rexmitted skb > was seen). If pcount is indeed >1 we might have other issues too somewhere We found that sometimes pcount > 1 on real servers. This change keeps the retrans_out/undo_retrans counters consistent. > but I fail to remember immediately what they would be. That change is not > bad though since using +/-1 is something we should be getting rid of > anyway and on long term it would be nice to make tcp_retransmit_skb to be > able to take advantage of TSO anyway whenever possible. > > I also noticed that the undo_retrans code in sacktag side is still doing > undo_retrans-- ops which could certainly cause real miscounts, though > it is extremely unlikely due to the fact that DSACK should be sent > immediately for a single segment at a time (so the sender would need to > split+recollapse in between). I have the same doubt but our servers never hit this condition (pcount > 1). So I keep this part intact. > > -- > i. -- 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
On Mon, 7 Feb 2011, Yuchung Cheng wrote: > On Mon, Feb 7, 2011 at 3:36 PM, Ilpo Järvinen <ilpo.jarvinen@helsinki.fi> wrote: > > > > On Mon, 7 Feb 2011, David Miller wrote: > > > > > From: Yuchung Cheng <ycheng@google.com> > > > Date: Mon, 7 Feb 2011 14:57:04 -0800 > > > > > > > Fix a bug that undo_retrans is incorrectly decremented when undo_marker is > > > > not set or undo_retrans is already 0. This happens when sender receives > > > > more DSACK ACKs than packets retransmitted during the current > > > > undo phase. This may also happen when sender receives DSACK after > > > > the undo operation is completed or cancelled. > > > > > > > > Fix another bug that undo_retrans is incorrectly incremented when > > > > sender retransmits an skb and tcp_skb_pcount(skb) > 1 (TSO). This case > > > > is rare but not impossible. > > > > > > > > Signed-off-by: Yuchung Cheng <ycheng@google.com> > > > > Neither is harmful to "fix" but I think they're partially also checking > > for things which shouldn't cause problems... E.g., undo_retrans is only > > used after checking undo_marker's validity first so I don't think > > undo_marker check is exactly necessary there (but on the other hand it > > does no harm)... > > logically we should check the validity of undo_marker/undo_retrans > before we use them? The current code has no problem if > tcp_fastretrans_alert() always call tcp_try_undo_* functions whenever > undo_marker != 0 and undo_retrans == 0. I don't think that's always > true. We certainly should be letting the undo_retrans to underflow that in this your patch has merit (the added tp->undo_retrans check). However, the only users are: static inline int tcp_may_undo(struct tcp_sock *tp) { return tp->undo_marker && (!tp->undo_retrans ...) and: static void tcp_try_undo_dsack(struct sock *sk) { struct tcp_sock *tp = tcp_sk(sk); if (tp->undo_marker && !tp->undo_retrans) { ...which check that undo_retrans is valid. > > The tcp_retransmit_skb problem I don't understand at all as we should be > > fragmenting or resetting pcount to 1 (the latter is true only if all > > bugfixes were included to the kernel where >1 pcount for a rexmitted skb > > was seen). If pcount is indeed >1 we might have other issues too somewhere > > We found that sometimes pcount > 1 on real servers. This change keeps > the retrans_out/undo_retrans counters consistent. There's still some bug then I guess... It might be related to the issues seen by those other guys who were complaining about small segments with >1 pcount breaking their hardware (few months ago). For the record, the last fix is from 2.6.31 or so. Like I said, I don't oppose this change anyway: > > but I fail to remember immediately what they would be. That change is not > > bad though since using +/-1 is something we should be getting rid of > > anyway and on long term it would be nice to make tcp_retransmit_skb to be > > able to take advantage of TSO anyway whenever possible. ...it certainly won't hurt to be on the safe side here if/when something else is wrong. > > I also noticed that the undo_retrans code in sacktag side is still doing > > undo_retrans-- ops which could certainly cause real miscounts, though > > it is extremely unlikely due to the fact that DSACK should be sent > > immediately for a single segment at a time (so the sender would need to > > split+recollapse in between). > > I have the same doubt but our servers never hit this condition (pcount > 1). So I keep this part intact. I could think of some scenario you cannot even reproduce in a large scale tests, unlikely indeed :-). ...Or too stable connectivity on the sender side. But I've changed my mind... the -1 operation is the correct one as we could otherwise overestimate due to pcount=1->2 split after the retransmission that triggered the DSACK (now that I remember this, I think I've once thought this line through already earlier... I'll try to write a comment one day there). For -rc/next purposes I don't see any big enough reasons to withhold: Acked-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi> ...but if you want this to stables too I don't think it's minimal w.r.t. undo_marker check.
On Tue, Feb 8, 2011 at 1:54 AM, Ilpo Järvinen <ilpo.jarvinen@helsinki.fi> wrote: > > On Mon, 7 Feb 2011, Yuchung Cheng wrote: > > > On Mon, Feb 7, 2011 at 3:36 PM, Ilpo Järvinen <ilpo.jarvinen@helsinki.fi> wrote: > > > > > > On Mon, 7 Feb 2011, David Miller wrote: > > > > > > > From: Yuchung Cheng <ycheng@google.com> > > > > Date: Mon, 7 Feb 2011 14:57:04 -0800 > > > > > > > > > Fix a bug that undo_retrans is incorrectly decremented when undo_marker is > > > > > not set or undo_retrans is already 0. This happens when sender receives > > > > > more DSACK ACKs than packets retransmitted during the current > > > > > undo phase. This may also happen when sender receives DSACK after > > > > > the undo operation is completed or cancelled. > > > > > > > > > > Fix another bug that undo_retrans is incorrectly incremented when > > > > > sender retransmits an skb and tcp_skb_pcount(skb) > 1 (TSO). This case > > > > > is rare but not impossible. > > > > > > > > > > Signed-off-by: Yuchung Cheng <ycheng@google.com> > > > > > > Neither is harmful to "fix" but I think they're partially also checking > > > for things which shouldn't cause problems... E.g., undo_retrans is only > > > used after checking undo_marker's validity first so I don't think > > > undo_marker check is exactly necessary there (but on the other hand it > > > does no harm)... > > > > logically we should check the validity of undo_marker/undo_retrans > > before we use them? The current code has no problem if > > tcp_fastretrans_alert() always call tcp_try_undo_* functions whenever > > undo_marker != 0 and undo_retrans == 0. I don't think that's always > > true. > > We certainly should be letting the undo_retrans to underflow that in this > your patch has merit (the added tp->undo_retrans check). > > However, the only users are: > > static inline int tcp_may_undo(struct tcp_sock *tp) > { > return tp->undo_marker && (!tp->undo_retrans ...) > > and: > > static void tcp_try_undo_dsack(struct sock *sk) > { > struct tcp_sock *tp = tcp_sk(sk); > > if (tp->undo_marker && !tp->undo_retrans) { > > > ...which check that undo_retrans is valid. But that does not make this bug go away. The sender does not always call these functions in tcp_fastretrans_alert(). A common example is that the sender receives a DUPACK with DSACK option during CA_Recovery and undo_retrans goes to 0. Since it's not a partial ACK, no undo function is called (another bug?) while processing the DUPACK. If the sender receives another DSACK, undo_retrans underflows and the undo chance is missed forever. I think that's another potential bug in handling this situation but I want to fix in this boundary checks first. HTH > > > > The tcp_retransmit_skb problem I don't understand at all as we should be > > > fragmenting or resetting pcount to 1 (the latter is true only if all > > > bugfixes were included to the kernel where >1 pcount for a rexmitted skb > > > was seen). If pcount is indeed >1 we might have other issues too somewhere > > > > We found that sometimes pcount > 1 on real servers. This change keeps > > the retrans_out/undo_retrans counters consistent. > > There's still some bug then I guess... It might be related to the issues > seen by those other guys who were complaining about small segments with > >1 pcount breaking their hardware (few months ago). For the record, the > last fix is from 2.6.31 or so. > > Like I said, I don't oppose this change anyway: > > > > but I fail to remember immediately what they would be. That change is not > > > bad though since using +/-1 is something we should be getting rid of > > > anyway and on long term it would be nice to make tcp_retransmit_skb to be > > > able to take advantage of TSO anyway whenever possible. > > ...it certainly won't hurt to be on the safe side here if/when something > else is wrong. > > > > I also noticed that the undo_retrans code in sacktag side is still doing > > > undo_retrans-- ops which could certainly cause real miscounts, though > > > it is extremely unlikely due to the fact that DSACK should be sent > > > immediately for a single segment at a time (so the sender would need to > > > split+recollapse in between). > > > > I have the same doubt but our servers never hit this condition (pcount > > 1). So I keep this part intact. > > I could think of some scenario you cannot even reproduce in a large scale > tests, unlikely indeed :-). ...Or too stable connectivity on the sender > side. But I've changed my mind... the -1 operation is the correct one as > we could otherwise overestimate due to pcount=1->2 split after the > retransmission that triggered the DSACK (now that I remember this, I think > I've once thought this line through already earlier... I'll try to write a > comment one day there). > > For -rc/next purposes I don't see any big enough reasons to withhold: > > Acked-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi> > > ...but if you want this to stables too I don't think it's minimal w.r.t. > undo_marker check. > > -- > i. -- 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
On Thu, 10 Feb 2011, Yuchung Cheng wrote: > On Tue, Feb 8, 2011 at 1:54 AM, Ilpo Järvinen <ilpo.jarvinen@helsinki.fi> wrote: > > > > On Mon, 7 Feb 2011, Yuchung Cheng wrote: > > > > > On Mon, Feb 7, 2011 at 3:36 PM, Ilpo Järvinen <ilpo.jarvinen@helsinki.fi> wrote: > > > > > > > > On Mon, 7 Feb 2011, David Miller wrote: > > > > > > > > > From: Yuchung Cheng <ycheng@google.com> > > > > > Date: Mon, 7 Feb 2011 14:57:04 -0800 > > > > > > > > > > > Fix a bug that undo_retrans is incorrectly decremented when undo_marker is > > > > > > not set or undo_retrans is already 0. This happens when sender receives > > > > > > more DSACK ACKs than packets retransmitted during the current > > > > > > undo phase. This may also happen when sender receives DSACK after > > > > > > the undo operation is completed or cancelled. > > > > > > > > > > > > Fix another bug that undo_retrans is incorrectly incremented when > > > > > > sender retransmits an skb and tcp_skb_pcount(skb) > 1 (TSO). This case > > > > > > is rare but not impossible. > > > > > > > > > > > > Signed-off-by: Yuchung Cheng <ycheng@google.com> > > > > > > > > Neither is harmful to "fix" but I think they're partially also checking > > > > for things which shouldn't cause problems... E.g., undo_retrans is only > > > > used after checking undo_marker's validity first so I don't think > > > > undo_marker check is exactly necessary there (but on the other hand it > > > > does no harm)... > > > > > > logically we should check the validity of undo_marker/undo_retrans > > > before we use them? The current code has no problem if > > > tcp_fastretrans_alert() always call tcp_try_undo_* functions whenever > > > undo_marker != 0 and undo_retrans == 0. I don't think that's always > > > true. > > > > We certainly should be letting the undo_retrans to underflow that in this > > your patch has merit (the added tp->undo_retrans check). > > > > However, the only users are: > > > > static inline int tcp_may_undo(struct tcp_sock *tp) > > { > > return tp->undo_marker && (!tp->undo_retrans ...) > > > > and: > > > > static void tcp_try_undo_dsack(struct sock *sk) > > { > > struct tcp_sock *tp = tcp_sk(sk); > > > > if (tp->undo_marker && !tp->undo_retrans) { > > > > > > ...which check that undo_retrans is valid. > But that does not make this bug go away. > The sender does not always call these functions in tcp_fastretrans_alert(). > > A common example is that the sender receives a DUPACK with DSACK option > during CA_Recovery and undo_retrans goes to 0. Since it's not a partial ACK, > no undo function is called (another bug?) while processing the DUPACK. If the > sender receives another DSACK, undo_retrans underflows and the undo > chance is missed forever. But the underflow can be fixed without checking for tp->undo_marker with the tp->undo_retrans > 0 condition only before decrementing it, right (in sacktag code like you do)? ...I don't understand what that has to do with these functions being called from tcp_fastretrans_alert or not. > I think that's another potential bug in handling this situation but I > want to fix in > this boundary checks first.
On Tue, Feb 8, 2011 at 1:54 AM, Ilpo Järvinen <ilpo.jarvinen@helsinki.fi> wrote: > > On Mon, 7 Feb 2011, Yuchung Cheng wrote: > > > On Mon, Feb 7, 2011 at 3:36 PM, Ilpo Järvinen <ilpo.jarvinen@helsinki.fi> wrote: > > > > > > On Mon, 7 Feb 2011, David Miller wrote: > > > > > > > From: Yuchung Cheng <ycheng@google.com> > > > > Date: Mon, 7 Feb 2011 14:57:04 -0800 > > > > > > > > > Fix a bug that undo_retrans is incorrectly decremented when undo_marker is > > > > > not set or undo_retrans is already 0. This happens when sender receives > > > > > more DSACK ACKs than packets retransmitted during the current > > > > > undo phase. This may also happen when sender receives DSACK after > > > > > the undo operation is completed or cancelled. > > > > > > > > > > Fix another bug that undo_retrans is incorrectly incremented when > > > > > sender retransmits an skb and tcp_skb_pcount(skb) > 1 (TSO). This case > > > > > is rare but not impossible. > > > > > > > > > > Signed-off-by: Yuchung Cheng <ycheng@google.com> > > > > > > Neither is harmful to "fix" but I think they're partially also checking > > > for things which shouldn't cause problems... E.g., undo_retrans is only > > > used after checking undo_marker's validity first so I don't think > > > undo_marker check is exactly necessary there (but on the other hand it > > > does no harm)... > > > > logically we should check the validity of undo_marker/undo_retrans > > before we use them? The current code has no problem if > > tcp_fastretrans_alert() always call tcp_try_undo_* functions whenever > > undo_marker != 0 and undo_retrans == 0. I don't think that's always > > true. > > We certainly should be letting the undo_retrans to underflow that in this > your patch has merit (the added tp->undo_retrans check). > > However, the only users are: > > static inline int tcp_may_undo(struct tcp_sock *tp) > { > return tp->undo_marker && (!tp->undo_retrans ...) > > and: > > static void tcp_try_undo_dsack(struct sock *sk) > { > struct tcp_sock *tp = tcp_sk(sk); > > if (tp->undo_marker && !tp->undo_retrans) { > > > ...which check that undo_retrans is valid. > > > > The tcp_retransmit_skb problem I don't understand at all as we should be > > > fragmenting or resetting pcount to 1 (the latter is true only if all > > > bugfixes were included to the kernel where >1 pcount for a rexmitted skb > > > was seen). If pcount is indeed >1 we might have other issues too somewhere > > > > We found that sometimes pcount > 1 on real servers. This change keeps > > the retrans_out/undo_retrans counters consistent. > > There's still some bug then I guess... It might be related to the issues > seen by those other guys who were complaining about small segments with > >1 pcount breaking their hardware (few months ago). For the record, the > last fix is from 2.6.31 or so. > > Like I said, I don't oppose this change anyway: > > > > but I fail to remember immediately what they would be. That change is not > > > bad though since using +/-1 is something we should be getting rid of > > > anyway and on long term it would be nice to make tcp_retransmit_skb to be > > > able to take advantage of TSO anyway whenever possible. > > ...it certainly won't hurt to be on the safe side here if/when something > else is wrong. > > > > I also noticed that the undo_retrans code in sacktag side is still doing > > > undo_retrans-- ops which could certainly cause real miscounts, though > > > it is extremely unlikely due to the fact that DSACK should be sent > > > immediately for a single segment at a time (so the sender would need to > > > split+recollapse in between). > > > > I have the same doubt but our servers never hit this condition (pcount > > 1). So I keep this part intact. > > I could think of some scenario you cannot even reproduce in a large scale > tests, unlikely indeed :-). ...Or too stable connectivity on the sender > side. But I've changed my mind... the -1 operation is the correct one as > we could otherwise overestimate due to pcount=1->2 split after the > retransmission that triggered the DSACK (now that I remember this, I think > I've once thought this line through already earlier... I'll try to write a > comment one day there). > > For -rc/next purposes I don't see any big enough reasons to withhold: > > Acked-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi> > > ...but if you want this to stables too I don't think it's minimal w.r.t. > undo_marker check. > Dave/Ilpo: can this patch get applied or it needs more clarification/justification? Thanks. Yuchung -- 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
From: Yuchung Cheng <ycheng@google.com> Date: Fri, 11 Feb 2011 16:10:50 -0800 > Dave/Ilpo: can this patch get applied or it needs more > clarification/justification? Thanks. Ilpo and you are still discussing the merits of doing the checks in one place or another, and whether this is the best place to handle this issue. I also see no rush in applying a patch for an obscure hard to trigger issue that has been present for such a long time. It's in the patchwork queue, so don't worry it won't get lost ;) -- 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
From: Yuchung Cheng <ycheng@google.com> Date: Mon, 7 Feb 2011 14:57:04 -0800 > Fix a bug that undo_retrans is incorrectly decremented when undo_marker is > not set or undo_retrans is already 0. This happens when sender receives > more DSACK ACKs than packets retransmitted during the current > undo phase. This may also happen when sender receives DSACK after > the undo operation is completed or cancelled. > > Fix another bug that undo_retrans is incorrectly incremented when > sender retransmits an skb and tcp_skb_pcount(skb) > 1 (TSO). This case > is rare but not impossible. > > Signed-off-by: Yuchung Cheng <ycheng@google.com> Applied and queued up for -stable, 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
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c index 2f692ce..08ea735 100644 --- a/net/ipv4/tcp_input.c +++ b/net/ipv4/tcp_input.c @@ -1222,7 +1222,7 @@ static int tcp_check_dsack(struct sock *sk, struct sk_buff *ack_skb, } /* D-SACK for already forgotten data... Do dumb counting. */ - if (dup_sack && + if (dup_sack && tp->undo_marker && tp->undo_retrans && !after(end_seq_0, prior_snd_una) && after(end_seq_0, tp->undo_marker)) tp->undo_retrans--; @@ -1299,7 +1299,8 @@ static u8 tcp_sacktag_one(struct sk_buff *skb, struct sock *sk, /* Account D-SACK for retransmitted packet. */ if (dup_sack && (sacked & TCPCB_RETRANS)) { - if (after(TCP_SKB_CB(skb)->end_seq, tp->undo_marker)) + if (tp->undo_marker && tp->undo_retrans && + after(TCP_SKB_CB(skb)->end_seq, tp->undo_marker)) tp->undo_retrans--; if (sacked & TCPCB_SACKED_ACKED) state->reord = min(fack_count, state->reord); diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c index 406f320..dfa5beb 100644 --- a/net/ipv4/tcp_output.c +++ b/net/ipv4/tcp_output.c @@ -2162,7 +2162,7 @@ int tcp_retransmit_skb(struct sock *sk, struct sk_buff *skb) if (!tp->retrans_stamp) tp->retrans_stamp = TCP_SKB_CB(skb)->when; - tp->undo_retrans++; + tp->undo_retrans += tcp_skb_pcount(skb); /* snd_nxt is stored to detect loss of retransmitted segment, * see tcp_input.c tcp_sacktag_write_queue().
Fix a bug that undo_retrans is incorrectly decremented when undo_marker is not set or undo_retrans is already 0. This happens when sender receives more DSACK ACKs than packets retransmitted during the current undo phase. This may also happen when sender receives DSACK after the undo operation is completed or cancelled. Fix another bug that undo_retrans is incorrectly incremented when sender retransmits an skb and tcp_skb_pcount(skb) > 1 (TSO). This case is rare but not impossible. Signed-off-by: Yuchung Cheng <ycheng@google.com> --- net/ipv4/tcp_input.c | 5 +++-- net/ipv4/tcp_output.c | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-)