Re: [RFC][PATCH 1/2] TCP: fix lost retransmit detection

["Ilpo Järvinen" <ilpo.jarvinen@helsinki.fi>]

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

On Tue, 9 Oct 2007, TAKANO Ryousei wrote:

> From: "Ilpo Järvinen" <ilpo.jarvinen@helsinki.fi>
> Subject: Re: [RFC][PATCH 1/2] TCP: fix lost retransmit detection
> Date: Mon, 8 Oct 2007 14:11:55 +0300 (EEST)
> 
> > On Sun, 7 Oct 2007, TAKANO Ryousei wrote:
> > 
> > > From: "Ilpo Järvinen" <ilpo.jarvinen@helsinki.fi>
> > > 
> > > > Just couple of thoughts, not that this change itself is incorrect...

...Added this line back, it's important. :-)

> > > > In case sacktag uses fastpath, this code won't be executed for the skb's 
> > > > that we would like to check (those with SACKED_RETRANS set, that are 
> > > > below the fastpath_skb_hint). We will eventually deal with the whole queue 
> > > > when fastpath_skb_hint gets set to NULL, with the next cumulative ACK that 
> > > > fully ACKs an skb at the latest. Maybe there's a need for a larger surgery 
> > > > than this to fix it. I think we need additional field to tcp_sock to avoid 
> > > > doing a full-walk per ACK:
> > >
> > > I think the problem occurs in slowpath. For example, in case when the receiver
> > > detects and sends back a new SACK block, the sender may fail to detect loss
> > > of a retransmitted packet.
> > 
> > ...No, the slow path is very likely to _correct_ the problem! The problem 
> > occurs because fastpath _skips_ processing of skbs below fastpath_skb_hint 
> > whereas slowpath starts from tcp_write_queue_head. The retransmitted skbs 
> > we're interested in reside exactly on that skipped range.
> > 
> Sorry, my explanaton is insufficient.
> This problem occurs even if we are in the slowpath.
>
> [...long explination snip...]
>
> Is it corrent?

Yes, we're dealing with two related, but different problems here. I 
understood well what you're trying to fix, and even acknowledged your 
change (see what I added back from my original reply above). The far
worse problem which I describe, however, occurs with fastpath only (well, 
very unlikely to occur with slow-path but it could in theory), has been 
like this since the hints were added. And it's preventing the execution
of this portion you fixed (until slowpath is taken).

...What makes the other problem even nastier is the fact that it
becomes more and more significant when fastpath (or it's equivalent)
skips more and more skb processing. It's good that we noticed it
now...

I tried to do a fix to that other problem, it seems that the solution
will be intertwined with the problem you're describing so that I in
fact ended removing the code block where your key modification is and
placing  somewhat similar sequence gathering elsewhere... Posting it 
shortly.

-- 
 i.