On Fri, May 03, 2024 at 10:43:52AM -0400, Jon Maloy wrote:On 2024-05-03 09:42, Stefano Brivio wrote: > On Thu, 2 May 2024 11:31:52 +1000 > David Gibson <david(a)gibson.dropbear.id.au> wrote:[snip]I'm struggling to follow the description above. As noted in my other mail, I think the problem here is that we can queue frames before we trigger the retransmit, but then send them and advance seq_to_tap after we trigger the retransmit.Right. It should be seq_to_tap....but Jon's comment refers to seq_from_tap (not seq_to_tap)? I'm confused./* Receive into buffers, don't dequeue until acknowledged by guest. */ do len = recvmsg(s, &mh_sock, MSG_PEEK); @@ -2195,7 +2220,10 @@ static int tcp_data_from_sock(struct ctx *c, struct tcp_tap_conn *conn) return 0; } - sendlen = len - already_sent; + sendlen = len; + if (!peek_offset_cap) + sendlen -= already_sent; + if (sendlen <= 0) { conn_flag(c, conn, STALLED); return 0; @@ -2365,9 +2393,17 @@ static int tcp_data_from_tap(struct ctx *c, struct tcp_tap_conn *conn, flow_trace(conn, "fast re-transmit, ACK: %u, previous sequence: %u", max_ack_seq, conn->seq_to_tap); + + /* Ensure seq_from_tap isn't updated twice after call */ + tcp_l2_data_buf_flush(c);tcp_l2_data_buf_flush() was replaced by tcp_payload_flush() in a recently merged change from Laurent. IIUC, this is necessary because otherwise our update to seq_to_tap canIf we don't flush, we may have a frame there, e.g. seqno 17, followed by a lower numbered frame, e.g. seqno 14. Both will point to a seq_to_tap we just gave the value 14. When the buffer queue is flushed we update seq_to_tap twice, so next sent packet will be 16. This would have worked in the old code, because we calculate the offset value (already_sent) based on the seq_to_tap value, so we just skip ahead one packet and continue transmitting. If we are lucky pkt #15 is already in the receiver's OOF queue, and we are ok.be clobbered from tcp_payload_flush() when we process the queued-but-not-sent frames....how? I don't quite understand the issue here: tcp_payload_flush() updates seq_to_tap once we send the frames, not before, right?It will *not* work in my code, because the kernel offset is advanced linearly, so we will resend a packet called #16, but with the contents of the original pkt #15.So when I say it is a pre-existing bug, I mean that even without your changes it meant that in this situation we could skip re-transmitting part of what we're supposed to retransmit. The consequences are less severe though, because we at least recalculate where we are in the peek buffer based on the messed messed on seq_to_tap value. We don't behave correctly but the receiver will probably be able to sort it out (to them it may not be distinguishable from things that could happen due to packet re-ordering). With Jon's change we wind back SO_PEEK_OFF in step with seq_to_tap at the re-transmit, but when we incorrectly push seq_to_tap back forward, we *don't* update the kernel. So the two are out of sync, hence horrible breakage.Hrm. If that's true then that's another different bug from the one I'm describing.No. Evidently not.This seems like a correct fix, but not an optimal one: we're flushing out data we've already determined we're going to retransmit. Instead, I think we want a different helper that simply discards the queued framesDon't we always send (within the same epoll_wait() cycle) what we queued? What am I missing?Hrm.. an epoll in between what and what, exactly? I can easily see how we get a data_from_sock(), then a data_from_tap() on the same connection during a single epoll cycle, leading to stale queued frames. I suspect there may also be paths where we enter data_from_sock() for the same connection twice in the same epoll cycle. I don't (so far) see any way we could have queued frames persisting across an epoll cycle.> - I'm thinking maybe we actually > want a helper that's called from both the fast and slow retransmit > paths and handles that. > > Ah, wait, we only want to discard queued frames that belong to this > connection, that's trickier. > > It seems to me this is a pre-existing bug, we just managed to get away > with it previously. I think this is at least one cause of the weirdly > jumping forwarding sequence numbers you observed. So I think we want > to make a patch fixing this that goes before the SO_PEEK_OFF changes.This was exactly the reason for my v2: comment in the commit log. But it may even be worse. See below.> > > + > > conn->seq_ack_from_tap = max_ack_seq; > > conn->seq_to_tap = max_ack_seq; > > + set_peek_offset(conn, 0); > > tcp_data_from_sock(c, conn); > > + > > + /* Empty queue before any POLL event tries to send it again */ > > + tcp_l2_data_buf_flush(c); > I'm not clear on what the second flush call is for. The only frames > queued should be those added by the tcp_data_from_sock() just above, > and those should be flushed when we get to tcp_defer_handler() before > we return to the epoll loop.Sadly no. My debugging clearly shows that an epoll() may come in between,and try to transmit a pkt #14 (from the example above), but now with the contents of the original pkt #15. All sorts of weirdities may happen after that. I am wondering if this is a generic problem: Is it possible that two consecutive epolls() may queue up two packets with the same number in the tap queue, whereafter the number will be incremented twice when flushed, and we create a gap in the sequence causing spurious retransmissions? I haven't checked this theory yet, but that is part of my plan for today. Anyway, I don't understand the point with the delayed update of set_to_tap at all. To me it looks plain wrong. But I am sure somebody can explain.This is actually a relatively recent change: it's there so that if we get a low-level error trying to push the frames out to the tap device we don't advance seq_to_tap. In particular this can occur if we overfill the socket send buffer on the tap socket with qemu. It's not technically necessary to do this: we can treat such a failure as packet loss that TCP will eventually deal with. This is an optimization: given that in this case we already know the packets didn't get through we don't want to wait for TCP to signal a retransmit. Instead we avoid advancing seq_to_tap, meaning that we'll carry on from the last point qt which the guest at least might get the data. ...and writing the above, I just realised this is another potential source of desync between the kernel SO_PEEK_OFF pointer and seq_to_tap, although I don't know if it's one you're hitting in practice Jon. Such a low-level transmit failure is essentially an internally triggered re-transmit, so it's another case where we need to wind back SO_PEEK_OFF. To tackle this sanely, I think we have to invert how we're handling the seq_to_tap update. Instead of deferring advancing it until the frames are sent, we should advance it immediately upon queuing. Then in the error path we need to explicitly treat this as a sort of retransmit, where we wind back both seq_to_tap and SO_PEEK_OFF in sync with each other. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson