Maintaining dupack counter per hole (was: The trouble with sack..)

[Jonathan Looney]

On 11/16/15, 1:06 PM, "hiren panchasara" <hiren at strugglingcoder.info>
wrote:

>Getting back to this old thread...
>
>On 10/30/15 at 01:09P, Jonathan Looney wrote:
>>The SACK hole-tracking code is already quite complex. If we're going to
>> make a fundamental change, perhaps it is time to consider a rewrite,
>> rather than a smaller patch? Maybe this is the best code we can write.
>>Or,
>> maybe it is time for a re-coding to make it more easily accessible.
>
>As Randall said (in response to this), that rewrite is not necessary. And
>I agree to that. I don't see tcp_sack.c being in that bad shape that it
>demands a rewrite. But ofc, if you have something really cleaner/faster
>in mind, I'm all ears. :-)

I don't have anything better in mind. :-)

In fact, I think the code performs pretty well. And, that may be because
it prioritizes performance over readability. But, since I think we've all
stared at this code for a long time trying to figure out how it works
(note the repetitive comments about obscure code), I think it is worth
considering whether it is time to try to rewrite it to be more readable.

Having said that, I don't see any evidence that now is the time. I just
wanted to prompt the discussion. :-)


>> 
>> Do you charge all three holes with the duplicate ACKs? Do you copy the
>> counter to the holes?
>> 
>> Or, is the fact that the ACK is slightly different enough to reset the
>> counter?
>
>Basically, whenever a hole gets broken up, subholes carry-forward the
>dupack strike counter.

This somewhat makes sense. And, it looks like you are charging all
existing holes for each ACK that doesn't cover them (regardless of the
fact that it isn't really a "duplicate ACK" in this case). That also
generally makes sense.

But, I think we could change this to help cases where packets are
re-ordered.



> 
>> 
>> If you reset the counter anytime the hole is broken up, it will take a
>> while to get to three in a really out-of-order network scenario. On the
>> other hand, if you don't reset the counter, you may retransmit too fast.
>
>By 'too fast', I think you mean spurious retransmissions. If so, can you
>explain a bit more?

Sure.

Imagine that my network is load-balancing over a 4xGE bundle, but the
bundle links have different loads and slightly re-order the packets.

The packets arrive at the remote side out of order, like this:

4500:5999

1500:2999

3000:4499

0:1499

The remote side ACKs them in that order and we receive the ACKs in that
order.

After the third ACK, we will view the 0:1499 block as having three strikes
and will retransmit it (assuming that is our threshold), even though the
ACK will arrive momentarily.

Of course, I think that problem already exists with our code today. But,
my question is how the per-hole counter improves this. I think the bottom
line is that our code today assumes that we need to do a SACK retransmit
(for all outstanding SACK holes) as soon as the first outstanding byte is
not ACKd for three ACKs in a row.

If we instead reset the counter for a hole anytime the hole was partially
ACKd (including being split), we would retransmit less aggressively. In
this example, that would probably be good.

A middle-ground is to charge all holes every time we receive an ACK that
does not partially ACK the hole. If a hole is partially ACKd, neither
charge the ACK with a strike nor reset the counter.

In any case, I think a rule that any particular byte must be ACKd in one
of the next three packets after a higher byte is ACKd is potentially too
rigid given the myriad ways packets can be re-ordered.

Jonathan