From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from gandalf.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id B2B3E5A026F for ; Thu, 5 Oct 2023 10:15:18 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=201602; t=1696493715; bh=E2yRzg/8e7228oigNnfXqJNctqUaXgTRaRqotZF9w+w=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=LeXb2Ye2uJBDTUcOW7yMcDZs1bM3Mb/JPwIy7Lwko4mbOx2xO49hHAQ0MpeM5pcZG ++U1qVlXxE8jho3lxWbU2k6OTLgTKwTSSsbR3X2RMc7QVh118kc4w1CDilzhXpiWi4 BpDUUV/NY52XEOBaZyNRCCOflZv1m5ITRvQVjTTk= Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4S1PW71fcYz4xKl; Thu, 5 Oct 2023 19:15:15 +1100 (AEDT) Date: Thu, 5 Oct 2023 18:38:39 +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> <20231005081900.01f7431a@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="5TR6rNXnzEvhF7SP" Content-Disposition: inline In-Reply-To: <20231005081900.01f7431a@elisabeth> Message-ID-Hash: KNCCCO2WWLHFHOOBPF4PNYT7VTTXB6UF X-Message-ID-Hash: KNCCCO2WWLHFHOOBPF4PNYT7VTTXB6UF 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: --5TR6rNXnzEvhF7SP Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Oct 05, 2023 at 08:19:00AM +0200, Stefano Brivio wrote: > On Tue, 3 Oct 2023 14:22:59 +1100 > David Gibson wrote: >=20 > > 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: =20 > > > > > 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 = silently > > > > > > > dropping them. This happens quite frequently if our sending b= uffer on > > > > > > > the UNIX domain socket is heavily constrained (for instance, = by the > > > > > > > 208 KiB default memory limit). > > > > > > >=20 > > > > > > > It might be argued that dropping frames is part of the expect= ed TCP > > > > > > > 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 dupl= icate 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 connection= s with > > > > > > > default wmem_max values. > > > > > > >=20 > > > > > > > Unfortunately, the 16 bits left as padding in the frame descr= iptors =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= case)? =20 > > > > >=20 > > > > > Yes, true. On the other hand, having this parallel array only aff= ects > > > > > 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 sti= ck 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 w= ork > > > > 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 > > >=20 > > > Hmm, yes. It's slightly more memory efficient, but the complexity see= ms > > > a bit overkill to me. =20 > >=20 > > 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. >=20 > Oh, I thought you wanted to rebuild the information about the > connection by looking into the hash table or something like that. No, I guess we could do that, but I think it would be substantially messier than the parallel array approach. > > [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, could= n't it > > > > > > happen that we get a normal socket EPOLLIN event for a particul= ar > > > > > > connection - calling tcp_data_from_sock() - but in the same epo= ll() > > > > > > 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 handlin= g and > > > > > > therefore the data_buf_flush(). =20 > > > > >=20 > > > > > Ah, yes, I actually wrote this before 2/5 and concluded it was ok= ay :/ > > > > > 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 > > >=20 > > > Another option to avoid this... > > > =20 > > > > > > Not sure how to deal with that short of separate 'seq_queued' a= nd > > > > > > '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 > > >=20 > > > other than this one, would be to use that external table to update > > > sequence numbers *in the frames* as we send stuff out. =20 > >=20 > > Not really sure what you're proposing there. >=20 > That tcp_l2_buf_fill_headers() calculates the sequence from > conn->seq_to_tap plus a cumulative count from that table, instead of > passing it from the caller. Ah, right, that could work. --=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 --5TR6rNXnzEvhF7SP Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmUeZ/gACgkQzQJF27ox 2Gf5DhAAk8qhilJq6hwS+ovfHfoj4giZ6QqUJWzUMZV1gS8ANMVFc+NvSld1K95k nva2elQ452vSndJ3AMyis6uhejTigIL1JnCZ4hw2t9yyvFAmcKn9dgak4ZYM5Upx kuGIYbHyWvNqUAAdirATfKRkgOH8h7H5ObakIuvIC0AOpMjPc6krckzkZPcLRyVM aqVfMz3ybBh26EfGzoibt8+ebnElo1zciTVnwsOpIV+PbJPt6wZFi5v1n/vHMKww 9mFhn9aycVrCnp9P14uSO7/UKwMjppfsqnKf/WNX0Xvd4iGbINq1oneRN6qv30TO 6NosfT5ldd1UfMzEnhYJvPbqiXuLFdLXh0tjzeP4d9YWrk8Ji2/j9hCNLk0Q5gqY BjZCtu/hAHDumchTgOh/YsPrH4pyNv4uJNSoozaLX52NqAeKOx+iny0aJZ1t3hXt R8ToD0lDlk2OjqIDQ/cstBqnozqcTWiFsjV/XL73AjkL94eeW818olpW1APklLcA 5v8sELXl7Rw6pGFIYBET46IvwYdyiHgVlLaEyHBUE3kMtnd0YueghF49DcAlvboC HqSdcRgD0Km73kInowke92TZXruSC1d5orKeSGzltOZ1QyhY1iK2RqYR8c1X95TW ZGUXADjaEJ/oOdUJFtaZzgA2xsYCjL5t+Z/N8YxLzKoznnt7Z8Y= =vibF -----END PGP SIGNATURE----- --5TR6rNXnzEvhF7SP--