Date: Wed, 22 May 2019 21:07:14 +0000 From: "v.maffione_gmail.com (Vincenzo Maffione)" <phabric-noreply@FreeBSD.org> To: Phabricator <phabric-noreply@FreeBSD.org> Cc: freebsd-virtualization@freebsd.org Subject: [Differential] D20276: [bhyve][virtio-net] Allow guest VM's to set JUMBO MTU in case of using the VALE switch. Message-ID: <262ea68ccd1ee1967080792acdd69ba6@localhost.localdomain> In-Reply-To: <differential-rev-PHID-DREV-pueoqfdkfh54jyuh6emz-req@reviews.freebsd.org> References: <differential-rev-PHID-DREV-pueoqfdkfh54jyuh6emz-req@reviews.freebsd.org>
index | next in thread | previous in thread | raw e-mail
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, bcranhelp
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?262ea68ccd1ee1967080792acdd69ba6>