From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id 861FD5A004C for ; Fri, 31 May 2024 03:42:40 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202312; t=1717119756; bh=YsSDpdnaHIhI67+tRKV232KZSp7FQOTQfmZ81r3ROGQ=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=b4KLGz9jfAANvXDzFLsVPmIB4nO4euUEOTm54ffosc5TAig4B2KgtGN5s6DZ0uvKK RKf1WUkjleUg4/vBThcxbyF2XM3bLgq1/acfH/JRL6/628uYGC4FDmYkU6c97upIQ9 75m6uu9Z6t/o6kZiNdJJlCH64x66HoCfwBcTI8NZ3z4yV598w2MLwwPuGNgQdAAqEY czW7b5NV4jur9HanaCwOsHVzlqDzYykfWWy1RoRu914/IvQLXWgbB+zKSb+RE2BRGA mQjB9XxJptXMSDP+6xCEjBLmL0Ew+JNbLjB8B4DvqXOeGXqEAgjVZaktTJAXYbkOCF hnXi+LXhNy5yQ== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4Vr5Tm4W9Dz4x7l; Fri, 31 May 2024 11:42:36 +1000 (AEST) Date: Fri, 31 May 2024 11:42:28 +1000 From: David Gibson To: Jon Maloy Subject: Re: [PATCH v7 1/3] tcp: move seq_to_tap update to when frame is queued Message-ID: References: <20240524172656.193183-1-jmaloy@redhat.com> <20240524172656.193183-2-jmaloy@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="OlvD0B0BOhFw8fxZ" Content-Disposition: inline In-Reply-To: <20240524172656.193183-2-jmaloy@redhat.com> Message-ID-Hash: 7YXL53YKP2BMC5AGDCCRSWQ4H3HY42I2 X-Message-ID-Hash: 7YXL53YKP2BMC5AGDCCRSWQ4H3HY42I2 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: passt-dev@passt.top, sbrivio@redhat.com, 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: --OlvD0B0BOhFw8fxZ Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, May 24, 2024 at 01:26:54PM -0400, Jon Maloy wrote: > commit a469fc393fa1 ("tcp, tap: Don't increase tap-side sequence counter = for dropped frames") > delayed update of conn->seq_to_tap until the moment the corresponding > frame has been successfully pushed out. This has the advantage that we > immediately can make a new attempt to transmit a frame after a failed > trasnmit, rather than waiting for the peer to later discover a gap and > trigger the fast retransmit mechanism to solve the problem. >=20 > This approach has turned out to cause a problem with spurious sequence > number updates during peer-initiated retransmits, and we have realized > it may not be the best way to solve the above issue. >=20 > We now restore the previous method, by updating the said field at the > moment a frame is added to the outqueue. To retain the advantage of > having a quick re-attempt based on local failure detection, we now scan > through the part of the outqueue that had do be dropped, and restore the > sequence counter for each affected connection to the most appropriate > value. >=20 > Signed-off-by: Jon Maloy This still has the issues I pointed out on the last revision... [snip] > +/** > + * tcp_revert_seq() - Revert affected conn->seq_to_tap after failed tran= smission > + * @conns: Array of connection pointers corresponding to queued fr= ames > + * @frames: Two-dimensional array containing queued frames with sub= -iovs > + * @num_frames: Number of entries in the two arrays to be compared > + */ > +static void tcp_revert_seq(struct tcp_tap_conn **conns, struct iovec (*f= rames)[TCP_NUM_IOVS], > + int num_frames) > +{ > + int i; > + > + for (i =3D 0; i < num_frames; i++) { > + struct tcp_tap_conn *conn =3D conns[i]; > + struct tcphdr *th =3D frames[i][TCP_IOV_PAYLOAD].iov_base; > + uint32_t seq =3D ntohl(th->seq); > + > + if (SEQ_LE(conn->seq_to_tap, seq)) > + continue; > + > + conn->seq_to_tap =3D seq; =2E..one trivial - this would be clearer without the continue - ... > + } > +} > + > /** > * tcp_payload_flush() - Send out buffers for segments with data > * @c: Execution context > */ > static void tcp_payload_flush(const struct ctx *c) > { > - unsigned i; > size_t m; > =20 > m =3D tap_send_frames(c, &tcp6_l2_iov[0][0], TCP_NUM_IOVS, > tcp6_payload_used); > - for (i =3D 0; i < m; i++) > - *tcp6_seq_update[i].seq +=3D tcp6_seq_update[i].len; > + if (m !=3D tcp6_payload_used) { > + tcp_revert_seq(tcp6_frame_conns, &tcp6_l2_iov[m], > + tcp6_payload_used - m); =2E. and one fatal - you're calling this with non-matching entries from frame_conns[] and l2_iov[]. --=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 --OlvD0B0BOhFw8fxZ Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmZZKuoACgkQzQJF27ox 2GfIBA/8Cka6rGVnQCCsDBxIKl61mf7LCVd05vue1nmPvYCWUP9DAicIVBcGuXYh 7GJJSfQtRlTxoqj/Ov40zqnUL++DRcKkPpXyNgTlwb9p0hUdTQ9RtVhvzVKcnUm+ 1oH+ItiyDOmEI66JyPJ+tAETQk8wqgmr3ScdmxwOIM/tzlnwWfTll8j5Int/u68s QuFgEEqLp05Dy85dzxhALyBS3VbFwXMYEwyjBkkWRqypCU6zlJPfGQTkG57kafyl LZx+dIr/WvMYfpXHterawGwHt2+uK6ZYTbABrRnrFowrXIiki1GqW8Q0icQK2/b4 a5NQm5zMMvFzxt7Pf68AJCqdO9mnj9PEcrAdNfMItbqcCH1rDi0RpZRBPGxO1oFo WeozJuPM3JEN/h20GdYHEw6+Ck7xW7TjXXctgYhpOjQ6l+ey9vz5kf9A0vfaHlIq oQqEzsFn0cTigBG2zWDKPx282nbwMDoRgvHN1EatrkvJv041nYVAYRsK7Ab1AQzL pDJ1KablFew+mkfb7ciU//lHdD8apThY/eQn0TCvJTm4NsO6tYjQqAu5T3Pph8BS xUK/nFBbGc8/BunG4dHlZVSbYCY/BwKaOwFokB09ocrkblhvFKMA68uZDvzAbVS3 sKu1mENqPSitFQ+FN/bg8r8UvSX0a23N+ADUyPHXQqYoaSpjbNs= =o5nU -----END PGP SIGNATURE----- --OlvD0B0BOhFw8fxZ--