[PATCH] [TCP]: Rexmit hint must be cleared instead of setting it

[PATCH] [TCP]: Rexmit hint must be cleared instead of setting it

[Ilpo Järvinen]

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

Stupid error from my side. Even though now that I noticed this,
I hoped it would have been an optimization but no, the counter
hint is then incorrect. Thus clearing is necessary for now (I
still suspect though that this path is never executed).

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>
---
 net/ipv4/tcp_input.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 7a59ffe..c855791 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -1763,7 +1763,7 @@ static void tcp_verify_retransmit_hint(s
 	    !(TCP_SKB_CB(skb)->sacked&TCPCB_SACKED_RETRANS) &&
 	    before(TCP_SKB_CB(skb)->seq,
 	    TCP_SKB_CB(tp->retransmit_skb_hint)->seq))
-		tp->retransmit_skb_hint = skb;
+		tp->retransmit_skb_hint = NULL;
 }
 
 /* Forward walk starting from until a not timedout skb is encountered, timeout
-- 
1.4.2

Re: [PATCH] [TCP]: Rexmit hint must be cleared instead of setting it

[David Miller]

From: "Ilpo_Järvinen" <ilpo.jarvinen@helsinki.fi>
Date: Wed, 28 Mar 2007 16:31:50 +0300 (EEST)

> Stupid error from my side. Even though now that I noticed this,
> I hoped it would have been an optimization but no, the counter
> hint is then incorrect. Thus clearing is necessary for now (I
> still suspect though that this path is never executed).
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>

Better safe than sorry :-)

We can start putting more aggressive assertions around if you'd
like to get some invariants like that validated.

Re: [PATCH] [TCP]: Rexmit hint must be cleared instead of setting it

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Wed, 28 Mar 2007 12:07:09 -0700 (PDT)

> From: "Ilpo_Järvinen" <ilpo.jarvinen@helsinki.fi>
> Date: Wed, 28 Mar 2007 16:31:50 +0300 (EEST)
> 
> > Stupid error from my side. Even though now that I noticed this,
> > I hoped it would have been an optimization but no, the counter
> > hint is then incorrect. Thus clearing is necessary for now (I
> > still suspect though that this path is never executed).
> > 
> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>
> 
> Better safe than sorry :-)
> 
> We can start putting more aggressive assertions around if you'd
> like to get some invariants like that validated.

In case it's not clear I did apply this patch.

Re: [PATCH] [TCP]: Rexmit hint must be cleared instead of setting it

[Ilpo Järvinen]

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

On Wed, 28 Mar 2007, David Miller wrote:

> From: David Miller <davem@davemloft.net>
> Date: Wed, 28 Mar 2007 12:07:09 -0700 (PDT)
> 
> > From: "Ilpo_Järvinen" <ilpo.jarvinen@helsinki.fi>
> > Date: Wed, 28 Mar 2007 16:31:50 +0300 (EEST)
> > 
> > > Stupid error from my side. Even though now that I noticed this,
> > > I hoped it would have been an optimization but no, the counter
> > > hint is then incorrect. Thus clearing is necessary for now (I
> > > still suspect though that this path is never executed).
> > > 
> > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>
> > 
> > Better safe than sorry :-)
> > 
> > We can start putting more aggressive assertions around if you'd
> > like to get some invariants like that validated.
> 
> In case it's not clear I did apply this patch.

I think more this on Friday, maybe WARN_ON could be placed there so that 
no harm is being done if it ever get there, probably a candidate for 
unlikely, if this is really needed. Anyway, applying the NULL this patch 
does no harm (it was supposed to be that way right from the
beginning)... :-)

...but lets keep in mind that the actual goal is, of course, to get rid of 
the hint altogether, rather than doing these clearing things... :-)

-- 
 i.

Re: [PATCH] [TCP]: Rexmit hint must be cleared instead of setting it

[David Miller]

From: "Ilpo_Järvinen" <ilpo.jarvinen@helsinki.fi>
Date: Wed, 28 Mar 2007 22:29:05 +0300 (EEST)

> ...but lets keep in mind that the actual goal is, of course, to get rid of 
> the hint altogether, rather than doing these clearing things... :-)

Of course.

The retranmit and forward SKB hints should be easy to kill.
In the worst case we can use the cached SACK sequence numbers
and perhaps one auxiliary sequence number hint to guide the
RB tree search for SKBs to retransmit.

A space intensive, and therefore not very appealing, scheme
is to have a linked list of SKBs which have been marked lost.