On Thu, Mar 14, 2024 at 08:02:17AM +0100, Stefano Brivio wrote:On Fri, 8 Mar 2024 17:53:22 +1100 David Gibson <david(a)gibson.dropbear.id.au> wrote:Right.tap_send_frames() takes a vector of buffers and requires exactly one frame 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. Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au> --- tap.c | 83 +++++++++++++++++++++++++++++++++++++---------------------- tap.h | 3 ++- tcp.c | 8 +++--- udp.c | 2 +- 4 files changed, 59 insertions(+), 37 deletions(-) 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, /** * 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 set of + * @bufs_per_frame contiguous buffers representing a single frame.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.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?+ * * 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 = bufs_per_frame * nframes; size_t i; - for (i = 0; i < n; i++) { - ssize_t rc = write(c->fd_tap, iov[i].iov_base, iov[i].iov_len); + for (i = 0; i < nbufs; i += bufs_per_frame) { + ssize_t rc = writev(c->fd_tap, iov + i, bufs_per_frame); + size_t framelen = iov_size(iov + i, bufs_per_frame); 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; } } - return i; + return i / bufs_per_frame; } /** * 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 set of + * @bufs_per_frame contiguous buffers representing a single frame. + * * 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 = bufs_per_frame * nframes; struct msghdr mh = { .msg_iov = (void *)iov, - .msg_iovlen = n, + .msg_iovlen = 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; /* Check for any partial frames due to short send */ - i = iov_skip_bytes(iov, n, sent, &buf_offset); + i = 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 */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: /* Number of partially sent or not sent buffers for the frame */ ?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. -- 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