On Thu, 22 Feb 2024 16:55:57 +1100 David Gibson wrote:

tap_send_frames_passt() needs to determine which buffer element a byte offset into an IO vector lies in. We have some upcoming uses for similar logic, so split this out into a helper function iov_offset().

Signed-off-by: David Gibson --- tap.c | 13 +++++-------- util.c | 23 +++++++++++++++++++++++ util.h | 1 +

Laurent, I guess this will need to be moved to iov.h by your series, at the point where you introduce the new header.

3 files changed, 29 insertions(+), 8 deletions(-)

diff --git a/tap.c b/tap.c index 396dee7e..f15eba6e 100644 --- a/tap.c +++ b/tap.c @@ -390,22 +390,19 @@ static size_t tap_send_frames_passt(const struct ctx *c, .msg_iovlen = n, }; unsigned int i; + size_t offset; ssize_t sent;

sent = sendmsg(c->fd_tap, &mh, MSG_NOSIGNAL | MSG_DONTWAIT); if (sent < 0) return 0;

- /* Check for any partial frames due to short send */

Why would you drop this comment, though? I feel it's needed even more as we don't open-code that any longer.

- for (i = 0; i < n; i++) { - if ((size_t)sent < iov[i].iov_len) - break; - sent -= iov[i].iov_len; - } + offset = (size_t)sent; + i = iov_offset(iov, n, &offset);

I think with these names and interface this becomes quite obscure: it sounds like 'i' should be an offset at this point... and 'offset', I have no idea (unless I read the comment to iov_offset()). Slightly different proposal below.

- if (i < n && sent) { + if (i < n && offset) { /* A partial frame was sent */ - tap_send_remainder(c, &iov[i], sent); + tap_send_remainder(c, &iov[i], offset); i++; }

diff --git a/util.c b/util.c index 21b35ff9..fb6a0430 100644 --- a/util.c +++ b/util.c @@ -574,3 +574,26 @@ int do_clone(int (*fn)(void *), char *stack_area, size_t stack_size, int flags, return clone(fn, stack_area + stack_size / 2, flags, arg); #endif } + +/* iov_offset() - interpret offset into an IO vector + * @iov: IO vector + * @n: Number of entries in @iov + * @offset: Pointer to offset into IO vector + * + * Return: index I of iovec which contains the given offset, or @n if + * the given offset is >= the total # of bytes in the vector. + * *@offset is updated to be the byte offset into (@iov + I), + * and is guaranteed to be less than @iov[I].iov_len + */ +size_t iov_offset(const struct iovec *iov, size_t n, size_t *offset)

...what do you think of: /* iov_entry_index() - Index and optionally entry offset, given global offset * @iov: IO vector * @n: Number of entries in @iov * @global_offset: Global offset of byte in IO vector we're looking for * @entry_offset: If not NULL, set on return: entry offset * * Return: index of IO vector entry for given byte offset, @n if not found * * Note: @entry_offset is guaranteed to be less than @iov[i].iov_len, where i is * the return value */ and tap_send_frames_passt() could (more) happily do: i = iov_entry_index(iov, n, sent, &entry_offset); if (i < n) { /* A partial frame was sent */ tap_send_remainder(c, &iov[i], entry_offset); i++; }

+{ + size_t i; + + for (i = 0; i < n; i++) { + if (*offset < iov[i].iov_len)

Indentation.

+ break; + *offset -= iov[i].iov_len; + } + + return i; +} diff --git a/util.h b/util.h index d2320f8c..62fad6fe 100644 --- a/util.h +++ b/util.h @@ -229,6 +229,7 @@ void write_pidfile(int fd, pid_t pid); int __daemon(int pidfile_fd, int devnull_fd); int fls(unsigned long x); int write_file(const char *path, const char *buf); +size_t iov_offset(const struct iovec *iov, size_t n, size_t *offset);

/** * mod_sub() - Modular arithmetic subtraction

-- Stefano

On Tue, Feb 27, 2024 at 03:25:51PM +0100, Stefano Brivio wrote:

On Thu, 22 Feb 2024 16:55:59 +1100 David Gibson wrote:

...

We have several places where we want to write(2) a buffer or buffers and we do (or should) handle sort write()s by retrying until everything is

After making sure that "handle sorting" doesn't exist (yet): is one between "handle" and "sort" redundant, or is there another meaning to this?

Oops. s/sort/short/ is what makes this make sense.

...

successfully written. Add a helper for this in util.c.

This version has some differences from the typical write_all() function. First, take an IO vector rather than a single buffer, because that will be useful for some of our cases. Second, allow it to take an parameter to skip the first n bytes of the given buffers. This will be usefl for some of the cases we want, and also falls out quite naturally from the implementation.

Signed-off-by: David Gibson --- util.c | 32 ++++++++++++++++++++++++++++++++ util.h | 1 + 2 files changed, 33 insertions(+)

diff --git a/util.c b/util.c index fb6a0430..a475f2b5 100644 --- a/util.c +++ b/util.c @@ -19,6 +19,7 @@ #include #include #include +#include #include #include #include @@ -597,3 +598,34 @@ size_t iov_offset(const struct iovec *iov, size_t n, size_t *offset)

return i; } + +/* write_remainder() - write the tail of an IO vector to an fd + * @fd: File descriptor + * @iov: IO vector + * @iovcnt: Number of entries in @iov + * @skip: Number of bytes of the vector to skip writing + * + * Return: 0 on success, -1 on error (with errno set) + * + * #syscalls write writev + */ +int write_remainder(int fd, const struct iovec *iov, int iovcnt, size_t skip) +{ + int i; + + while ((i = iov_offset(iov, iovcnt, &skip)) < iovcnt) {

With my proposal for 1/6 this becomes:

while ((i = iov_entry_index(iov, iovcnt, skip, &offset)) < iovcnt) {

Hrm, the easiest conversion is to use (..., skip, &skip). Using a new variable means we'd need to feed that back into skip somehow on the next loop.

if (offset) ...

which I don't really find brilliant, but at least we don't do if (skip) where 'skip' used to be something completely different.

So.. the version I have now you might not like based on this comment, because it still has 'skip' essentially become a local variable with a different meaning from the one it has at entry.

...

+ ssize_t rc; + + if (skip)

Curly brackets here for consistency (undecided about readability to be honest).

Uh.. consistency with what? We don't typically brace single line clauses in passt.

...

+ rc = write(fd, (char *)iov[i].iov_base + skip, + iov[i].iov_len - skip); + else + rc = writev(fd, &iov[i], iovcnt - i); + + if (rc < 0) + return -1; + skip += rc; + } + + return 0; +} diff --git a/util.h b/util.h index 62fad6fe..ee380f55 100644 --- a/util.h +++ b/util.h @@ -230,6 +230,7 @@ int __daemon(int pidfile_fd, int devnull_fd); int fls(unsigned long x); int write_file(const char *path, const char *buf); size_t iov_offset(const struct iovec *iov, size_t n, size_t *offset); +int write_remainder(int fd, const struct iovec *iov, int iovcnt, size_t skip);

/** * mod_sub() - Modular arithmetic subtraction

-- 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

On Thu, 22 Feb 2024 16:56:01 +1100 David Gibson wrote:

pcap_frame() explicitly takes a single frame, and only allows a single buffer (iovec) to be passed. pcap_multiple() takes multiple buffers, but explicitly expects exactly one frame per buffer.

Future changes are going to want to split single frames across multiple buffers in some circumstances, so extend the pcap functions to allow for that.

Signed-off-by: David Gibson --- pcap.c | 24 ++++++++++++++---------- pcap.h | 3 ++- tap.c | 2 +- 3 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/pcap.c b/pcap.c index eeb71a3c..9eb4f3d2 100644 --- a/pcap.c +++ b/pcap.c @@ -31,6 +31,7 @@ #include "util.h" #include "passt.h" #include "log.h" +#include "pcap.h"

#define PCAP_VERSION_MINOR 4

@@ -67,14 +68,15 @@ struct pcap_pkthdr {

/** * pcap_frame() - Capture a single frame to pcap file with given timestamp - * @iov: iovec referencing buffer containing frame (with L2 headers) - * @offset: Offset of the frame from @iov->iov_base + * @iov: IO vector containing frame (with L2 headers and tap headers) + * @iovcnt: Number of buffers (@iov entries) in frame + * @offset: Byte offset of the L2 headers within @iov

I swear I didn't read "Byte offset" from here before commenting on 1/6, which is rather promising.

* @tv: Timestamp * * Returns: 0 on success, -errno on error writing to the file */ -static void pcap_frame(const struct iovec *iov, size_t offset, - const struct timeval *tv) +static void pcap_frame(const struct iovec *iov, size_t iovcnt, + size_t offset, const struct timeval *tv) { size_t len = iov->iov_len - offset; struct pcap_pkthdr h = { @@ -86,7 +88,7 @@ static void pcap_frame(const struct iovec *iov, size_t offset, struct iovec hiov = { &h, sizeof(h) };

if (write_remainder(pcap_fd, &hiov, 1, 0) < 0 || - write_remainder(pcap_fd, iov, 1, offset) < 0) + write_remainder(pcap_fd, iov, iovcnt, offset) < 0) debug("Cannot log packet, length %zu: %s", len, strerror(errno)); } @@ -105,16 +107,18 @@ void pcap(const char *pkt, size_t len) return;

gettimeofday(&tv, NULL); - pcap_frame(&iov, 0, &tv); + pcap_frame(&iov, 1, 0, &tv); }

/** * pcap_multiple() - Capture multiple frames - * @iov: Array of iovecs, one entry per frame + * @iov: Array of iovecs, every @framebufs entries is one frame + * @framebufs: Number of buffers per frame

I found this a bit hard to understand. What about: * @iov: Array of IO vectors, with item count @frame_parts * @n * @frame_parts: Number of IO vector items for each frame ? The rest of the series looks good to me. -- Stefano