Re: [PATCH 01/24] iov: add some functions to manage iovec

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

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

On Tue, Feb 06, 2024 at 03:28:10PM +0100, Laurent Vivier wrote:
> On 2/5/24 06:57, David Gibson wrote:
> > On Fri, Feb 02, 2024 at 03:11:28PM +0100, Laurent Vivier wrote:
> > ...
> > > diff --git a/iov.c b/iov.c
> > > new file mode 100644
> > > index 000000000000..38a8e7566021
> > > --- /dev/null
> > > +++ b/iov.c
> > > 
> > > +	for (i = 0, done = 0; (offset || done < bytes) && i < iov_cnt; i++) {
> > Not immediately seeing why you need the 'offset ||' part of the condition.
> 
> In fact the loop has two purposes:
> 
> 1- scan the the iovec to reach byte offset in the iov (so until offset is 0)
> 
> 2- copy the bytes (until done == byte)
> 
> It could be written like this:
> 
> for (i = 0; offset && i < iov_cnt && offset >= iov[i].iov_len ; i++)
>         offset -= iov[i].iov_len;
> 
> for (done = 0; done < bytes && i < iov_cnt; i++) {
>     size_t len = MIN(iov[i].iov_len - offset, bytes - done);
>     memcpy((char *)iov[i].iov_base + offset, (char *)buf + done, len);
>     done += len;
> }

Right, but done starts at 0 and will remain zero until you reach the
first segment where you need to copy something.  So, unless bytes ==
0, then done < bytes will always be true when offset != 0.  And if
bytes *is* zero, then there's nothing to do, so there's no need to
step through the vector at all.

> ...
> 
> > > +unsigned iov_copy(struct iovec *dst_iov, unsigned int dst_iov_cnt,
> > > +		  const struct iovec *iov, unsigned int iov_cnt,
> > > +		  size_t offset, size_t bytes)
> > > +{
> > > +	size_t len;
> > > +	unsigned int i, j;
> > > +	for (i = 0, j = 0;
> > > +		 i < iov_cnt && j < dst_iov_cnt && (offset || bytes); i++) {
> > > +		if (offset >= iov[i].iov_len) {
> > > +			offset -= iov[i].iov_len;
> > > +			continue;
> > > +		}
> > > +		len = MIN(bytes, iov[i].iov_len - offset);
> > > +
> > > +		dst_iov[j].iov_base = (char *)iov[i].iov_base + offset;
> > > +		dst_iov[j].iov_len = len;
> > > +		j++;
> > > +		bytes -= len;
> > > +		offset = 0;
> > > +	}
> > > +	return j;
> > > +}
> > Small concern about the interface to iov_copy().  If dst_iov_cnt <
> > iov_cnt and the chunk of the input iovec you want doesn't fit in the
> > destination it will silently truncate - you can't tell if this has
> > happened from the return value.  If the assumption is that dst_iov_cnt = iov_cnt,
> > then there's not really any need to pass it.
> 
> In fact this function will be removed with "vhost-user: use guest buffer
> directly in vu_handle_tx()" as it is not needed when we use directly guest
> buffers. So I don't think we need to improve this.

Ok, fair enough.

> If I remember correctly I think it behaves like passt already does with
> socket backend: if it doesn't fit, it's dropped silently.
> 
> I've fixed all your other comments.
> 
> Thanks,
> Laurent
> 

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