From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from gandalf.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id 095AC5A0271 for ; Tue, 3 Oct 2023 05:23:12 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=201602; t=1696303389; bh=SZy5qvvea+C4ZQaWNo+vA6ryvSzGvjO9Xk0j15G1gHM=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=TRqWeeXSOc6EPIGcH8n5m5X1p3XuY9r+58ik0zNtui95rq4Mls6fItWONuz4Rfeu2 e2olbKZPFDCVk/LO2CLxAxS9VmNnZw33616+hU/vhjwmkWTa3XM80A1MybW8FZEsOD nIqYerf/MxQu1GMA62MKKmDE0ybQtJssucHG7vxY= Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4S03716g7lz4xPf; Tue, 3 Oct 2023 14:23:09 +1100 (AEDT) Date: Tue, 3 Oct 2023 14:22:59 +1100 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH RFT 4/5] tcp, tap: Don't increase tap-side sequence counter for dropped frames Message-ID: References: <20230922220610.58767-1-sbrivio@redhat.com> <20230922220610.58767-5-sbrivio@redhat.com> <20230927190603.10a1ed74@elisabeth> <20230929171950.5086d408@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="wnKwMCtPSa0IWFnz" Content-Disposition: inline In-Reply-To: <20230929171950.5086d408@elisabeth> Message-ID-Hash: XIJNZE4N7B2CAY4SWGDECM37GTU7I34H X-Message-ID-Hash: XIJNZE4N7B2CAY4SWGDECM37GTU7I34H 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: Matej Hrica , passt-dev@passt.top 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: --wnKwMCtPSa0IWFnz Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Sep 29, 2023 at 05:19:50PM +0200, Stefano Brivio wrote: > On Thu, 28 Sep 2023 11:58:45 +1000 > David Gibson wrote: >=20 > > On Wed, Sep 27, 2023 at 07:06:03PM +0200, Stefano Brivio wrote: > > > On Mon, 25 Sep 2023 14:47:52 +1000 > > > David Gibson wrote: > > > =20 > > > > On Sat, Sep 23, 2023 at 12:06:09AM +0200, Stefano Brivio wrote: =20 > > > > > ...so that we'll retry sending them, instead of more-or-less sile= ntly > > > > > dropping them. This happens quite frequently if our sending buffe= r on > > > > > the UNIX domain socket is heavily constrained (for instance, by t= he > > > > > 208 KiB default memory limit). > > > > >=20 > > > > > It might be argued that dropping frames is part of the expected T= CP > > > > > flow: we don't dequeue those from the socket anyway, so we'll > > > > > eventually retransmit them. > > > > >=20 > > > > > But we don't need the receiver to tell us (by the way of duplicat= e or > > > > > missing ACKs) that we couldn't send them: we already know as > > > > > sendmsg() reports that. This seems to considerably increase > > > > > throughput stability and throughput itself for TCP connections wi= th > > > > > default wmem_max values. > > > > >=20 > > > > > Unfortunately, the 16 bits left as padding in the frame descripto= rs =20 > > > >=20 > > > > I assume you're referring to the 'pad' fields in tcp[46]_l2_buf_t, > > > > yes? =20 > > >=20 > > > Right, that. > > > =20 > > > > For AVX2 we have substantially more space here. Couldn't we put > > > > a conn (or seq) pointer in here at the cost of a few bytes MSS for > > > > non-AVX2 and zero cost for AVX2 (which is probably the majority cas= e)? =20 > > >=20 > > > Yes, true. On the other hand, having this parallel array only affects > > > readability I guess, whereas inserting pointers and lengths in > > > tcp[46]_l2_buf_t actually decreases the usable MSS (not just on > > > non-AVX2 x86, but also on other architectures). So I'd rather stick to > > > this. =20 > >=20 > > Yeah, I guess so. > >=20 > > Actually.. I did just think of one other option. It avoids both any > > extra padding and a parallel array, but at the cost of additional work > > when frames are dropped. We could use that 16-bits of padding to > > store the TCP payload length. Then when we don't manage to send all > > our frames, we do another loop through and add up how many stream > > bytes we actually sent to update the seq pointer. >=20 > Hmm, yes. It's slightly more memory efficient, but the complexity seems > a bit overkill to me. More importantly, I forgot the fact that by the time we're sending the frames, we don't know what connection they're associated with any more. [snip] > > > > > @@ -2282,14 +2310,15 @@ static int tcp_data_from_sock(struct ctx = *c, struct tcp_tap_conn *conn) > > > > > =20 > > > > > /* Finally, queue to tap */ > > > > > plen =3D mss; > > > > > + seq =3D conn->seq_to_tap; =20 > > > >=20 > > > > This will only be correct if tcp_l2_data_buf_flush() is *always* > > > > called between tcp_data_from_sock() calls for the same socket. That > > > > should be true for the normal course of things. However, couldn't = it > > > > happen that we get a normal socket EPOLLIN event for a particular > > > > connection - calling tcp_data_from_sock() - but in the same epoll() > > > > round we also get a tap ack for the same connection which causes > > > > another call to tcp_data_from_sock() (with the change from patch > > > > 2/5). IIRC those would both happen before the deferred handling and > > > > therefore the data_buf_flush(). =20 > > >=20 > > > Ah, yes, I actually wrote this before 2/5 and concluded it was okay :/ > > > but with that change, it's not. Unless we drop that change from 2/5. = =20 > >=20 > > Even if we drop the change, it's a worryingly subtle constraint. >=20 > Another option to avoid this... >=20 > > > > Not sure how to deal with that short of separate 'seq_queued' and > > > > 'seq_sent' counters in the connection structure, which is a bit > > > > unfortunate. =20 > > >=20 > > > I wonder how bad it is if we call tcp_l2_data_buf_flush() > > > unconditionally before calling tcp_data_from_sock() from > > > tcp_tap_handler(). But again, maybe this is not needed at all, we > > > should check that epoll detail from 2/5 first... >=20 > other than this one, would be to use that external table to update > sequence numbers *in the frames* as we send stuff out. Not really sure what you're proposing there. --=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 --wnKwMCtPSa0IWFnz Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmUbiQ0ACgkQzQJF27ox 2Gca8Q/9HIJ8JEafGr7xz+VqNwEd77apnlIn5AYlmG0GfqIXiZwd+xj3P/OgTnVu ROYO+nnC9dQp8WLyy3bgrKSedprAuiAB2dl0k+xUPXBxqdOshvq1uAMjHkWVHIYw WpxcnAQ1riFo38A6g3Zce9u170bQYVUhC1r3tFBItlqzJfER9chjbiyGwOXBTtxD /FlVSLDgtPE4L0waXBNEVBjnbTQSxUo5pf3q3s3I+R80u34MREv7Q4738GdkuNv/ NNOeh+9w1azhh7b8Mn7alFZwgaCdyQmZApC1MPrwmynkj18oTXHdCJh9DFWOYWgV tHdipSM2aZDzuOBGd8zjZ0wO0uvYMkXFoSCQ3j8kQjC5M+v7lLQ3ELq20w8XH8HV tqcn97b6L24yRaOSMTxdqW1JA/kIr35ff4F4/9vt8plz3Xmv72tWnWbEaDJ42Zk/ gTeZl9bZ3IBoDnqx5NhpKVKbCvJ5EYfwPvTrVGRHKuvgZtZc4c1SYMBF09t4od1A K0gQsmIcKEPDSAAp9iZ/kPIy5tO1OIgpj8RYkCmqqXpfeF8bUxG1bgNVau3Zpsdv oXK7wEbFhAeGJKqfyOyJ2xLqe4M3ofEz7sg9JlcFVBlSfm8WbLgd3nC3gOvgDtF3 gZGp0tOIX7u9KuT1QBRkKhXJNF1V8b5PVutfXLY67cRgeeIOqe0= =OKoz -----END PGP SIGNATURE----- --wnKwMCtPSa0IWFnz--