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 7EFD05A005E for ; Tue, 14 Feb 2023 00:42:26 +0100 (CET) Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4PG18s0WD3z4x5W; Tue, 14 Feb 2023 10:42:21 +1100 (AEDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=201602; t=1676331741; bh=nbyQbqUbtK5+R8izk/3J9eF14JuaMfhJHjpa6jzNmpI=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=AjDiP7PmiaWrEoMiLdnXzXxRqbHmfEJS9KI8hqTrS8ZkX3Bn5UNROtRgGq5DhJqe4 bCLzslBSZkjq3P9Oy0bR70oZZA5IbFwU3pjuWlcdtchMzzxJihQU+KHE823u/+6JIk yJqleKSKai5vhq5IgAJm+Fe4Dxv9PJFbTsbzBXrU= Date: Tue, 14 Feb 2023 10:39:23 +1100 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH] tap: Send frames after the first one in tap_send_frames_pasta() Message-ID: References: <20230213011211.1198729-1-sbrivio@redhat.com> <20230213114609.0f88cc31@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="FwQkpAcL5EPbZysg" Content-Disposition: inline In-Reply-To: <20230213114609.0f88cc31@elisabeth> Message-ID-Hash: NR3SD3TMXXAD33LBOLQNERFD526XOBJV X-Message-ID-Hash: NR3SD3TMXXAD33LBOLQNERFD526XOBJV 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 X-Mailman-Version: 3.3.3 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: --FwQkpAcL5EPbZysg Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Feb 13, 2023 at 11:46:09AM +0100, Stefano Brivio wrote: > On Mon, 13 Feb 2023 13:24:58 +1100 > David Gibson wrote: >=20 > > On Mon, Feb 13, 2023 at 02:12:11AM +0100, Stefano Brivio wrote: > > > ...instead of repeatedly sending out the first one in iov. > > >=20 > > > Fixes: e21ee41ac35a ("tcp: Combine two parts of pasta tap send path t= ogether") > > > Signed-off-by: Stefano Brivio > > > --- > > > I just applied this, to unblock a series by David which was pending > > > for way too long. The commit reference in Fixes: refers to a commit > > > from said series which I'm pushing out together with this patch. =20 > >=20 > > Huh... how did this ever work even slightly. From that point of view, > >=20 > > Reviewed-by: David Gibson > >=20 > > > Posting anyway for reviews. =20 > >=20 > > That said.. > >=20 > > >=20 > > > tap.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > >=20 > > > diff --git a/tap.c b/tap.c > > > index af9bc15..716d887 100644 > > > --- a/tap.c > > > +++ b/tap.c > > > @@ -316,12 +316,13 @@ static void tap_send_frames_pasta(struct ctx *c, > > > { > > > size_t i; > > > =20 > > > - for (i =3D 0; i < n; i++) { > > > + for (i =3D 0; i < n; i++, iov++) { =20 > >=20 > > I quite dislike having multiple "counters" that need to be updated for > > each loop iteration (manual strength reduction. It's really easy to > > make a mistake in later changes and let the two values get out of sync > > - which is exactly what I did with the earlier change that introduced > > this bug. >=20 > Um, yes. I try, whenever possible, to use just one "iterator", which > would be iov, but the price of doing that "cleanly" here is wasting a > struct iovec just to have a zero iov_len at the end, which makes little > sense. Right.. I mean it's nice when you can use the pointer/object itself as the iterator. But in C, its pretty common for that to get awkward, so I was conciously switching these from the iterator being 'iov' to the iterator being 'i'. > > W.r.t. performance, I generally trust the compiler's automatic > > strength reduction to have a better idea of whether it will be worth > > it or not than my own guess. > >=20 > > > if (write(c->fd_tap, (char *)iov->iov_base, iov->iov_len) < 0) { = =20 > >=20 > > So, my *intention* on the older patch was to replace 'iov->' above > > with 'iov[i].' >=20 > That would also be consistent with tap_send_frames_passt(), so sure, > let's change it. I can submit a patch too. >=20 --=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 --FwQkpAcL5EPbZysg Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmPqyhYACgkQzQJF27ox 2GfB1w//SLIT3repg2NdI08g7GrVkbKkItcATwySKGEYuWAEQjHOKt83hNq2mB6M mxQr8i5yr+tTmjz5q5K+hfDXUxdz5nru+g0qGF/W/P4g5MNs/IVFlk+trwEmQg2J awP0xbQX5T21qKDSo/3o5cKtklQyYry79zP6sDLrOVDW87ioNoC6Ur2NATMvWgOv xWkDpmIJpBV7uFPH1Wxd2oMNbZJd8xic4jYWpRpPPB+SJwsk03BPR/gJMllHf7nj 3KMxzz4VOxkh8ejEZ00TUoUbl55wp5yx9703eETtHDxwCV3uW8errlXBm3cYv5ll q1SwCP8k81C1EAbtWRZFMV8LlWyTcXf2yYrM/8NXFh1294F7IFxpDT/nhwuq8HbP CaUimxCj9X5tgBUWHzHMFb41JPZDtpLd4DGoE3/yMhMd3kHmcodHcg/6pg97da0C Jkla7vWr6dFpiZFAVY3bRgcNDxs79gASGTV8EUe+n3FqsjO2/aNhjVG0/Wy8sedK Bw9UnO+Sx77mGDg5pMNVkYjERefL4QQdyDwQsEr1A1CAfXhpWEleqjQIbi4lXw/n 7ClFoYGHkeWdcOoDerYKlHxYQf0quxo4eFEzGRd/q7wuzxfgnTJM/nvVdvdO80Nc 6MMqFoXP6v3GhaqMaYWgmPmICMYuF5gI4/urvgFkNhsx1kN8O8M= =VOEh -----END PGP SIGNATURE----- --FwQkpAcL5EPbZysg--