From: Neil Ramaswamy <[email protected]>
Thanks for the feedback Eric and the patch Neal. I took some time to read
through the code, and I have a few questions.
First, I think the core invariant we're trying to maintain is that
tsorted_sent_queue and tsorted_lost_queue are always themselves sorted by
transmission timestamp. A nice property to have would be that all the
skbs in tsorted_lost_queue precede everything in tsorted_sent_queue in
transmission order so that partial undo can always list splice these
together, but there's the edge case you point out:
> The "often" is the RTO case: tcp_timeout_mark_lost() leaves the skbs it
> marks in tsorted_sent_queue, and they can be older than what RACK moves
> to the lost queue afterwards.
I'm struggling to see why RACK cannot move all LOST (going to use this as
shorthand instead of TCPCB_LOST) and RACK-eligible segments from
tsorted_sent_queue to the lost queue; I think that this would actually
maintain the simpler invariant that tsorted and tlost are both sorted and
that all of tlost precedes tsorted, allowing for O(1) splice. To do
this, I believe that we would have to remove:
/* Skip ones marked lost but not yet retransmitted */
if ((scb->sacked & TCPCB_LOST) &&
!(scb->sacked & TCPCB_SACKED_RETRANS))
continue;
from tcp_rack_detect_loss, and instead continue to check whether remaining
<= 0; if it is, we add the segment to a local queue, which we then can
splice to the end of the lost queue at the end of the function. We would
probably want to retain the skip for LOST segments without SACKED_RETRANS
when remaining > 0 to avoid updating reo_timeout.
It seems like this would also prevent subsequent RACK scans from having to
ever reconsider this segment, since it would have been moved out to the
LOST list. But mostly, this seems like a conceptual simplification. Perhaps
it's undesirable to have RACK be moving around segments marked LOST by the
RTO, but I will leave that call to someone else.
Finally, I handed the invariant that both lists are always sorted to an LLM
and asked it to try to find exceptions. I think it provided a plausible
suggestion that the RTX scan for retransmission can see a segment that is
LOST, attempt to retransmit it in __tcp_transmit_skb, update its
transmission timestamp, and then fail with -ENOBUFS, leaving the new
timestamp there. This could cause either list to have a timestamp that's
out-of-order which can cause a subsequent RACK scan to stop early and miss
segements.
I'm not sure if restoring the timestamp to what it was before the failure
is safe. I also couldn't figure out how to get packetdrill to simulate an
error like this so I'm not sure how to create a repro for you. Let me know
if I should write out a sequence of events.
> in case that is useful for discussion or using. It also proposed 2 packetdrill
> tests (one that claims "This is the case that Eric suggested for segments with
> TCPCB_EVER_RETRANS."), which I can post if there is interest.
Did it propose 2 different packetdrill tests? If I understand Eric's
suggestion correctly, he's proposing to just take the test from 3/3 and
modify its final lines to ensure prompt retransmission without an RTO. Are
your LLM's proposed tests covering cases beyond this?
Thanks.
