[PATCH] tcp: undo_retrans counter fixes

[PATCH] tcp: undo_retrans counter fixes

[Yuchung Cheng]

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(-)

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().
-- 
1.7.3.1

Re: [PATCH] tcp: undo_retrans counter fixes

[David Miller]

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.

Re: [PATCH] tcp: undo_retrans counter fixes

[Ilpo Järvinen]

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).

-- 
 i.

Re: [PATCH] tcp: undo_retrans counter fixes

[Yuchung Cheng]

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.

Re: [PATCH] tcp: undo_retrans counter fixes

[Ilpo Järvinen]

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

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.

-- 
 i.

Re: [PATCH] tcp: undo_retrans counter fixes

[Yuchung Cheng]

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.

Re: [PATCH] tcp: undo_retrans counter fixes

[Ilpo Järvinen]

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

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.



-- 
 i.

Re: [PATCH] tcp: undo_retrans counter fixes

[Yuchung Cheng]

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

Re: [PATCH] tcp: undo_retrans counter fixes

[David Miller]

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 ;)

Re: [PATCH] tcp: undo_retrans counter fixes

[David Miller]

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.