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