> > > > https://ftp.netbsd.org/pub/NetBSD/misc/yamaguchi/vioif_mutilq.patch
I have updated the patch. 2018年12月26日(水) 17:20 s ymgch <[email protected]>: > > Thank you for your helpful comments! > > > 2018年12月26日(水) 11:37 Taylor R Campbell <[email protected]>: > > > > > Date: Wed, 26 Dec 2018 10:03:15 +0900 > > > From: Shoichi Yamaguchi <[email protected]> > > > > > > > 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. > > > > > > > > > > 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. > > Yes, indeed. I'll wait for other comments for more one or two week. > > > > > > 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? > > The reason why I don't use "struct virtqueue" but use "struct virtqueue *" > is to register a virtqueue array ("struct virtqueue []") to virtio(4) > as an argument > of virtio_child_attach_start() or virtio_child_attach_set_vqs(). Virtqueues > used > in a child device of virtio(4) like vioif(4) must be array. > > > > > 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. > > It is for the error handling code. > Example: > for (...) { > txq[i]->txq_lock = mutex_obj_alloc(); > s = softint_establish() > if (s == NULL) > goto err; > } > .... > err: > for (...) > if (txq[i]->txq_lock) > mutex_obj_free(txq->txq_lock); > > I use the pointer value as a flag weather the lock already allocated or not, > and > I didn't want to add a field into vioif_txqueue to save it. > > > > > 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? > > I'll add comments for the locks. > Currently, the locks cover all fields in the structure, and two or more > than locks can't be held at once. > > > > > > +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.) > > nvq_pairs is always less than VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MAX(= 0x8000)
