Re: [PATCH net-2.6 2/2] [TCP]: Bidir flow must not disregard SACK blocks for lost marking

[David Miller <davem@davemloft.net>]

From: "Ilpo_Järvinen" <ilpo.jarvinen@helsinki.fi>
Date: Mon, 30 Jul 2007 20:19:40 +0300 (EEST)

> It's possible that new SACK blocks that should trigger new LOST
> markings arrive with new data (which previously made is_dupack
> false). In addition, I think this fixes a case where we get
> a cumulative ACK with enough SACK blocks to trigger the fast
> recovery (is_dupack would be false there too).
> 
> I'm not completely pleased with this solution because readability
> of the code is somewhat questionable as 'is_dupack' in SACK case
> is no longer about dupacks only but would mean something like
> 'lost_marker_work_todo' too... But because of Eifel stuff done
> in CA_Recovery, the FLAG_DATA_SACKED check cannot be placed to
> the if statement which seems attractive solution. Nevertheless,
> I didn't like adding another variable just for that either... :-)
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>

I've applied this as well.

I don't mind the complex conditionals so much in loss
handling, they are almost inevitable.  However I believe
they could be simplified as a lot of pieces of code ask
similar if not identical questions.

We could ask several of these things up-front, regardless
of path we will take (reno, DSACK, reorder, FRTO, etc.)
and pass the answers along in a bitmask.  We do that to
some extent already with how we analyze the retransmit
queue at the beginning of ACK processing.