From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id 890815A9D0A for ; Mon, 6 May 2024 09:15:22 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202312; t=1714979718; bh=MefJFpUnUFN/VCBIju/2M2S5hJyEVEoHSolHgMaMRfs=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=IF0A0BDcfjU7/cz5XFl4XruoAuYFr4j6aApIQeWyp0Hb91Dndfu1NZcMAbUGprJ8z 1kqpXssvbcF4Ud6Cy5u/06/qIpQg86dURQe1Kk9hjFW5kIuTWSKQQLwOHvN3awT5e4 spjstE7mJvmM6Upeg4PVVi7LmdKXDnUByc/dT5kMA4deTBZPMZfSPNK8CMikzpSLIk D5gTzz/EW/vk53M6A+miivnZ96cgJzNMbPRSDGxAWT1Kb0kRCLQohSBp6aApFH5cvw WxNBWDENLzZyhTKwT06h0htLxo7BSdFrSaKIEO1+/gKNFiK66EVfNbq5WysC6MxRvt qqNkhQMzs/s+w== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4VXt3B33Mrz4wnv; Mon, 6 May 2024 17:15:18 +1000 (AEST) Date: Mon, 6 May 2024 17:15:13 +1000 From: David Gibson To: Jon Maloy Subject: Re: [PATCH v2 1/2] tcp: leverage support of SO_PEEK_OFF socket option when available Message-ID: References: <20240501202839.3491885-1-jmaloy@redhat.com> <20240501202839.3491885-2-jmaloy@redhat.com> <20240503154255.3d062430@elisabeth> <767934ce-269a-cc9e-0cf3-1cb062103802@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="ICXIqdTY46u0geze" Content-Disposition: inline In-Reply-To: <767934ce-269a-cc9e-0cf3-1cb062103802@redhat.com> Message-ID-Hash: VN3DVRDXNZLRWHWTJDARP4TVCHKE55IJ X-Message-ID-Hash: VN3DVRDXNZLRWHWTJDARP4TVCHKE55IJ X-MailFrom: dgibson@gandalf.ozlabs.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: Stefano Brivio , passt-dev@passt.top, lvivier@redhat.com, dgibson@redhat.com X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: --ICXIqdTY46u0geze Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, May 03, 2024 at 10:43:52AM -0400, Jon Maloy wrote: >=20 >=20 > On 2024-05-03 09:42, Stefano Brivio wrote: > > On Thu, 2 May 2024 11:31:52 +1000 > > David Gibson wrote: [snip] > > > > /* Receive into buffers, don't dequeue until acknowledged by gue= st. */ > > > > do > > > > len =3D 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 =3D len - already_sent; > > > > + sendlen =3D len; > > > > + if (!peek_offset_cap) > > > > + sendlen -=3D already_sent; > > > > + > > > > if (sendlen <=3D 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. > > >=20 > > > IIUC, this is necessary because otherwise our update to seq_to_tap can > > ...but Jon's comment refers to seq_from_tap (not seq_to_tap)? I'm > > confused. > Right. It should be seq_to_tap. > > > 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? > If 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. 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. > 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. > > > 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 frames > > Don't we always send (within the same epoll_wait() cycle) what we > > queued? What am I missing? > No. Evidently not. Hrm. If that's true then that's another different bug from the one I'm describing. > > > - I'm thinking maybe we actually > > > want a helper that's called from both the fast and slow retransmit > > > paths and handles that. > > >=20 > > > Ah, wait, we only want to discard queued frames that belong to this > > > connection, that's trickier. > > >=20 > > > 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. > > >=20 > > > > + > > > > conn->seq_ack_from_tap =3D max_ack_seq; > > > > conn->seq_to_tap =3D 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, 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. > 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. >=20 > 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. >=20 > 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. =2E..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. --=20 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 --ICXIqdTY46u0geze Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmY4g4AACgkQzQJF27ox 2GdFwg/+ORCxIQmLLXi7+jCIk1icqwqThHa7LVhuzlUp4MPuDSBp8mW++Yu9nJQa jfMGcS6beUYZkUfwaC7eUqtZ9aOjGpv6zXBLG0YyoHgvDJWHuxYJLJQp6fUZOvqK QxN+016Htwz4cWJeupmSvzl0uYYbGzm961MEPK1D2mpomeciYyYDP1LVcgef0Ru/ F5uLJ3olc0Z8uuZDYOmInvyKoJmTFzK+UATitjcKwCYBK8k0Ts1qddahLapAua+g lYp/2CPV2Nc6xyNRtfbcF8AA5y+PV1SCeqAFVcls7HZoCxZaEYgty1/uxDiTtvlQ ckl+Oy0EhNls37/zNtZ6k5xuaDehwHIXucEG8W734BrLNX+2Y2vg5Jw5PjprnLlA B7wOwSc+NjLWB374d3CqKUS53hGqSTFMetSOroKi1i5soANZm05ET991qvmjim4n C9ldSw7XmJiPg3r25oH+aMzYX5zLRq5RkBqmg8tQcquKBdglGXzH+wtQCnrrrfK/ BHrpahCM43Jp6t+G05CZZFPmXo41cYc3Gi+T5QFAc+YUgd/8/SwI87jdAHPP3Knj 3dcf41ujOEOvjlsjE5UE1DXVYUsh2+aAzp1APJLG8QC18nHPivnfqs+wDjghhFpK Bxu7X4CB3ieyE3TqyXivNMQh/O5Ji/OA19Qxe3uSAg3IG1hYVfY= =oywe -----END PGP SIGNATURE----- --ICXIqdTY46u0geze--