Page MenuHomeFreeBSD

bhyve/virtio: Rework iovec handling functions for efficiency and clarity
Needs ReviewPublic

Authored by rosenfeld_grumpf.hope-2000.org on Oct 30 2025, 7:38 AM.
Tags
None
Referenced Files
F145333508: D53468.diff
Wed, Feb 18, 12:44 PM
Unknown Object (File)
Tue, Feb 17, 2:25 AM
Unknown Object (File)
Tue, Feb 10, 9:32 PM
Unknown Object (File)
Tue, Jan 27, 3:43 AM
Unknown Object (File)
Jan 16 2026, 6:39 AM
Unknown Object (File)
Jan 16 2026, 4:42 AM
Unknown Object (File)
Jan 8 2026, 2:05 PM
Unknown Object (File)
Jan 5 2026, 6:38 PM

Details

Reviewers
jhb
corvink
markj
Group Reviewers
bhyve
Summary

Add check_iov_len() to check whether an iovec array covers a certain
length without the need to call count_iov() on the whole array first.

Garbage-collect the 'seek' argument to buf_to_iov(), used only by
virtio-scsi control request handling. The apparent benefit of using it
to copy only the final status byte instead of the whole TMF or AN
request (25 and 21 bytes, respectively) is dubious at best, given that
the extra code to handle this in buf_to_iov() allocates and frees a new
iovec array and uses seek_iov(), which traverses the whole array and
copies iovecs around.

Replace seek_iov() and truncate_iov(), used only by virtio-scsi, with
a single function split_iov() which combines the functionality of both
in a more efficient way:
While seek_iov() always copies all iovecs past the seek offset into a
new iovec array, split_iov() works in place and doesn't copy iovecs
unless actually necessary. By using split_iov(), we can avoid almost
all copying of iovecs in I/O handling code paths of virtio-scsi.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 70549
Build 67432: arc lint + arc unit

Event Timeline

Thanks for splitting your initial commit! Note that this can also be split into smaller commits to make it easier to read: add check_iov_len, update buf_to_iov and replace seek + truncate by split. While I'm personally preferring splitting the commits, I'd be fine keeping it as is too.

usr.sbin/bhyve/iov.c
36

It's already included by iov.h.

usr.sbin/bhyve/iov.h
41

Why removing the parameter names from existing functions?

usr.sbin/bhyve/pci_virtio_scsi.c
493–521

IMO, it's a bit confusing, that we're passing two pointers here which are related to each other. Wouldn't it make things more clear to pass a single iov pointer and then passing an niov_in and niov_out length to this function?

Rework split_iov() as requested by Corvin.

usr.sbin/bhyve/iov.c
36

I think that to prevent header dependency issues, every source or header file should always include every header file whose definitions and declarations it uses directly. This file uses now uses the standard bool type and thus needs to include stdbool.h.

usr.sbin/bhyve/iov.h
41

Given that these parameter names in function prototypes are ignored by the compiler, not even causing a warning if the name is different from what the actual function implementation is using, they serve very little purpose if any purpose at all. I generally consider them noise that adds no value but clutters up the code and lengthens lines needlessly.

For this reason I don't add them for new functions or functions that I change, and in a case like this where it's relatively unintrusive I change the others, too, to make it all nice and consistent.

usr.sbin/bhyve/pci_virtio_scsi.c
493–521

I reworked it to return the data_iov pointer instead and streamlined the split_iov() code somewhat. Please let me know what you think?

rosenfeld_grumpf.hope-2000.org retitled this revision from bhyve/virtio: rework iovec handling functions for efficiency and clarity to bhyve/virtio: Rework iovec handling functions for efficiency and clarity.Mon, Feb 9, 4:23 PM

Sync with latest illumos bits after code review.

Use a size_t type where appropriate instead of the abomination that is uint.

Looks ok to me aside from the comment on split_iov().

usr.sbin/bhyve/iov.c
45

Conceptually, this function is splitting an iovec array in two and returning the number of entries on the left side in *niov, and the number of entries on the right side in *niov_rem. I think I have trouble with the meaning of "remaining" here, I would naively expect *niov_rem to be the number of iovecs before offset, not after. More generally, the "remainder" is what's left of the original after you take something away, but here it's used to refer to that which is being taken away, so the implementation is somewhat confusing to me.

Can we please rename the variables and reword the comment accordingly?

132

Not something to be fixed in this patch, but we should be checking for overflow here...

145

Same here with respect to overflow.