Re: [PATCH 1/1] iov: iov tail helpers

[Stefano Brivio <sbrivio@redhat.com>]

On Wed, 6 Nov 2024 13:38:38 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Wed, Nov 06, 2024 at 01:56:31AM +0100, Stefano Brivio wrote:
> > On Tue,  5 Nov 2024 13:32:22 +1100
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > In the vhost-user code we have a number of places where we need to locate
> > > a particular header within the guest-supplied IO vector.  We need to work
> > > out which buffer the header is in, and verify that it's contiguous and
> > > aligned as we need.  At the moment this is open-coded, but introduce a
> > > helper to make this more straightforward.
> > > 
> > > We add a new datatype 'struct iov_tail' representing an IO vector from
> > > which we've logically consumed some number of headers.  The IOV_PULL_HEADER
> > > macro consumes a new header from the vector, returning a pointer and
> > > updating the iov_tail.  
> > 
> > The interfaces look usable and straightforward to me. I find some names
> > and comments a bit obscure, though.  
> 
> Yeah, I don't love the names either.
> 
> > First off, I would intuitively say that the "tail" is always at the
> > end, and if we already consumed something, that's always at the "head".  
> 
> Right.. which is true in a sense.  The idea is you'd set one of these
> up, to cover a whole (say) frame, then pull bits off the front as you
> need it.  So, the iov_tail does represent the "tail", as in the
> unprocessed bit of the frame at each point...
> 
> > If we call the whole abstraction "tail", we risk ending up talking about
> > the tail of the tail, and the head of the tail. Consider this part from
> > the cover letter:  
> 
> .. but, yeah, heads of tails and tails of tails gets confusing.
> Unless we rewrite in LISP, I guess.
> 
> > > "iov tail", that is an iov from which you've
> > > already consumed (in some sense) some data from the beginning.   
> > 
> > ...in other words, that's an IO vector called tail, and we
> > already consumed some data from its head.
> > 
> > What about (iov-based) "batch"?  
> 
> I don't think "batch" is really any better, it's just unclear on a
> different set of axes.
> 
> Would "iov remainder" be any better?

Now that I read your reply, the "tail" name becomes clearer as it's
clear that we just use it for left-overs. I don't think "remainder"
makes it much clearer, so I would rather stick to "tail" and try to
explain the purpose of struct iov_tail a bit more specifically (see
below).

> > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > ---
> > >  iov.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  iov.h | 24 +++++++++++++++++
> > >  2 files changed, 107 insertions(+)
> > > 
> > > diff --git a/iov.c b/iov.c
> > > index 3f9e229..3d384ae 100644
> > > --- a/iov.c
> > > +++ b/iov.c
> > > @@ -156,3 +156,86 @@ size_t iov_size(const struct iovec *iov, size_t iov_cnt)
> > >  
> > >  	return len;
> > >  }
> > > +
> > > +/**
> > > + * iov_tail_shorten() - Remove any buffers from an IOV tail that are wholly consumed  
> > 
> > "Remove" is a bit difficult to interpret (does it deallocate? Throw
> > data away^), I would rather say that we... detach (?) those buffers from
> > the batch/tail.  
> 
> Yeah, I'm not sure how to express this either.  In a sense this
> operation is a logical no-op: it shouldn't change the results of any
> future operation, but it might make them slightly faster.

Oh, this wasn't clear to me. I guess that "reduce", "minimise", or
"prune" would convey this better.

> > This operation itself raises a question though: if the batch already
> > carries the information that some buffers were completely consumed,
> > should it ever be in a state where we want to drop these buffers from
> > it?
> > 
> > That is, it sounds like we have some other operation that allows it to
> > be in an inconsistent state.  
> 
> Sort of, yes, but I think this is the right design choice.  It means
> we can trivially construct one of these things with an arbitrary
> offset, and we only do the work of stepping through the buffers when
> we actually have to.  We can discard bytes simply by adding to the
> offset, without having to look at the actual buffers until later.
> 
> Basically if we want to ensure that the representation is always
> "minimal", in the sense that the offset lies within the first buffer,
> then a bunch more operations need to do work to maintain that.  Plus,
> this approach is naturally robust: if we somehow get an iov_tail in
> non-minimal form, peek/pull will just handle it with no extra logic.
> 
> > > + * @tail:	IO vector tail (modified)
> > > + *
> > > + * Return:	true if the tail still contains any bytes, otherwise false
> > > + */
> > > +bool iov_tail_shorten(struct iov_tail *tail)
> > > +{
> > > +	size_t i;
> > > +
> > > +	i = iov_skip_bytes(tail->iov, tail->cnt, tail->off, &tail->off);
> > > +	tail->iov += i;
> > > +	tail->cnt -= i;
> > > +
> > > +	return !!tail->cnt;
> > > +}
> > > +
> > > +/**
> > > + * iov_tail_size - Calculate the total size of an IO vector tail
> > > + * @tail:	IO vector tail
> > > + *
> > > + * Returns:    The total size in bytes.
> > > + */
> > > +/* cppcheck-suppress unusedFunction */
> > > +size_t iov_tail_size(struct iov_tail *tail)
> > > +{
> > > +	iov_tail_shorten(tail);
> > > +	return iov_size(tail->iov, tail->cnt) - tail->off;
> > > +}
> > > +
> > > +/**
> > > + * iov_peek_header_() - Get pointer to header from an IOV tail  
> > 
> > I think that this needs to be more generic than "header", because yes,
> > we're using it for headers, but that word doesn't really help in this
> > context.  
> 
> Hm.  Well, we are using it for headers, and we're also always pulling
> from the "head" of whatever we have left.
> 
> > What about "aligned block", or just "block"?  
> 
> Maybe... but it really does have to be from the start of the current
> tail.

"Head block"? Or forget about it, "header" is actually fine.

> >   
> > > + * @tail:	IO vector tail to get header from
> > > + * @len:	Length of header to remove in bytes  
> > 
> > to remove, in bytes
> >   
> > > + * @align:	Required alignment of header in bytes  
> > 
> > Judging from this comment alone, it's not clear if 0 or 1 should be
> > used to get freely aligned blocks.
> >   
> > > + *
> > > + * @tail may be modified, but will be semantically equivalent.
> > > + *
> > > + * Returns:	Pointer to the removed header, NULL if it overruns the IO
> > > + *		vector, is not contiguous or is misaligned.
> > > + */
> > > +void *iov_peek_header_(struct iov_tail *tail, size_t len, size_t align)
> > > +{
> > > +	char *p;
> > > +
> > > +	if (!iov_tail_shorten(tail))
> > > +		return NULL; /* Nothing left */
> > > +
> > > +	if (tail->off + len < tail->off)
> > > +		return NULL; /* Overflow */
> > > +
> > > +	if (tail->off + len > tail->iov[0].iov_len)
> > > +		return NULL; /* Not contiguous */  
> > 
> > I'm not sure if this observation is useful in some cases, but this
> > doesn't necessarily mean that the header/block is not contiguous: if
> > tail->iov[0].iov_base + tail->iov[0].iov_len == tail->iov[1].iov_base,
> > it actually is.  
> 
> Hm, true.  Not sure if it's worth the tests handle that case though.

I guess it's worth it if there's any chance we'll ever want to support
split headers.

> > > +	p = (char *)tail->iov[0].iov_base + tail->off;
> > > +	if ((uintptr_t)p % align)
> > > +		return NULL; /* not aligned */
> > > +
> > > +	return p;
> > > +}
> > > +/**
> > > + * iov_pull_header_() - Remove a header from an IOV tail  
> > 
> > I know that "pulling" is widely used, but it's sometimes ambiguous (I
> > guess we already had a discussion about that in the past). What about
> > "remove", or "drop"?  
> 
> "remove" might work.  I don't like "drop" because that implies to me
> it's just gone, rather than returned.
> 
> > > + * @tail:	IO vector tail to remove header from (modified)
> > > + * @len:	Length of header to remove in bytes
> > > + * @align:	Required alignment of header in bytes
> > > + *
> > > + * @tail is updated so that it no longer includes the extracted header
> > > + *
> > > + * Returns:	Pointer to the removed header, NULL if it overruns the IO
> > > + *		vector, is not contiguous or is misaligned.
> > > + */
> > > +/* cppcheck-suppress unusedFunction */
> > > +void *iov_pull_header_(struct iov_tail *tail, size_t len, size_t align)
> > > +{
> > > +	char *p = iov_peek_header_(tail, len, align);
> > > +
> > > +	if (!p)
> > > +		return NULL;
> > > +
> > > +	tail->off = tail->off + len;  
> > 
> > This could just be += I guess.  
> 
> True.
> 
> > > +	return p;
> > > +}
> > > diff --git a/iov.h b/iov.h
> > > index a9e1722..a2f449c 100644
> > > --- a/iov.h
> > > +++ b/iov.h
> > > @@ -28,4 +28,28 @@ size_t iov_from_buf(const struct iovec *iov, size_t iov_cnt,
> > >  size_t iov_to_buf(const struct iovec *iov, size_t iov_cnt,
> > >                    size_t offset, void *buf, size_t bytes);
> > >  size_t iov_size(const struct iovec *iov, size_t iov_cnt);
> > > +
> > > +/**
> > > + * struct iov_tail - Represents the fail portion of an IO vector  

...so, it's not just a tail portion, it's a tail portion in a rather
specific and constrained context, and once that's revealed these
helpers are easier (for me) to understand. What about:

 * struct iov_tail - Remaining, not consumed portion (tail) of IO vector

?

> > s/fail/tail/
> >   
> > > + * @iov:	IO vector
> > > + * @cnt:	Number of entries in @iov
> > > + * @off:	Current offset in @iov
> > > + */
> > > +struct iov_tail {
> > > +	const struct iovec *iov;
> > > +	size_t cnt, off;
> > > +};
> > > +
> > > +#define IOV_TAIL(iov_, cnt_, off_) \
> > > +	(struct iov_tail){ .iov = (iov_), .cnt = (cnt_), .off = (off_) }
> > > +
> > > +bool iov_tail_shorten(struct iov_tail *tail);
> > > +size_t iov_tail_size(struct iov_tail *tail);
> > > +void *iov_peek_header_(struct iov_tail *tail, size_t len, size_t align);
> > > +#define IOV_PEEK_HEADER(tail_, ty_) \
> > > +	((ty_ *)(iov_peek_header_((tail_), sizeof(ty_), __alignof__(ty_))))  
> > 
> > I guess 'x' would be as clear as 'ty_' (actually, I'm failing to guess
> > what it stands for).  
> 
> "type"

Oh. That's kind of obscure. I guess it would be better to actually
spell that out and define this on multiple lines.

> > > +void *iov_pull_header_(struct iov_tail *tail, size_t len, size_t align);
> > > +#define IOV_PULL_HEADER(tail_, ty_) \
> > > +	((ty_ *)(iov_pull_header_((tail_), sizeof(ty_), __alignof__(ty_))))
> > > +
> > >  #endif /* IOVEC_H */  
> > 
> > I would have expected some functions to add data or build those
> > tails... or we don't need them for some reason?  
> 
> The IOV_TAIL() macro builds one.  Adding data doesn't make sense; the
> idea is this is a view into part of an existing IO vector, which
> doesn't require copying the struct iovecs of that original vector.

I see now, I didn't pay much attention to that. Perhaps some one-liner
function-like comments around IOV_TAIL(), IOV_PEEK_HEADER() and
IOV_PULL_HEADER() would help.

-- 
Stefano