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 DC1145A026D for ; Thu, 14 Mar 2024 09:48:16 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202312; t=1710406092; bh=/T+K6lsOVDNZi+PgA8FHlKLP4Flj2gyqX0cMLQ3Fidk=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=gQeNPqZTwmw4YmRWFAKHP8g98L32y5Xn4lb19leH35sHWRL8LqP7BKR2Xs6fVQWlf gsgf+uGXpL+8TEfbyBNIyp7D6iWZKAWNbHuWYY0K1R8UO18PVAa+MSmJHEbL1/JYVj 5WNPoYERa+IYlwFNoMSLt2bTkkkupZe7zDfGX3L8cnzKzUghdAPn3E1T8JFZgRRc6n W5zhKmf34UymhCwkwu4PrwXr/G/qoAnXe4GUQl0FeKkB7gqgdEvHulyi1gCRHdz+Z5 cz6+LtYDUhtHXAjH9S5r9B75ikWEKacZOpXZUt80embUH4uJ4mYUeykr4rujaFfBAi 6RLttL5Rv8vsA== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4TwLcr0J7Sz4wqM; Thu, 14 Mar 2024 19:48:12 +1100 (AEDT) Date: Thu, 14 Mar 2024 18:47:59 +1000 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH 1/4] tap: Extend tap_send_frames() to allow multi-buffer frames Message-ID: References: <20240308065325.2181322-1-david@gibson.dropbear.id.au> <20240308065325.2181322-2-david@gibson.dropbear.id.au> <20240314080217.5fa84a1e@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="9886F4zGTJilJm/2" Content-Disposition: inline In-Reply-To: <20240314080217.5fa84a1e@elisabeth> Message-ID-Hash: 7752P4UDQYSZWUHS7YBPDZHMMQAC5WYC X-Message-ID-Hash: 7752P4UDQYSZWUHS7YBPDZHMMQAC5WYC 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, Laurent Vivier 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: --9886F4zGTJilJm/2 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Mar 14, 2024 at 08:02:17AM +0100, Stefano Brivio wrote: > On Fri, 8 Mar 2024 17:53:22 +1100 > David Gibson wrote: >=20 > > tap_send_frames() takes a vector of buffers and requires exactly one fr= ame > > per buffer. We have future plans where we want to have multiple buffers > > per frame in some circumstances, so extend tap_send_frames() to take the > > number of buffers per frame as a parameter. > >=20 > > Signed-off-by: David Gibson > > --- > > tap.c | 83 +++++++++++++++++++++++++++++++++++++---------------------- > > tap.h | 3 ++- > > tcp.c | 8 +++--- > > udp.c | 2 +- > > 4 files changed, 59 insertions(+), 37 deletions(-) > >=20 > > diff --git a/tap.c b/tap.c > > index f4051cec..f9e2a8d9 100644 > > --- a/tap.c > > +++ b/tap.c > > @@ -309,21 +309,28 @@ void tap_icmp6_send(const struct ctx *c, > > =20 > > /** > > * tap_send_frames_pasta() - Send multiple frames to the pasta tap > > - * @c: Execution context > > - * @iov: Array of buffers, each containing one frame > > - * @n: Number of buffers/frames in @iov > > + * @c: Execution context > > + * @iov: Array of buffers > > + * @bufs_per_frame: Number of buffers (iovec entries) per frame > > + * @nframes: Number of frames to send > > * > > + * @iov must have total length @bufs_per_frame * @nframes, with each s= et of > > + * @bufs_per_frame contiguous buffers representing a single frame. >=20 > Oh, this does pretty much what I was suggesting as a comment to > Laurent's "tcp: Replace TCP buffer structure by an iovec array" -- I > should have reviewed this first. Right. > > + *=20 > > * Return: number of frames successfully sent > > * > > * #syscalls:pasta write > > */ > > static size_t tap_send_frames_pasta(const struct ctx *c, > > - const struct iovec *iov, size_t n) > > + const struct iovec *iov, > > + size_t bufs_per_frame, size_t nframes) > > { > > + size_t nbufs =3D bufs_per_frame * nframes; > > size_t i; > > =20 > > - for (i =3D 0; i < n; i++) { > > - ssize_t rc =3D write(c->fd_tap, iov[i].iov_base, iov[i].iov_len); > > + for (i =3D 0; i < nbufs; i +=3D bufs_per_frame) { > > + ssize_t rc =3D writev(c->fd_tap, iov + i, bufs_per_frame); > > + size_t framelen =3D iov_size(iov + i, bufs_per_frame); > > =20 > > if (rc < 0) { > > debug("tap write: %s", strerror(errno)); > > @@ -340,32 +347,37 @@ static size_t tap_send_frames_pasta(const struct = ctx *c, > > default: > > die("Write error on tap device, exiting"); > > } > > - } else if ((size_t)rc < iov[i].iov_len) { > > - debug("short write on tuntap: %zd/%zu", > > - rc, iov[i].iov_len); > > + } else if ((size_t)rc < framelen) { > > + debug("short write on tuntap: %zd/%zu", rc, framelen); > > break; > > } > > } > > =20 > > - return i; > > + return i / bufs_per_frame; > > } > > =20 > > /** > > * tap_send_frames_passt() - Send multiple frames to the passt tap > > - * @c: Execution context > > - * @iov: Array of buffers, each containing one frame > > - * @n: Number of buffers/frames in @iov > > + * @c: Execution context > > + * @iov: Array of buffers, each containing one frame > > + * @bufs_per_frame: Number of buffers (iovec entries) per frame > > + * @nframes: Number of frames to send > > * > > + * @iov must have total length @bufs_per_frame * @nframes, with each s= et of > > + * @bufs_per_frame contiguous buffers representing a single frame. > > + *=20 > > * Return: number of frames successfully sent > > * > > * #syscalls:passt sendmsg > > */ > > static size_t tap_send_frames_passt(const struct ctx *c, > > - const struct iovec *iov, size_t n) > > + const struct iovec *iov, > > + size_t bufs_per_frame, size_t nframes) > > { > > + size_t nbufs =3D bufs_per_frame * nframes; > > struct msghdr mh =3D { > > .msg_iov =3D (void *)iov, > > - .msg_iovlen =3D n, > > + .msg_iovlen =3D nbufs, > > }; > > size_t buf_offset; > > unsigned int i; > > @@ -376,44 +388,53 @@ static size_t tap_send_frames_passt(const struct = ctx *c, > > return 0; > > =20 > > /* Check for any partial frames due to short send */ > > - i =3D iov_skip_bytes(iov, n, sent, &buf_offset); > > + i =3D iov_skip_bytes(iov, nbufs, sent, &buf_offset); > > + > > + if (i < nbufs && (buf_offset || (i % bufs_per_frame))) { > > + /* Number of not-fully-sent buffers in the frame */ >=20 > Strictly speaking, this comment is correct, but "not-fully-sent" seems > to imply that rembufs only counts partially sent buffers. It also > counts the ones that weren't sent at all. What about: >=20 > /* Number of partially sent or not sent buffers for the frame */ >=20 > ? Yeah, the original is technically correct, but easy to misread. Maybe Number of unsent or partially sent buffers for the frame for slightly more brevity than your suggestion? =09 > The rest of the series looks good to me, I can change this text on > merge if you like the proposal, or even apply it as it is (well, it's > correct, after all). Yes, please adjust and go ahead. --=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 --9886F4zGTJilJm/2 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmXyua0ACgkQzQJF27ox 2GeVZw/+P/47rgQL+BhQbw5dFl4S0xcWwO+pGvCV15WWrfG5cLx3IJfRVOS6WICq Sia1oiq2tG07xRQ4i30HoUIR+fh3ggRPlzgz+KQqdJFMiYj/ZDUTyMH5oUhXTUdN izzIM3cl2MJEk41l0HSWez/nzae3naKiZDwUGiwBGU5r9pUPRuJKN0Y6w0Leao9e 0fOqW0OLOhXygRNf9+3peN5NbEBtEJNuQQmlTXN6zXYagon2PJE4+HjJ3PqRlN4F Tg0uwSRiicXgHoYNFvPsDjA66LCEIWIt2SYgU1goq1SXcuq11rVtsw3mRzmAcow6 C2NOnuzQiLdHKhsQnvnJDYDsCHiwHKJCr1JLeZC5I59tMNBvvFfz9seOU0eTfJ1M KD6UvzUAA2VVPtFQL2xU5GalBZNPkWtnW3R1EShu/GYwt54/SjW2UXx/LEdE24QS J950X6s7CkE861SwlQHhdCb2DvV9TmbplD1178N9O1rjcbR13IIjgT9jxvlHCuFm 6IGVtEHMvRy5wZsP6UaRiSpR2dEG3gQiv3zkrA+QJm7AFKgFTijIDQCcq/+b4DWg WqThJBfNaoRigYktfGg94+JFiR6QsyU92mFPpYG2uPOVJCEeyaA7hvgLQcJEMTtU CRDDiMLUZzuzfnScTJ8q/+4zXykQqpS01IFCCYRmaslEMrZ2Ee0= =hX44 -----END PGP SIGNATURE----- --9886F4zGTJilJm/2--