> Date: Wed, 26 Dec 2018 10:03:15 +0900 > From: Shoichi Yamaguchi <yamagu...@netbsd.org> > > > I implemented a patch that make vioif(4) support multi-queue. And I have put > > the patch on ftp.n.o. I used vioif(4) multiqueue on qemu-kvm on Linux kernel > > 4.19.5. And It seems to be working fine. > > > > https://ftp.netbsd.org/pub/NetBSD/misc/yamaguchi/vioif_mutilq.patch > > Do you have any comments? > I would like to going to commit the patch if there is no comment until > tomorrow.
Hi, Yamaguchi-san! A lot of Americans and Europeans are on vacation this time of year, so it might be better to hold off for another week or two. Here's a quick review -- I don't know anything about virtio, so this is just about use of kernel APIs and abstractions. Someone who knows something about virtio should take a look too. > diff --git a/sys/dev/pci/if_vioif.c b/sys/dev/pci/if_vioif.c > index 3bbd300e88e..769b108e793 100644 > --- a/sys/dev/pci/if_vioif.c > +++ b/sys/dev/pci/if_vioif.c > > /* Feature bits */ > -#define VIRTIO_NET_F_CSUM (1<<0) > [...] > +#define VIRTIO_NET_F_MQ (1<<22) If you're going to modify all of the lines here, maybe take the opportunity to convert them to use __BIT? > @@ -171,73 +184,110 @@ struct virtio_net_ctrl_vlan { > [...] > /* > * if_vioifvar.h: > */ > +struct vioif_txqueue { > + struct virtqueue *txq_vq; Why not make this an embedded structure? struct vioif_txqueue { struct virtqueue txq_vq; ... }; struct vioif_softc { struct vioif_txqueue *sc_txq; struct vioif_rxqueue *sc_rxq; struct vioif_ctrlqueue *sc_ctrlq; ... }; > + kmutex_t *txq_lock; Why is this a pointer to kmutex_t with mutex_obj_alloc/free and not just a kmutex_t with mutex_init/destroy? Is it reused anywhere? If it is reused, this needs explanation in the comments. If it is not, just use kmutex_t. Can you write a comment summarizing what locks cover what fields, and, if more than one lock can be held at once, what the lock order is? > +struct vioif_rxqueue { > + struct virtqueue *rxq_vq; > + kmutex_t *rxq_lock; Likewise. > -#define VIOIF_TX_LOCK(_sc) mutex_enter(&(_sc)->sc_tx_lock) > -#define VIOIF_TX_UNLOCK(_sc) mutex_exit(&(_sc)->sc_tx_lock) > -#define VIOIF_TX_LOCKED(_sc) mutex_owned(&(_sc)->sc_tx_lock) > -#define VIOIF_RX_LOCK(_sc) mutex_enter(&(_sc)->sc_rx_lock) > -#define VIOIF_RX_UNLOCK(_sc) mutex_exit(&(_sc)->sc_rx_lock) > -#define VIOIF_RX_LOCKED(_sc) mutex_owned(&(_sc)->sc_rx_lock) > +#define VIOIF_TXQ_LOCK(_q) mutex_enter((_q)->txq_lock) > +#define VIOIF_TXQ_TRYLOCK(_q) mutex_tryenter((_q)->txq_lock) > +#define VIOIF_TXQ_UNLOCK(_q) mutex_exit((_q)->txq_lock) > +#define VIOIF_TXQ_LOCKED(_q) mutex_owned((_q)->txq_lock) > + > +#define VIOIF_RXQ_LOCK(_q) mutex_enter((_q)->rxq_lock) > +#define VIOIF_RXQ_UNLOCK(_q) mutex_exit((_q)->rxq_lock) > +#define VIOIF_RXQ_LOCKED(_q) mutex_owned((_q)->rxq_lock) Can we just use mutex_enter/exit/&c. without the macros? Sometimes we use macros where they are conditional, depending on NET_MPSAFE, but if there's no need for that, I would prefer to read direct calls to mutex_enter/exit/&c. > +static int > +vioif_alloc_queues(struct vioif_softc *sc) > +{ > + int nvq_pairs = sc->sc_max_nvq_pairs; > + int nvqs = nvq_pairs * 2; > + int i; > + > + sc->sc_rxq = kmem_zalloc(sizeof(sc->sc_rxq[0]) * nvq_pairs, > + KM_NOSLEEP); > + if (sc->sc_rxq == NULL) > + return -1; > + > + sc->sc_txq = kmem_zalloc(sizeof(sc->sc_txq[0]) * nvq_pairs, > + KM_NOSLEEP); > + if (sc->sc_txq == NULL) > + return -1; Check to avoid arithmetic overflow here: if (nvq_pairs > INT_MAX/2 - 1 || nvq_pairs > SIZE_MAX/sizeof(sc->sc_rxq[0])) return -1; nvqs = nvq_pairs * 2; if (...) nvqs++; sc->sc_rxq = kmem_zalloc(sizeof(sc->sc_rxq[0]) * nvq_pairs, ...); Same in all the other allocations like this. (We should have a kmem_allocarray -- I have a draft somewhere.) > @@ -586,69 +759,109 @@ vioif_attach(device_t parent, device_t self, void *aux) > [...] > + /* Limit the number of queue pairs to use */ > + if (sc->sc_max_nvq_pairs <= ncpu) > + sc->sc_req_nvq_pairs = sc->sc_max_nvq_pairs; > + else > + sc->sc_req_nvq_pairs = ncpu; How about sc->sc_req_nvq_pairs = MIN(sc->sc_max_nvq_pairs, ncpu)? > +static void > +vioif_ctrl_release(struct vioif_softc *sc) > +{ > + struct vioif_ctrlqueue *ctrlq = &sc->sc_ctrlq; > + > + mutex_enter(&ctrlq->ctrlq_wait_lock); KASSERT(ctrlq->ctrlq_inuse != FREE) Might be helpful to record the lwp that owns this ctrlq, too, for diagnostics: KASSERT(ctrlq->ctrlq_owner == curlwp). > diff --git a/sys/dev/pci/virtio_pci.c b/sys/dev/pci/virtio_pci.c > index 65c5222b774..bb972997be2 100644 > --- a/sys/dev/pci/virtio_pci.c > +++ b/sys/dev/pci/virtio_pci.c > @@ -604,8 +677,14 @@ virtio_pci_setup_interrupts(struct virtio_softc *sc) > [...] > if (pci_intr_type(pc, psc->sc_ihp[0]) == PCI_INTR_TYPE_MSIX) { > - psc->sc_ihs = kmem_alloc(sizeof(*psc->sc_ihs) * 2, > + psc->sc_ihs = kmem_alloc(sizeof(*psc->sc_ihs) * nmsix, > KM_SLEEP); Check for arithmetic overflow here.