[Differential] D20276: [bhyve][virtio-net] Allow guest VM's to set JUMBO MTU in case of using the VALE switch.

[v.maffione_gmail.com (Vincenzo Maffione)]

v.maffione_gmail.com added a comment.


  Netmap usage by itself looks mostly ok, except where noted. We could improve the management of r->cur in certain cases, but it's not particularly important for now (for reference, please look at my QEMU implementation, for instance the transmit routine is here https://github.com/qemu/qemu/blob/master/net/netmap.c#L160-L225).
  I still need to review your changes to the virtqueue processing. But why did you drop vq_getchain() and write a custom method? If another method is needed, it should be added to virtio.c IMHO.

INLINE COMMENTS

> pci_virtio_net.c:468
>  
> -		if (nm_ring_empty(ring)) {
> -			r++;
> -			if (r > nmd->last_rx_ring)
> -				r = nmd->first_rx_ring;
> -			if (r == nmd->cur_rx_ring)
> -				break;
> -			continue;
> +	i = r->cur;
> +	buf = NETMAP_BUF(r, r->slot[i].buf_idx);

r->head is better than r->cur, although there is no difference right now (see comment below).

> pci_virtio_net.c:496
> +	r->head = r->cur = nm_ring_next(r, i);
> +	ioctl(nmd->fd, NIOCRXSYNC, NULL);
> +

In theory this NIOCRXSYNC is not needed, because poll() or kqueue_wait() calls NIOCRXSYNC internally. This works perfectly with poll(), at least. As far as I know bhyve uses kqueue to wait on the netmap file descriptor. What happens if you remove this ioctl()?

> pci_virtio_net.c:573
> +
> +	for (i = r->cur; i != r->tail; i = nm_ring_next(r, i)) {
> +		len += r->slot[i].len;

You should use r->head to get the next to use. r->cur is for synchronization, and points at the next unseen, which may be ahead of r->head, although not your code.
This may cause problems in the future.

> pci_virtio_net.c:588
> +
> +	for (i = r->cur; i != r->tail; i = nm_ring_next(r, i)) {
> +		if (!(r->slot[i].flags & NS_MOREFRAG)) {

r->head rather than r->cur

CHANGES SINCE LAST ACTION
  https://reviews.freebsd.org/D20276/new/

REVISION DETAIL
  https://reviews.freebsd.org/D20276

EMAIL PREFERENCES
  https://reviews.freebsd.org/settings/panel/emailpreferences/

To: aleksandr.fedorov_itglobal.com, #bhyve, jhb, rgrimes, krion, v.maffione_gmail.com
Cc: mizhka_gmail.com, novel, olevole_olevole.ru, freebsd-virtualization-list, evgueni.gavrilov_itglobal.com, bcran