Re: [PATCH 1/6] util: Add helper to find offset into io vector

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 5226 bytes --]

On Tue, Feb 27, 2024 at 03:23:35PM +0100, Stefano Brivio wrote:
> On Thu, 22 Feb 2024 16:55:57 +1100
> David Gibson <david@gibson.dropbear.id.au> 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 <david@gibson.dropbear.id.au>
> > ---
> >  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.

To avoid that shuffle, I cherry picked Laurent's first patch into my
series, and rebased putting this function directly into iov.c.
Several of Laurent's functions can also be slightly simplified using
this helper, so I've done that.

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

I guess I thought it was redundant with the comment a few lines down?
Anyway, I've put it back in.

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

Yeah, that's fair.  I was think of "iov_offset" as "offset the IOV",
but "offset *of* the IOV" is probably a more obvious interpretation.

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

Hm, I find this name and and one line description confusing in a
different way from mine.  In my next spin, I've tried to synthesize
your suggestion and mine, into a version under the name
iov_skip_bytes().

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

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]